From 6b04c58a7679e824d78003ff07a615bfce08fc02 Mon Sep 17 00:00:00 2001
From: Elena Nadolinski <elena.nadolinski@coinbase.com>
Date: Mon, 22 Jun 2026 00:26:45 -0700
Subject: [PATCH] fix: reject unknown critical cert extensions

---
 src/CertManager.sol    | 18 ++++++++++--------
 test/CertManager.t.sol | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/src/CertManager.sol b/src/CertManager.sol
index cd9145b..f844a8e 100644
--- a/src/CertManager.sol
+++ b/src/CertManager.sol
@@ -416,16 +416,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 critical;
 
-            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) {
+                require(valuePtr.length() == 1, "invalid critical bool value");
+                critical = certificate[valuePtr.content()] != 0x00;
+                valuePtr = certificate.nextSiblingOf(valuePtr);
+            }
 
+            if (oid == BASIC_CONSTRAINTS_OID || oid == KEY_USAGE_OID) {
                 valuePtr = certificate.octetString(valuePtr);
 
                 if (oid == BASIC_CONSTRAINTS_OID) {
@@ -435,6 +435,8 @@ contract CertManager is ICertManager {
                     keyUsageFound = true;
                     _verifyKeyUsageExtension(certificate, valuePtr, ca);
                 }
+            } else {
+                require(!critical, "unsupported critical extension");
             }
 
             if (extensionPtr.content() + extensionPtr.length() == end) {
diff --git a/test/CertManager.t.sol b/test/CertManager.t.sol
index 687e222..740e323 100644
--- a/test/CertManager.t.sol
+++ b/test/CertManager.t.sol
@@ -21,11 +21,23 @@ contract Asn1DecodeHarness {
     }
 }
 
+contract CertManagerHarness is CertManager {
+    using Asn1Decode for bytes;
+
+    constructor() CertManager(new P384Verifier()) {}
+
+    function verifyExtensions(bytes memory der, bool ca) external pure returns (int64) {
+        return _verifyExtensions(der, der.root(), ca);
+    }
+}
+
 contract CertManagerTest is Test {
     Asn1DecodeHarness public harness;
+    CertManagerHarness public certManagerHarness;
 
     function setUp() public {
         harness = new Asn1DecodeHarness();
+        certManagerHarness = new CertManagerHarness();
     }
 
     // 's' INTEGER from cabundle[3] (2026-04-02 attestation): DER-encoded with a 0x00
@@ -41,6 +53,25 @@ contract CertManagerTest is Test {
         assertEq(lo, 0xa2eda9c549dc01460f5fe650814ebe0e7ee855d3bcffde95afd2e82e21df0eac);
     }
 
+    function test_VerifyExtensionsAllowsUnknownNonCriticalExtension() public view {
+        bytes memory unknownNameConstraints = hex"30090603551d1e04023000";
+
+        assertEq(int256(certManagerHarness.verifyExtensions(_clientExtensionsWith(unknownNameConstraints), false)), -1);
+    }
+
+    function test_VerifyExtensionsAllowsUnknownCriticalFalseExtension() public view {
+        bytes memory unknownNameConstraints = hex"300c0603551d1e01010004023000";
+
+        assertEq(int256(certManagerHarness.verifyExtensions(_clientExtensionsWith(unknownNameConstraints), false)), -1);
+    }
+
+    function test_VerifyExtensionsRejectsUnknownCriticalExtension() public {
+        bytes memory unknownNameConstraints = hex"300c0603551d1e0101ff04023000";
+
+        vm.expectRevert("unsupported critical extension");
+        certManagerHarness.verifyExtensions(_clientExtensionsWith(unknownNameConstraints), false);
+    }
+
     // Cert chain from the 2026-04-02 ~15:35 UTC dev attestation that produced the live revert.
     // CB0 is the AWS Nitro root (keccak256(CB0) == CertManager.ROOT_CA_CERT_HASH, pinned in the
     // constructor), so the chain is verified starting from CB1.
@@ -155,4 +186,14 @@ contract CertManagerTest is Test {
 
         return der;
     }
+
+    function _clientExtensionsWith(bytes memory extraExtension) internal pure returns (bytes memory) {
+        bytes memory body =
+            abi.encodePacked(hex"300c0603551d130101ff04023000", hex"300e0603551d0f0101ff040403020780", extraExtension);
+
+        return
+            abi.encodePacked(
+                bytes1(0xa3), bytes1(uint8(body.length + 2)), bytes1(0x30), bytes1(uint8(body.length)), body
+            );
+    }
 }