diff --git a/modules/4337/.solhint.json b/modules/4337/.solhint.json index 49bc4977c..6f92cb60a 100644 --- a/modules/4337/.solhint.json +++ b/modules/4337/.solhint.json @@ -13,6 +13,6 @@ "reason-string": "off", "no-empty-blocks": "off", "avoid-low-level-calls": "off", - "custom-errors": "off" + "gas-custom-errors": "off" } } diff --git a/modules/passkey/.solhint.json b/modules/passkey/.solhint.json index 49bc4977c..6f92cb60a 100644 --- a/modules/passkey/.solhint.json +++ b/modules/passkey/.solhint.json @@ -13,6 +13,6 @@ "reason-string": "off", "no-empty-blocks": "off", "avoid-low-level-calls": "off", - "custom-errors": "off" + "gas-custom-errors": "off" } } diff --git a/modules/passkey/contracts/libraries/WebAuthn.sol b/modules/passkey/contracts/libraries/WebAuthn.sol index 77fd6eb2b..05b730def 100644 --- a/modules/passkey/contracts/libraries/WebAuthn.sol +++ b/modules/passkey/contracts/libraries/WebAuthn.sol @@ -70,18 +70,55 @@ library WebAuthn { /** * @notice Casts calldata bytes to a WebAuthn signature data structure. * @param signature The calldata bytes of the WebAuthn signature. + * @return isValid Whether or not the encoded signature bytes is valid. * @return data A pointer to the signature data in calldata. - * @dev This method casts the dynamic bytes array to a signature calldata pointer without - * additional verification. This is not a security issue for the WebAuthn implementation, as any - * signature data that would be represented from an invalid `signature` value, could also be - * encoded by a valid one. It does, however, mean that callers into the WebAuthn signature - * verification implementation might not validate as much of the data that they pass in as they - * would expect. With that in mind, callers should not rely on the encoding being verified. + * @dev This method casts the dynamic bytes array to a signature calldata pointer with some + * additional verification. Specifically, we ensure that the signature bytes encoding is no + * larger than standard ABI encoding form, to prevent attacks where valid signatures are padded + * with 0s in order to increase signature verifications the costs for ERC-4337 user operations. */ - function castSignature(bytes calldata signature) internal pure returns (Signature calldata data) { + function castSignature(bytes calldata signature) internal pure returns (bool isValid, Signature calldata data) { + uint256 authenticatorDataLength; + uint256 clientDataFieldsLength; + // solhint-disable-next-line no-inline-assembly assembly ("memory-safe") { data := signature.offset + + // Read the lengths of the dynamic byte arrays in assembly. This is done because + // Solidity generates calldata bounds checks which aren't required for the security of + // the signature verification, as it can only lead to _shorter_ signatures which are + // are less gas expensive anyway. + authenticatorDataLength := calldataload(add(data, calldataload(data))) + clientDataFieldsLength := calldataload(add(data, calldataload(add(data, 0x20)))) + } + + // Use of unchecked math as any overflows in dynamic length computations would cause + // out-of-gas reverts when computing the WebAuthn signing message. + unchecked { + // Allow for signature encodings where the dynamic bytes are aligned to 32-byte + // boundaries. This allows for high interoperability (as this is how Solidity and most + // tools `abi.encode`s the `Signature` struct) while setting a strict upper bound to how + // many additional padding bytes can be added to the signature, increasing gas costs. + // Note that we compute the aligned lengths with the formula: `l + 0x1f & ~0x1f`, which + // rounds `l` up to the next 32-byte boundary. + uint256 alignmentMask = ~uint256(0x1f); + uint256 authenticatorDataAlignedLength = (authenticatorDataLength + 0x1f) & alignmentMask; + uint256 clientDataFieldsAlignedLength = (clientDataFieldsLength + 0x1f) & alignmentMask; + + // The fixed parts of the signature length are 6 32-byte words for a total of 192 bytes: + // - offset of the `authenticatorData` bytes + // - offset of the `clientDataFields` string + // - signature `r` value + // - signature `s` value + // - length of the `authenticatorData` bytes + // - length of the `clientDataFields` string + // + // This implies that the signature length must be less than or equal to: + // 192 + authenticatorDataAlignedLength + clientDataFieldsAlignedLength + // which is equivalent to strictly less than: + // 193 + authenticatorDataAlignedLength + clientDataFieldsAlignedLength + isValid = signature.length < 193 + authenticatorDataAlignedLength + clientDataFieldsAlignedLength; } } @@ -256,7 +293,11 @@ library WebAuthn { uint256 y, P256.Verifiers verifiers ) internal view returns (bool success) { - success = verifySignature(challenge, castSignature(signature), authenticatorFlags, x, y, verifiers); + Signature calldata signatureStruct; + (success, signatureStruct) = castSignature(signature); + if (success) { + success = verifySignature(challenge, signatureStruct, authenticatorFlags, x, y, verifiers); + } } /** diff --git a/modules/passkey/contracts/test/TestWebAuthnLib.sol b/modules/passkey/contracts/test/TestWebAuthnLib.sol index f6e34280f..8f0ca573c 100644 --- a/modules/passkey/contracts/test/TestWebAuthnLib.sol +++ b/modules/passkey/contracts/test/TestWebAuthnLib.sol @@ -4,6 +4,12 @@ pragma solidity ^0.8.0; import {P256, WebAuthn} from "../libraries/WebAuthn.sol"; contract TestWebAuthnLib { + function castSignature(bytes calldata signature) external pure returns (WebAuthn.Signature calldata data) { + bool success; + (success, data) = WebAuthn.castSignature(signature); + require(success, "invalid signature encoding"); + } + function encodeClientDataJson( bytes32 challenge, string calldata clientDataFields diff --git a/modules/passkey/test/libraries/WebAuthn.spec.ts b/modules/passkey/test/libraries/WebAuthn.spec.ts index 22d1198f8..ec1725d74 100644 --- a/modules/passkey/test/libraries/WebAuthn.spec.ts +++ b/modules/passkey/test/libraries/WebAuthn.spec.ts @@ -1,6 +1,6 @@ import { expect } from 'chai' import { deployments, ethers } from 'hardhat' -import { DUMMY_CLIENT_DATA_FIELDS, base64UrlEncode, getSignatureBytes } from '../../src/utils/webauthn' +import { DUMMY_AUTHENTICATOR_DATA, DUMMY_CLIENT_DATA_FIELDS, base64UrlEncode, getSignatureBytes } from '../../src/utils/webauthn' const base64 = { encodeFromHex: (h: string) => { @@ -19,6 +19,105 @@ describe('WebAuthn Library', () => { return { webAuthnLib, mockP256Verifier } }) + describe('castSignature', function () { + it('Should correctly cast an ABI encoded WebAuthn signature', async () => { + const { webAuthnLib } = await setupTests() + + const signature = { + authenticatorData: DUMMY_AUTHENTICATOR_DATA, + clientDataFields: DUMMY_CLIENT_DATA_FIELDS, + r: 42n, + s: 1337n, + } + + expect( + await webAuthnLib.castSignature( + ethers.AbiCoder.defaultAbiCoder().encode( + ['bytes', 'string', 'uint256', 'uint256'], + [signature.authenticatorData, signature.clientDataFields, signature.r, signature.s], + ), + ), + ).to.deep.equal([ethers.hexlify(signature.authenticatorData), signature.clientDataFields, signature.r, signature.s]) + }) + + it('Should correctly cast a packed encoded WebAuthn signature', async () => { + const { webAuthnLib } = await setupTests() + + const signature = { + authenticatorData: DUMMY_AUTHENTICATOR_DATA, + clientDataFields: DUMMY_CLIENT_DATA_FIELDS, + r: 42n, + s: 1337n, + } + + expect( + await webAuthnLib.castSignature( + ethers.solidityPacked( + ['uint256', 'uint256', 'uint256', 'uint256', 'uint256', 'bytes', 'uint256', 'string'], + [ + 128, // offset of authenticator data + 160 + signature.authenticatorData.length, // offset of client data fields + signature.r, + signature.s, + signature.authenticatorData.length, + signature.authenticatorData, + signature.clientDataFields.length, + signature.clientDataFields, + ], + ), + ), + ).to.deep.equal([ethers.hexlify(signature.authenticatorData), signature.clientDataFields, signature.r, signature.s]) + }) + + it('Should correctly cast a partially packed encoded WebAuthn signature', async () => { + const { webAuthnLib } = await setupTests() + + const signature = { + authenticatorData: DUMMY_AUTHENTICATOR_DATA, + clientDataFields: DUMMY_CLIENT_DATA_FIELDS, + r: 42n, + s: 1337n, + } + + expect( + await webAuthnLib.castSignature( + ethers.solidityPacked( + ['uint256', 'uint256', 'uint256', 'uint256', 'bytes1', 'uint256', 'bytes', 'bytes2', 'uint256', 'string'], + [ + 128 + 1, // offset of authenticator data + 160 + signature.authenticatorData.length + 3, // offset of client data fields + signature.r, + signature.s, + '0x00', // padding + signature.authenticatorData.length, + signature.authenticatorData, + '0x0000', // padding + signature.clientDataFields.length, + signature.clientDataFields, + ], + ), + ), + ).to.deep.equal([ethers.hexlify(signature.authenticatorData), signature.clientDataFields, signature.r, signature.s]) + }) + + it('Should detect encoded WebAuthn signatures with too much padding', async () => { + const { webAuthnLib } = await setupTests() + + const signature = { + authenticatorData: DUMMY_AUTHENTICATOR_DATA, + clientDataFields: DUMMY_CLIENT_DATA_FIELDS, + r: 42n, + s: 1337n, + } + const signatureBytes = ethers.AbiCoder.defaultAbiCoder().encode( + ['bytes', 'string', 'uint256', 'uint256'], + [signature.authenticatorData, signature.clientDataFields, signature.r, signature.s], + ) + + await expect(webAuthnLib.castSignature(ethers.concat([signatureBytes, '0x00']))).to.be.revertedWith('invalid signature encoding') + }) + }) + describe('encodeClientDataJson', function () { it('Should correctly base-64 encode the 32 byte challenge', async () => { const { webAuthnLib } = await setupTests()