fix: bound BasicConstraints ASN.1 parsing (#41)

* fix: bound basic constraints parsing

* fix: reject empty basic constraints path length

Author: leanthebean

src/CertManager.sol

@@ -477,18 +477,49 @@ contract CertManager is ICertManager {
         pure
         returns (int64 maxPathLen)
     {
+        require(certificate[valuePtr.header()] == 0x30, "invalid basicConstraints");
+
         maxPathLen = -1;
-        Asn1Ptr basicConstraintsPtr = certificate.firstChildOf(valuePtr);
         bool isCA;
-        if (certificate[basicConstraintsPtr.header()] == 0x01) {
-            require(basicConstraintsPtr.length() == 1, "invalid isCA bool value");
-            isCA = certificate[basicConstraintsPtr.content()] == 0xff;
-            basicConstraintsPtr = certificate.nextSiblingOf(basicConstraintsPtr);
+        uint256 end = valuePtr.content() + valuePtr.length();
+        uint256 cursor = valuePtr.content();
+
+        if (cursor < end) {
+            Asn1Ptr basicConstraintsPtr = certificate.firstChildOf(valuePtr);
+            cursor = _requireAsn1ChildWithin(basicConstraintsPtr, end);
+
+            if (certificate[basicConstraintsPtr.header()] == 0x01) {
+                require(basicConstraintsPtr.length() == 1, "invalid isCA bool value");
+                isCA = certificate[basicConstraintsPtr.content()] == 0xff;
+
+                if (cursor == end) {
+                    require(ca == isCA, "isCA must be true for CA certs");
+                    return maxPathLen;
+                }
+
+                basicConstraintsPtr = certificate.nextSiblingOf(basicConstraintsPtr);
+                cursor = _requireAsn1ChildWithin(basicConstraintsPtr, end);
+            }
+
+            require(ca == isCA, "isCA must be true for CA certs");
+
+            if (certificate[basicConstraintsPtr.header()] == 0x02) {
+                require(basicConstraintsPtr.length() > 0, "invalid pathLenConstraint");
+                maxPathLen = int64(uint64(certificate.uintAt(basicConstraintsPtr)));
+            } else {
+                revert("invalid basicConstraints field");
+            }
+
+            require(cursor == end, "trailing basicConstraints fields");
+            return maxPathLen;
         }
+
         require(ca == isCA, "isCA must be true for CA certs");
-        if (certificate[basicConstraintsPtr.header()] == 0x02) {
-            maxPathLen = int64(uint64(certificate.uintAt(basicConstraintsPtr)));
-        }
+    }
+
+    function _requireAsn1ChildWithin(Asn1Ptr ptr, uint256 parentEnd) internal pure returns (uint256 childEnd) {
+        childEnd = ptr.header() + ptr.totalLength();
+        require(childEnd <= parentEnd, "basicConstraints out of bounds");
     }
 
     function _verifyKeyUsageExtension(bytes memory certificate, Asn1Ptr valuePtr, bool ca) internal pure {

test/CertManager.t.sol

@@ -22,15 +22,27 @@ contract Asn1DecodeHarness {
     }
 }
 
+contract CertManagerHarness is CertManager {
+    using Asn1Decode for bytes;
+
+    constructor() CertManager(new P384Verifier()) {}
+
+    function verifyBasicConstraints(bytes memory der, bool ca) external pure returns (int64) {
+        return _verifyBasicConstraintsExtension(der, der.root(), ca);
+    }
+}
+
 contract CertManagerTest is Test {
     using Asn1Decode for bytes;
     using LibAsn1Ptr for Asn1Ptr;
     using LibBytes for bytes;
 
     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
@@ -46,6 +58,43 @@ contract CertManagerTest is Test {
         assertEq(lo, 0xa2eda9c549dc01460f5fe650814ebe0e7ee855d3bcffde95afd2e82e21df0eac);
     }
 
+    function test_BasicConstraintsEmptySequenceIsClientCert() public view {
+        assertEq(int256(certManagerHarness.verifyBasicConstraints(hex"3000", false)), -1);
+    }
+
+    function test_BasicConstraintsEmptySequenceRejectsCACert() public {
+        vm.expectRevert("isCA must be true for CA certs");
+        certManagerHarness.verifyBasicConstraints(hex"3000", true);
+    }
+
+    function test_BasicConstraintsAcceptsCAWithoutPathLen() public view {
+        assertEq(int256(certManagerHarness.verifyBasicConstraints(hex"30030101ff", true)), -1);
+    }
+
+    function test_BasicConstraintsAcceptsCAWithPathLen() public view {
+        assertEq(int256(certManagerHarness.verifyBasicConstraints(hex"30060101ff020100", true)), 0);
+    }
+
+    function test_BasicConstraintsRejectsEmptyPathLen() public {
+        vm.expectRevert("invalid pathLenConstraint");
+        certManagerHarness.verifyBasicConstraints(hex"30050101ff0200", true);
+    }
+
+    function test_BasicConstraintsRejectsOutOfBoundsChild() public {
+        vm.expectRevert("basicConstraints out of bounds");
+        certManagerHarness.verifyBasicConstraints(hex"3003020200", false);
+    }
+
+    function test_BasicConstraintsRejectsTrailingFields() public {
+        vm.expectRevert("trailing basicConstraints fields");
+        certManagerHarness.verifyBasicConstraints(hex"30090101ff020100020100", true);
+    }
+
+    function test_BasicConstraintsRejectsUnknownField() public {
+        vm.expectRevert("invalid basicConstraints field");
+        certManagerHarness.verifyBasicConstraints(hex"30020400", 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.