Decode WebAuthn object from calldata buffers, with out-of-bound detection (#191)

Co-authored-by: Ernesto García <[email protected]>

Author: Amxx

contracts/utils/cryptography/WebAuthn.sol

@@ -60,11 +60,6 @@ library WebAuthn {
     /// @dev Bit 4 of the authenticator data flags: "Backup State" bit.
     bytes1 private constant AUTH_DATA_FLAGS_BS = 0x10;
 
-    /// @dev The expected type string in the client data JSON when verifying assertion signatures.
-    /// https://www.w3.org/TR/webauthn-2/#dom-collectedclientdata-type
-    // solhint-disable-next-line quotes
-    bytes32 private constant EXPECTED_TYPE_HASH = keccak256('"type":"webauthn.get"');
-
     /**
      * @dev Performs the absolute minimal verification of a WebAuthn Authentication Assertion.
      * This function includes only the essential checks required for basic WebAuthn security:
@@ -89,18 +84,16 @@ library WebAuthn {
         // - 32 bytes for rpIdHash
         // - 1 byte for flags
         // - 4 bytes for signature counter
-        if (auth.authenticatorData.length < 37) return false;
-        bytes memory clientDataJSON = bytes(auth.clientDataJSON);
-
         return
-            validateExpectedTypeHash(clientDataJSON, auth.typeIndex) && // 11
-            validateChallenge(clientDataJSON, auth.challengeIndex, challenge) && // 12
+            auth.authenticatorData.length > 36 &&
+            validateExpectedTypeHash(auth.clientDataJSON, auth.typeIndex) && // 11
+            validateChallenge(auth.clientDataJSON, auth.challengeIndex, challenge) && // 12
             // Handles signature malleability internally
             P256.verify(
                 sha256(
                     abi.encodePacked(
                         auth.authenticatorData,
-                        sha256(clientDataJSON) // 19
+                        sha256(bytes(auth.clientDataJSON)) // 19
                     )
                 ),
                 auth.r,
@@ -222,29 +215,64 @@ library WebAuthn {
      * @dev Validates that the https://www.w3.org/TR/webauthn-2/#type[Type] field in the client data JSON
      * is set to "webauthn.get".
      */
-    function validateExpectedTypeHash(bytes memory clientDataJSON, uint256 typeIndex) internal pure returns (bool) {
+    function validateExpectedTypeHash(string memory clientDataJSON, uint256 typeIndex) internal pure returns (bool) {
         // 21 = length of '"type":"webauthn.get"'
-        bytes memory typeValueBytes = Bytes.slice(clientDataJSON, typeIndex, typeIndex + 21);
-        return keccak256(typeValueBytes) == EXPECTED_TYPE_HASH;
+        bytes memory typeValueBytes = Bytes.slice(bytes(clientDataJSON), typeIndex, typeIndex + 21);
+
+        // solhint-disable-next-line quotes
+        return bytes21(typeValueBytes) == bytes21('"type":"webauthn.get"');
     }
 
     /// @dev Validates that the challenge in the client data JSON matches the `expectedChallenge`.
     function validateChallenge(
-        bytes memory clientDataJSON,
+        string memory clientDataJSON,
         uint256 challengeIndex,
-        bytes memory expectedChallenge
+        bytes memory challenge
     ) internal pure returns (bool) {
-        bytes memory expectedChallengeBytes = bytes(
-            // solhint-disable-next-line quotes
-            string.concat('"challenge":"', Base64.encodeURL(expectedChallenge), '"')
-        );
-        if (challengeIndex + expectedChallengeBytes.length > clientDataJSON.length) return false;
-        bytes memory actualChallengeBytes = Bytes.slice(
-            clientDataJSON,
-            challengeIndex,
-            challengeIndex + expectedChallengeBytes.length
+        // solhint-disable-next-line quotes
+        string memory expectedChallenge = string.concat('"challenge":"', Base64.encodeURL(challenge), '"');
+        string memory actualChallenge = string(
+            Bytes.slice(bytes(clientDataJSON), challengeIndex, challengeIndex + bytes(expectedChallenge).length)
         );
 
-        return Strings.equal(string(actualChallengeBytes), string(expectedChallengeBytes));
+        return Strings.equal(actualChallenge, expectedChallenge);
+    }
+
+    /**
+     * @dev Verifies that calldata bytes (`input`) represents a valid `WebAuthnAuth` object. If encoding is valid,
+     * returns true and the calldata view at the object. Otherwise, returns false and an invalid calldata object.
+     *
+     * NOTE: The returned `auth` object should not be accessed if `success` is false. Trying to access the data may
+     * cause revert/panic.
+     */
+    function tryDecodeAuth(bytes calldata input) internal pure returns (bool success, WebAuthnAuth calldata auth) {
+        assembly ("memory-safe") {
+            auth := input.offset
+        }
+
+        // Minimum length to hold 6 objects (32 bytes each)
+        if (input.length < 0xC0) return (false, auth);
+
+        // Get offset of non-value-type elements relative to the input buffer
+        uint256 authenticatorDataOffset = uint256(bytes32(input[0x80:]));
+        uint256 clientDataJSONOffset = uint256(bytes32(input[0xa0:]));
+
+        // The elements length (at the offset) should be 32 bytes long. We check that this is within the
+        // buffer bounds. Since we know input.length is at least 32, we can subtract with no overflow risk.
+        if (input.length - 0x20 < authenticatorDataOffset || input.length - 0x20 < clientDataJSONOffset)
+            return (false, auth);
+
+        // Get the lengths. offset + 32 is bounded by input.length so it does not overflow.
+        uint256 authenticatorDataLength = uint256(bytes32(input[authenticatorDataOffset:]));
+        uint256 clientDataJSONLength = uint256(bytes32(input[clientDataJSONOffset:]));
+
+        // Check that the input buffer is long enough to store the non-value-type elements
+        // Since we know input.length is at least xxxOffset + 32, we can subtract with no overflow risk.
+        if (
+            input.length - authenticatorDataOffset - 0x20 < authenticatorDataLength ||
+            input.length - clientDataJSONOffset - 0x20 < clientDataJSONLength
+        ) return (false, auth);
+
+        return (true, auth);
     }
 }

contracts/utils/cryptography/signers/SignerWebAuthn.sol

@@ -40,21 +40,11 @@ abstract contract SignerWebAuthn is SignerP256 {
         bytes calldata signature
     ) internal view virtual override returns (bool) {
         (bytes32 qx, bytes32 qy) = signer();
+        (bool decodeSuccess, WebAuthn.WebAuthnAuth calldata auth) = WebAuthn.tryDecodeAuth(signature);
 
         return
-            WebAuthn.verifyMinimal(abi.encodePacked(hash), _toWebAuthnSignature(signature), qx, qy) ||
-            super._rawSignatureValidation(hash, signature);
-    }
-
-    /// @dev Non-reverting version of signature decoding.
-    function _toWebAuthnSignature(bytes calldata signature) private pure returns (WebAuthn.WebAuthnAuth memory auth) {
-        bool decodable;
-        assembly ("memory-safe") {
-            let offset := calldataload(signature.offset)
-            // Validate the offset is within bounds and makes sense for a WebAuthnAuth struct
-            // A valid offset should be 32 and point to data within the signature bounds
-            decodable := and(eq(offset, 32), lt(add(offset, 0x80), signature.length))
-        }
-        return decodable ? abi.decode(signature, (WebAuthn.WebAuthnAuth)) : auth;
+            decodeSuccess
+                ? WebAuthn.verifyMinimal(abi.encodePacked(hash), auth, qx, qy)
+                : super._rawSignatureValidation(hash, signature);
     }
 }

test/helpers/signers.js

@@ -92,16 +92,14 @@ class WebAuthnSigningKey extends P256SigningKey {
     const { r, s } = super.sign(sha256(concat([authenticatorData, sha256(toUtf8Bytes(clientDataJSON))])));
 
     const serialized = AbiCoder.defaultAbiCoder().encode(
-      ['tuple(bytes32,bytes32,uint256,uint256,bytes,string)'],
+      ['bytes32', 'bytes32', 'uint256', 'uint256', 'bytes', 'string'],
       [
-        [
-          r,
-          s,
-          clientDataJSON.indexOf('"challenge"'),
-          clientDataJSON.indexOf('"type"'),
-          authenticatorData,
-          clientDataJSON,
-        ],
+        r,
+        s,
+        clientDataJSON.indexOf('"challenge"'),
+        clientDataJSON.indexOf('"type"'),
+        authenticatorData,
+        clientDataJSON,
       ],
     );