From b3630f137f4f76cc16b5cc31ed6497ff951ceaee Mon Sep 17 00:00:00 2001 From: Elena Nadolinski Date: Fri, 19 Jun 2026 11:27:38 -0700 Subject: [PATCH] fix: validate certificate public key encoding --- src/CertManager.sol | 43 ++++++++++++----------- test/CertManager.t.sol | 77 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 96 insertions(+), 24 deletions(-) diff --git a/src/CertManager.sol b/src/CertManager.sol index d1d9515..9bcfff6 100644 --- a/src/CertManager.sol +++ b/src/CertManager.sol @@ -17,6 +17,9 @@ contract CertManager is ICertManager { using LibAsn1Ptr for Asn1Ptr; using LibBytes for bytes; + error InvalidBasicConstraints(); + error InvalidSubjectPublicKey(); + event CertVerified(bytes32 indexed certHash); event CertRevoked(bytes32 indexed certHash); event CertUnrevoked(bytes32 indexed certHash); @@ -400,17 +403,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) { @@ -481,7 +486,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 +498,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 +510,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..fcac97a 100644 --- a/test/CertManager.t.sol +++ b/test/CertManager.t.sol @@ -32,6 +32,24 @@ contract CertManagerHarness is CertManager { } } +contract CertManagerPubKeyHarness is CertManager { + using Asn1Decode for bytes; + + constructor() CertManager(new P384Verifier()) {} + + function parsePubKey(bytes memory subjectPublicKeyInfo) external pure returns (bytes memory) { + return _parsePubKey(subjectPublicKeyInfo, subjectPublicKeyInfo.root()); + } + + function parsePubKeyAt(bytes memory certificate, uint256 header, uint256 content, uint256 length) + external + pure + returns (bytes memory) + { + return _parsePubKey(certificate, LibAsn1Ptr.toAsn1Ptr(header, content, length)); + } +} + contract CertManagerTest is Test { using Asn1Decode for bytes; using LibAsn1Ptr for Asn1Ptr; @@ -39,10 +57,12 @@ contract CertManagerTest is Test { Asn1DecodeHarness public harness; CertManagerHarness public certManagerHarness; + CertManagerPubKeyHarness public certManagerPubKeyHarness; function setUp() public { harness = new Asn1DecodeHarness(); certManagerHarness = new CertManagerHarness(); + certManagerPubKeyHarness = new CertManagerPubKeyHarness(); } // 's' INTEGER from cabundle[3] (2026-04-02 attestation): DER-encoded with a 0x00 @@ -63,7 +83,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 +96,65 @@ 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_ParsePubKeyAcceptsUncompressedP384Point() public view { + bytes memory pubKey = _patternBytes(96); + bytes memory spki = abi.encodePacked(hex"3076301006072a8648ce3d020106052b8104002203620004", pubKey); + + assertEq(certManagerPubKeyHarness.parsePubKey(spki), pubKey); + } + + function test_ParsePubKeyRejectsCompressedP384Point() public { + bytes memory compressedKey = _patternBytes(48); + bytes memory spki = abi.encodePacked(hex"3046301006072a8648ce3d020106052b8104002203320002", compressedKey); + bytes memory paddedCertificate = abi.encodePacked(new bytes(128), spki); + + vm.expectRevert(CertManager.InvalidSubjectPublicKey.selector); + certManagerPubKeyHarness.parsePubKeyAt(paddedCertificate, 128, 130, 0x46); + } + + function test_ParsePubKeyRejectsOversizedP384Point() public { + bytes memory oversizedKey = _patternBytes(97); + bytes memory spki = abi.encodePacked(hex"3077301006072a8648ce3d020106052b8104002203630004", oversizedKey); + + vm.expectRevert(CertManager.InvalidSubjectPublicKey.selector); + certManagerPubKeyHarness.parsePubKey(spki); + } + + function test_ParsePubKeyRejectsTruncatedP384Point() public { + bytes memory truncatedKey = _patternBytes(95); + bytes memory spki = abi.encodePacked(hex"3076301006072a8648ce3d020106052b8104002203620004", truncatedKey); + + vm.expectRevert(CertManager.InvalidSubjectPublicKey.selector); + certManagerPubKeyHarness.parsePubKey(spki); + } + + function test_ParsePubKeyRejectsMissingUncompressedPrefix() public { + bytes memory pubKey = _patternBytes(96); + bytes memory spki = abi.encodePacked(hex"3076301006072a8648ce3d020106052b8104002203620002", pubKey); + + vm.expectRevert(CertManager.InvalidSubjectPublicKey.selector); + certManagerPubKeyHarness.parsePubKey(spki); + } + // 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 +405,13 @@ contract CertManagerTest is Test { return der; } + + function _patternBytes(uint256 len) internal pure returns (bytes memory out) { + out = new bytes(len); + for (uint256 i = 0; i < len; i++) { + out[i] = bytes1(uint8(i + 1)); + } + } } /// @dev Exposes the internal revocation-chain walk and lets tests seed the `verifiedParent`