Merge remote-tracking branch 'origin/main' into docs/cert-parser-strictness

Author: leopoldjoy

src/Asn1Decode.sol

@@ -58,6 +58,11 @@ library LibAsn1Ptr {
 }
 
 library Asn1Decode {
+    error InvalidAsn1Length();
+    error InvalidAsn1Type();
+    error InvalidAsn1Value();
+    error UnsupportedAsn1Tag();
+
     using LibAsn1Ptr for Asn1Ptr;
     using LibBytes for bytes;
 
@@ -97,7 +102,7 @@ library Asn1Decode {
      * @return A pointer to the first child node
      */
     function firstChildOf(bytes memory der, Asn1Ptr ptr) internal pure returns (Asn1Ptr) {
-        require(der[ptr.header()] & 0x20 == 0x20, "Not a constructed type");
+        if (der[ptr.header()] & 0x20 != 0x20) revert InvalidAsn1Type();
         return readNodeLength(der, ptr.content());
     }
 
@@ -108,9 +113,9 @@ library Asn1Decode {
      * @return A pointer to a bitstring
      */
     function bitstring(bytes memory der, Asn1Ptr ptr) internal pure returns (Asn1Ptr) {
-        require(der[ptr.header()] == 0x03, "Not type BIT STRING");
+        if (der[ptr.header()] != 0x03) revert InvalidAsn1Type();
         // Only 00 padded bitstr can be converted to bytestr!
-        require(der[ptr.content()] == 0x00, "Non-0-padded BIT STRING");
+        if (der[ptr.content()] != 0x00) revert InvalidAsn1Value();
         return LibAsn1Ptr.toAsn1Ptr(ptr.header(), ptr.content() + 1, ptr.length() - 1);
     }
 
@@ -122,22 +127,22 @@ library Asn1Decode {
      *         significant byte, so X.509 bit masks are stable across multi-octet encodings.
      */
     function bitstringUintAt(bytes memory der, Asn1Ptr ptr) internal pure returns (uint256) {
-        require(der[ptr.header()] == 0x03, "Not type BIT STRING");
-        require(ptr.length() > 0, "invalid BIT STRING length");
+        if (der[ptr.header()] != 0x03) revert InvalidAsn1Type();
+        if (ptr.length() == 0) revert InvalidAsn1Length();
 
         uint256 unusedBits = uint8(der[ptr.content()]);
-        require(unusedBits <= 7, "invalid BIT STRING padding");
+        if (unusedBits > 7) revert InvalidAsn1Value();
 
         uint256 len = ptr.length() - 1;
-        require(len <= 32, "BIT STRING too long");
+        if (len > 32) revert InvalidAsn1Length();
         if (len == 0) {
-            require(unusedBits == 0, "invalid BIT STRING padding");
+            if (unusedBits != 0) revert InvalidAsn1Value();
             return 0;
         }
 
         if (unusedBits != 0) {
             uint8 unusedMask = uint8((uint256(1) << unusedBits) - 1);
-            require(uint8(der[ptr.content() + len]) & unusedMask == 0, "Non-zero unused BIT STRING bits");
+            if (uint8(der[ptr.content() + len]) & unusedMask != 0) revert InvalidAsn1Value();
         }
 
         uint256 value;
@@ -154,7 +159,7 @@ library Asn1Decode {
      * @return A pointer to an octet string
      */
     function octetString(bytes memory der, Asn1Ptr ptr) internal pure returns (Asn1Ptr) {
-        require(der[ptr.header()] == 0x04, "Not type OCTET STRING");
+        if (der[ptr.header()] != 0x04) revert InvalidAsn1Type();
         return readNodeLength(der, ptr.content());
     }
 
@@ -165,8 +170,8 @@ library Asn1Decode {
      * @return Uint value of node
      */
     function uintAt(bytes memory der, Asn1Ptr ptr) internal pure returns (uint256) {
-        require(der[ptr.header()] == 0x02, "Not type INTEGER");
-        require(der[ptr.content()] & 0x80 == 0, "Not positive");
+        if (der[ptr.header()] != 0x02) revert InvalidAsn1Type();
+        if (der[ptr.content()] & 0x80 != 0) revert InvalidAsn1Value();
         uint256 len = ptr.length();
         return uint256(readBytesN(der, ptr.content(), len) >> (32 - len) * 8);
     }
@@ -178,8 +183,8 @@ library Asn1Decode {
      * @return 384-bit uint encoded in uint128 and uint256
      */
     function uint384At(bytes memory der, Asn1Ptr ptr) internal pure returns (uint128, uint256) {
-        require(der[ptr.header()] == 0x02, "Not type INTEGER");
-        require(der[ptr.content()] & 0x80 == 0, "Not positive");
+        if (der[ptr.header()] != 0x02) revert InvalidAsn1Type();
+        if (der[ptr.content()] & 0x80 != 0) revert InvalidAsn1Value();
         uint256 valueLength = ptr.length();
         uint256 start = ptr.content();
         if (der[start] == 0) {
@@ -205,12 +210,12 @@ library Asn1Decode {
         uint256 length = ptr.length();
 
         // content validation:
-        require((_type == 0x17 && length == 13) || (_type == 0x18 && length == 15), "Invalid TIMESTAMP");
-        require(der[offset + length - 1] == 0x5A, "TIMESTAMP must be UTC"); // 0x5A == 'Z'
+        if ((_type != 0x17 || length != 13) && (_type != 0x18 || length != 15)) revert InvalidAsn1Value();
+        if (der[offset + length - 1] != 0x5A) revert InvalidAsn1Value(); // 0x5A == 'Z'
         for (uint256 i = 0; i < length - 1; i++) {
             // all other characters must be digits between 0 and 9
             uint8 v = uint8(der[offset + i]);
-            require(48 <= v && v <= 57, "Invalid character in TIMESTAMP");
+            if (v < 48 || v > 57) revert InvalidAsn1Value();
         }
 
         uint16 _years;
@@ -231,14 +236,19 @@ library Asn1Decode {
     }
 
     function readNodeLength(bytes memory der, uint256 ix) private pure returns (Asn1Ptr) {
-        require(der[ix] & 0x1f != 0x1f, "ASN.1 tags longer than 1-byte are not supported");
+        if (ix + 1 >= der.length) revert InvalidAsn1Length();
+        if (der[ix] & 0x1f == 0x1f) revert UnsupportedAsn1Tag();
         uint256 length;
         uint256 ixFirstContentByte;
         if ((der[ix + 1] & 0x80) == 0) {
             length = uint8(der[ix + 1]);
             ixFirstContentByte = ix + 2;
         } else {
             uint8 lengthbytesLength = uint8(der[ix + 1] & 0x7F);
+            if (lengthbytesLength == 0 || lengthbytesLength > 32 || ix + 2 + lengthbytesLength > der.length) {
+                revert InvalidAsn1Length();
+            }
+            if (der[ix + 2] == 0) revert InvalidAsn1Length();
             if (lengthbytesLength == 1) {
                 length = uint8(der[ix + 2]);
             } else if (lengthbytesLength == 2) {
@@ -247,8 +257,10 @@ library Asn1Decode {
                 length = uint256(readBytesN(der, ix + 2, lengthbytesLength) >> (32 - lengthbytesLength) * 8);
                 require(length <= 2 ** 64 - 1); // bound to max uint64 to be safe
             }
+            if (length < 128) revert InvalidAsn1Length();
             ixFirstContentByte = ix + 2 + lengthbytesLength;
         }
+        if (ixFirstContentByte + length > der.length) revert InvalidAsn1Length();
         return LibAsn1Ptr.toAsn1Ptr(ix, ixFirstContentByte, length);
     }
 

src/CertManager.sol

@@ -17,9 +17,20 @@ contract CertManager is ICertManager {
     using LibAsn1Ptr for Asn1Ptr;
     using LibBytes for bytes;
 
+    error InvalidExtension();
+    error InvalidBasicConstraints();
+    error InvalidSubjectPublicKey();
+    error UnsupportedCriticalExtension();
+    error NotOwner();
+    error NotRevoker();
+    error IncompleteCertChain();
+    error DeprecatedEntrypoint();
+    error InvalidOwner();
+    error InvalidRevoker();
+
     event CertVerified(bytes32 indexed certHash);
-    event CertRevoked(bytes32 indexed certHash);
-    event CertUnrevoked(bytes32 indexed certHash);
+    event CertRevoked(bytes32 indexed certHash, address indexed account);
+    event CertUnrevoked(bytes32 indexed certHash, address indexed account);
     event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
     event RevokerUpdated(address indexed previousRevoker, address indexed newRevoker);
 
@@ -78,11 +89,11 @@ contract CertManager is ICertManager {
     }
 
     function _onlyOwner() internal view {
-        require(msg.sender == owner, "not owner");
+        if (msg.sender != owner) revert NotOwner();
     }
 
     function _onlyRevoker() internal view {
-        require(msg.sender == revoker, "not revoker");
+        if (msg.sender != revoker) revert NotRevoker();
     }
 
     constructor(IP384Verifier p384Verifier_) {
@@ -107,12 +118,12 @@ contract CertManager is ICertManager {
     /// @notice DEPRECATED — always reverts. The fully on-chain (non-hinted) path is too expensive
     ///         post-Fusaka and has been removed. Use {verifyCACertWithHints}.
     function verifyCACert(bytes memory, bytes32) external pure returns (bytes32) {
-        revert("use hinted cert verification");
+        revert DeprecatedEntrypoint();
     }
 
     /// @notice DEPRECATED — always reverts. Use {verifyClientCertWithHints}.
     function verifyClientCert(bytes memory, bytes32) external pure returns (VerifiedCert memory) {
-        revert("use hinted cert verification");
+        revert DeprecatedEntrypoint();
     }
 
     /// @notice Verify a CA certificate against its (already-cached) parent and cache the result.
@@ -168,13 +179,13 @@ contract CertManager is ICertManager {
     }
 
     function transferOwnership(address newOwner) external onlyOwner {
-        require(newOwner != address(0), "invalid owner");
+        if (newOwner == address(0)) revert InvalidOwner();
         emit OwnershipTransferred(owner, newOwner);
         owner = newOwner;
     }
 
     function setRevoker(address newRevoker) external onlyOwner {
-        require(newRevoker != address(0), "invalid revoker");
+        if (newRevoker == address(0)) revert InvalidRevoker();
         emit RevokerUpdated(revoker, newRevoker);
         revoker = newRevoker;
     }
@@ -195,12 +206,12 @@ contract CertManager is ICertManager {
 
     function unrevokeCert(bytes32 certId) external onlyOwner {
         revoked[certId] = false;
-        emit CertUnrevoked(certId);
+        emit CertUnrevoked(certId, msg.sender);
     }
 
     function _revokeCert(bytes32 certId) internal {
         revoked[certId] = true;
-        emit CertRevoked(certId);
+        emit CertRevoked(certId, msg.sender);
     }
 
     function _requireCanRevoke(bytes32 certId) internal view {
@@ -235,7 +246,7 @@ contract CertManager is ICertManager {
         // Fail closed: a chain that terminates at bytes32(0) without reaching the pinned root is
         // broken and must not be treated as a verified, non-revoked chain. Reverting here instead
         // of returning silently means revocation safety never depends on upstream guards.
-        revert("incomplete cert chain");
+        revert IncompleteCertChain();
     }
 
     function _verifyCert(
@@ -400,17 +411,19 @@ contract CertManager is ICertManager {
         Asn1Ptr subjectPublicKeyPtr = certificate.nextSiblingOf(pubKeyAlgoPtr);
         Asn1Ptr subjectPubKeyPtr = certificate.bitstring(subjectPublicKeyPtr);
 
-        require(
-            certificate.keccak(pubKeyAlgoIdPtr.content(), pubKeyAlgoIdPtr.length()) == EC_PUB_KEY_OID,
-            "invalid cert algo id"
-        );
-        require(
-            certificate.keccak(algoParamsPtr.content(), algoParamsPtr.length()) == SECP_384_R1_OID,
-            "invalid cert algo param"
-        );
+        if (certificate.keccak(pubKeyAlgoIdPtr.content(), pubKeyAlgoIdPtr.length()) != EC_PUB_KEY_OID) {
+            revert InvalidSubjectPublicKey();
+        }
+        if (certificate.keccak(algoParamsPtr.content(), algoParamsPtr.length()) != SECP_384_R1_OID) {
+            revert InvalidSubjectPublicKey();
+        }
 
-        uint256 end = subjectPubKeyPtr.content() + subjectPubKeyPtr.length();
-        subjectPubKey = certificate.slice(end - 96, 96);
+        uint256 keyStart = subjectPubKeyPtr.content();
+        uint256 keyLength = subjectPubKeyPtr.length();
+        if (keyLength != 97 || keyStart + keyLength > certificate.length || certificate[keyStart] != 0x04) {
+            revert InvalidSubjectPublicKey();
+        }
+        subjectPubKey = certificate.slice(keyStart + 1, 96);
     }
 
     function _verifyValidity(bytes memory certificate, Asn1Ptr validityPtr) internal view returns (uint64 notAfter) {
@@ -433,7 +446,7 @@ contract CertManager is ICertManager {
         pure
         returns (int64 maxPathLen)
     {
-        require(certificate[extensionsPtr.header()] == 0xa3, "invalid extensions");
+        if (certificate[extensionsPtr.header()] != 0xa3) revert InvalidExtension();
         extensionsPtr = certificate.firstChildOf(extensionsPtr);
         Asn1Ptr extensionPtr = certificate.firstChildOf(extensionsPtr);
         uint256 end = extensionsPtr.content() + extensionsPtr.length();
@@ -444,16 +457,16 @@ contract CertManager is ICertManager {
         while (true) {
             Asn1Ptr oidPtr = certificate.firstChildOf(extensionPtr);
             bytes32 oid = certificate.keccak(oidPtr.content(), oidPtr.length());
+            Asn1Ptr valuePtr = certificate.nextSiblingOf(oidPtr);
+            bool recognized = oid == BASIC_CONSTRAINTS_OID || oid == KEY_USAGE_OID;
 
-            if (oid == BASIC_CONSTRAINTS_OID || oid == KEY_USAGE_OID) {
-                Asn1Ptr valuePtr = certificate.nextSiblingOf(oidPtr);
-
-                if (certificate[valuePtr.header()] == 0x01) {
-                    // skip optional critical bool
-                    require(valuePtr.length() == 1, "invalid critical bool value");
-                    valuePtr = certificate.nextSiblingOf(valuePtr);
-                }
+            if (certificate[valuePtr.header()] == 0x01) {
+                if (valuePtr.length() != 1) revert InvalidExtension();
+                if (!recognized && certificate[valuePtr.content()] != 0x00) revert UnsupportedCriticalExtension();
+                valuePtr = certificate.nextSiblingOf(valuePtr);
+            }
 
+            if (recognized) {
                 valuePtr = certificate.octetString(valuePtr);
 
                 if (oid == BASIC_CONSTRAINTS_OID) {
@@ -471,17 +484,15 @@ contract CertManager is ICertManager {
             extensionPtr = certificate.nextSiblingOf(extensionPtr);
         }
 
-        require(basicConstraintsFound, "basicConstraints not found");
-        require(keyUsageFound, "keyUsage not found");
-        require(ca || maxPathLen == -1, "maxPathLen must be undefined for client cert");
+        if (!basicConstraintsFound || !keyUsageFound || (!ca && maxPathLen != -1)) revert InvalidExtension();
     }
 
     function _verifyBasicConstraintsExtension(bytes memory certificate, Asn1Ptr valuePtr, bool ca)
         internal
         pure
         returns (int64 maxPathLen)
     {
-        require(certificate[valuePtr.header()] == 0x30, "invalid basicConstraints");
+        if (certificate[valuePtr.header()] != 0x30) revert InvalidBasicConstraints();
 
         maxPathLen = -1;
         bool isCA;
@@ -493,37 +504,37 @@ contract CertManager is ICertManager {
             cursor = _requireAsn1ChildWithin(basicConstraintsPtr, end);
 
             if (certificate[basicConstraintsPtr.header()] == 0x01) {
-                require(basicConstraintsPtr.length() == 1, "invalid isCA bool value");
+                if (basicConstraintsPtr.length() != 1) revert InvalidBasicConstraints();
                 isCA = certificate[basicConstraintsPtr.content()] == 0xff;
 
                 if (cursor == end) {
-                    require(ca == isCA, "isCA must be true for CA certs");
+                    if (ca != isCA) revert InvalidBasicConstraints();
                     return maxPathLen;
                 }
 
                 basicConstraintsPtr = certificate.nextSiblingOf(basicConstraintsPtr);
                 cursor = _requireAsn1ChildWithin(basicConstraintsPtr, end);
             }
 
-            require(ca == isCA, "isCA must be true for CA certs");
+            if (ca != isCA) revert InvalidBasicConstraints();
 
             if (certificate[basicConstraintsPtr.header()] == 0x02) {
-                require(basicConstraintsPtr.length() > 0, "invalid pathLenConstraint");
+                if (basicConstraintsPtr.length() == 0) revert InvalidBasicConstraints();
                 maxPathLen = int64(uint64(certificate.uintAt(basicConstraintsPtr)));
             } else {
-                revert("invalid basicConstraints field");
+                revert InvalidBasicConstraints();
             }
 
-            require(cursor == end, "trailing basicConstraints fields");
+            if (cursor != end) revert InvalidBasicConstraints();
             return maxPathLen;
         }
 
-        require(ca == isCA, "isCA must be true for CA certs");
+        if (ca != isCA) revert InvalidBasicConstraints();
     }
 
     function _requireAsn1ChildWithin(Asn1Ptr ptr, uint256 parentEnd) internal pure returns (uint256 childEnd) {
         childEnd = ptr.header() + ptr.totalLength();
-        require(childEnd <= parentEnd, "basicConstraints out of bounds");
+        if (childEnd > parentEnd) revert InvalidBasicConstraints();
     }
 
     function _verifyKeyUsageExtension(bytes memory certificate, Asn1Ptr valuePtr, bool ca) internal pure {

src/ICertManager.sol

@@ -45,9 +45,9 @@ interface ICertManager {
 
     // --- DEPRECATED: these always revert; use the *WithHints variants above. ---
 
-    /// @dev DEPRECATED — always reverts ("use hinted cert verification").
+    /// @dev DEPRECATED — always reverts.
     function verifyCACert(bytes memory cert, bytes32 parentCertHash) external returns (bytes32);
 
-    /// @dev DEPRECATED — always reverts ("use hinted cert verification").
+    /// @dev DEPRECATED — always reverts.
     function verifyClientCert(bytes memory cert, bytes32 parentCertHash) external returns (VerifiedCert memory);
 }