diff --git a/src/CertManager.sol b/src/CertManager.sol index d1d9515..2db4738 100644 --- a/src/CertManager.sol +++ b/src/CertManager.sol @@ -17,6 +17,10 @@ contract CertManager is ICertManager { using LibAsn1Ptr for Asn1Ptr; using LibBytes for bytes; + error InvalidExtension(); + error InvalidBasicConstraints(); + error UnsupportedCriticalExtension(); + event CertVerified(bytes32 indexed certHash); event CertRevoked(bytes32 indexed certHash); event CertUnrevoked(bytes32 indexed certHash); @@ -433,7 +437,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 +448,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,9 +475,7 @@ 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) @@ -481,7 +483,7 @@ contract CertManager is ICertManager { pure returns (int64 maxPathLen) { - require(certificate[valuePtr.header()] == 0x30, "invalid basicConstraints"); + if (certificate[valuePtr.header()] != 0x30) revert InvalidBasicConstraints(); maxPathLen = -1; bool isCA; @@ -493,11 +495,11 @@ 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; } @@ -505,25 +507,25 @@ contract CertManager is ICertManager { 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 { diff --git a/test/CertManager.t.sol b/test/CertManager.t.sol index cc40cfd..39c517b 100644 --- a/test/CertManager.t.sol +++ b/test/CertManager.t.sol @@ -32,6 +32,16 @@ contract CertManagerHarness is CertManager { } } +contract CertManagerExtensionsHarness 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 { using Asn1Decode for bytes; using LibAsn1Ptr for Asn1Ptr; @@ -39,10 +49,12 @@ contract CertManagerTest is Test { Asn1DecodeHarness public harness; CertManagerHarness public certManagerHarness; + CertManagerExtensionsHarness public certManagerExtensionsHarness; function setUp() public { harness = new Asn1DecodeHarness(); certManagerHarness = new CertManagerHarness(); + certManagerExtensionsHarness = new CertManagerExtensionsHarness(); } // 's' INTEGER from cabundle[3] (2026-04-02 attestation): DER-encoded with a 0x00 @@ -63,7 +75,7 @@ contract CertManagerTest is Test { } function test_BasicConstraintsEmptySequenceRejectsCACert() public { - vm.expectRevert("isCA must be true for CA certs"); + vm.expectRevert(CertManager.InvalidBasicConstraints.selector); certManagerHarness.verifyBasicConstraints(hex"3000", true); } @@ -76,25 +88,50 @@ contract CertManagerTest is Test { } function test_BasicConstraintsRejectsEmptyPathLen() public { - vm.expectRevert("invalid pathLenConstraint"); + vm.expectRevert(CertManager.InvalidBasicConstraints.selector); certManagerHarness.verifyBasicConstraints(hex"30050101ff0200", true); } function test_BasicConstraintsRejectsOutOfBoundsChild() public { - vm.expectRevert("basicConstraints out of bounds"); + vm.expectRevert(CertManager.InvalidBasicConstraints.selector); certManagerHarness.verifyBasicConstraints(hex"3003020200", false); } function test_BasicConstraintsRejectsTrailingFields() public { - vm.expectRevert("trailing basicConstraints fields"); + vm.expectRevert(CertManager.InvalidBasicConstraints.selector); certManagerHarness.verifyBasicConstraints(hex"30090101ff020100020100", true); } function test_BasicConstraintsRejectsUnknownField() public { - vm.expectRevert("invalid basicConstraints field"); + vm.expectRevert(CertManager.InvalidBasicConstraints.selector); certManagerHarness.verifyBasicConstraints(hex"30020400", false); } + function test_VerifyExtensionsAllowsUnknownNonCriticalExtension() public view { + bytes memory unknownNameConstraints = hex"30090603551d1e04023000"; + + assertEq( + int256(certManagerExtensionsHarness.verifyExtensions(_clientExtensionsWith(unknownNameConstraints), false)), + -1 + ); + } + + function test_VerifyExtensionsAllowsUnknownCriticalFalseExtension() public view { + bytes memory unknownNameConstraints = hex"300c0603551d1e01010004023000"; + + assertEq( + int256(certManagerExtensionsHarness.verifyExtensions(_clientExtensionsWith(unknownNameConstraints), false)), + -1 + ); + } + + function test_VerifyExtensionsRejectsUnknownCriticalExtension() public { + bytes memory unknownNameConstraints = hex"300c0603551d1e0101ff04023000"; + + vm.expectRevert(CertManager.UnsupportedCriticalExtension.selector); + certManagerExtensionsHarness.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. @@ -345,6 +382,16 @@ 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 + ); + } } /// @dev Exposes the internal revocation-chain walk and lets tests seed the `verifiedParent`