Verify WebAuthn Signature Encoding (#388)

This PR adds logic to verify the WebAuthn signature encoding length.
This is done so that encodings have a strict upper bound (determined by
the 'standard' ABI encoding of the `WebAuthn.Signature` struct) on the
length of the signatures.

This prevents a potential griefing attack where the signature that is
sent to an ERC-4337 bundler could be arbitrarily padded with additional
`0`s (which would have trivial increases to the `calldatasize` with
onchain LZMA calldata decompression that already exists) while causing
accounts to pay significantly more in gas costs for signature
verification (bounded by the `verificationGasLimit`).

---------

Co-authored-by: Mikhail <16622558+mmv08@users.noreply.github.com>

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"
   }
 }

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"
   }
 }

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);
+        }
     }
 
     /**

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

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()