From 77fd22beeae5d6c3fa42d96adb3ad547855047d7 Mon Sep 17 00:00:00 2001 From: Leopold Joy Date: Fri, 26 Jun 2026 12:49:15 +0100 Subject: [PATCH 1/2] fix: reject ambiguous cert field consumption Co-authored-by: OpenCode --- src/CertManager.sol | 54 +++++++++++++++++------- test/CertManager.t.sol | 94 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 14 deletions(-) diff --git a/src/CertManager.sol b/src/CertManager.sol index d1d9515..dc31805 100644 --- a/src/CertManager.sol +++ b/src/CertManager.sol @@ -349,40 +349,51 @@ contract CertManager is ICertManager { view returns (uint64 notAfter, int64 maxPathLen, bytes32 issuerHash, bytes32 subjectHash, bytes memory pubKey) { + Asn1Ptr sigAlgoPtr = _verifyTbsHeader(certificate, ptr); + + (notAfter, maxPathLen, issuerHash, subjectHash, pubKey) = + _parseTbsInner(certificate, sigAlgoPtr, ca, ptr.content() + ptr.length()); + } + + function _verifyTbsHeader(bytes memory certificate, Asn1Ptr ptr) internal pure returns (Asn1Ptr sigAlgoPtr) { Asn1Ptr versionPtr = certificate.firstChildOf(ptr); Asn1Ptr vPtr = certificate.firstChildOf(versionPtr); - Asn1Ptr serialPtr = certificate.nextSiblingOf(versionPtr); - Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(serialPtr); + sigAlgoPtr = certificate.nextSiblingOf(certificate.nextSiblingOf(versionPtr)); require(certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) == CERT_ALGO_OID, "invalid cert sig algo"); uint256 version = certificate.uintAt(vPtr); // as extensions are used in cert, version should be 3 (value 2) as per https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.1 require(version == 2, "version should be 3"); - - (notAfter, maxPathLen, issuerHash, subjectHash, pubKey) = _parseTbsInner(certificate, sigAlgoPtr, ca); } - function _parseTbsInner(bytes memory certificate, Asn1Ptr sigAlgoPtr, bool ca) + function _parseTbsInner(bytes memory certificate, Asn1Ptr sigAlgoPtr, bool ca, uint256 tbsEnd) internal view returns (uint64 notAfter, int64 maxPathLen, bytes32 issuerHash, bytes32 subjectHash, bytes memory pubKey) { Asn1Ptr issuerPtr = certificate.nextSiblingOf(sigAlgoPtr); + _requireAsn1NodeWithin(issuerPtr, tbsEnd); issuerHash = certificate.keccak(issuerPtr.content(), issuerPtr.length()); Asn1Ptr validityPtr = certificate.nextSiblingOf(issuerPtr); + _requireAsn1NodeWithin(validityPtr, tbsEnd); Asn1Ptr subjectPtr = certificate.nextSiblingOf(validityPtr); + _requireAsn1NodeWithin(subjectPtr, tbsEnd); subjectHash = certificate.keccak(subjectPtr.content(), subjectPtr.length()); Asn1Ptr subjectPublicKeyInfoPtr = certificate.nextSiblingOf(subjectPtr); + _requireAsn1NodeWithin(subjectPublicKeyInfoPtr, tbsEnd); Asn1Ptr extensionsPtr = certificate.nextSiblingOf(subjectPublicKeyInfoPtr); if (certificate[extensionsPtr.header()] == 0x81) { // skip optional issuerUniqueID + _requireAsn1NodeWithin(extensionsPtr, tbsEnd); extensionsPtr = certificate.nextSiblingOf(extensionsPtr); } if (certificate[extensionsPtr.header()] == 0x82) { // skip optional subjectUniqueID + _requireAsn1NodeWithin(extensionsPtr, tbsEnd); extensionsPtr = certificate.nextSiblingOf(extensionsPtr); } + require(_requireAsn1NodeWithin(extensionsPtr, tbsEnd) == tbsEnd, "trailing tbs fields"); notAfter = _verifyValidity(certificate, validityPtr); maxPathLen = _verifyExtensions(certificate, extensionsPtr, ca); @@ -442,30 +453,40 @@ contract CertManager is ICertManager { maxPathLen = -1; while (true) { + uint256 extensionEnd = _requireAsn1NodeWithin(extensionPtr, end); Asn1Ptr oidPtr = certificate.firstChildOf(extensionPtr); + _requireAsn1NodeWithin(oidPtr, extensionEnd); bytes32 oid = certificate.keccak(oidPtr.content(), oidPtr.length()); - if (oid == BASIC_CONSTRAINTS_OID || oid == KEY_USAGE_OID) { - Asn1Ptr valuePtr = certificate.nextSiblingOf(oidPtr); + Asn1Ptr valuePtr = certificate.nextSiblingOf(oidPtr); + _requireAsn1NodeWithin(valuePtr, extensionEnd); - 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) { + // skip optional critical bool + require(valuePtr.length() == 1, "invalid critical bool value"); + valuePtr = certificate.nextSiblingOf(valuePtr); + _requireAsn1NodeWithin(valuePtr, extensionEnd); + } - valuePtr = certificate.octetString(valuePtr); + require(_requireAsn1NodeWithin(valuePtr, extensionEnd) == extensionEnd, "trailing extension fields"); + + if (oid == BASIC_CONSTRAINTS_OID || oid == KEY_USAGE_OID) { + Asn1Ptr valueNodePtr = valuePtr; + + valuePtr = certificate.octetString(valueNodePtr); if (oid == BASIC_CONSTRAINTS_OID) { + require(!basicConstraintsFound, "duplicate basicConstraints"); basicConstraintsFound = true; maxPathLen = _verifyBasicConstraintsExtension(certificate, valuePtr, ca); } else { + require(!keyUsageFound, "duplicate keyUsage"); keyUsageFound = true; _verifyKeyUsageExtension(certificate, valuePtr, ca); } } - if (extensionPtr.content() + extensionPtr.length() == end) { + if (extensionEnd == end) { break; } extensionPtr = certificate.nextSiblingOf(extensionPtr); @@ -526,6 +547,11 @@ contract CertManager is ICertManager { require(childEnd <= parentEnd, "basicConstraints out of bounds"); } + function _requireAsn1NodeWithin(Asn1Ptr ptr, uint256 parentEnd) internal pure returns (uint256 nodeEnd) { + nodeEnd = ptr.header() + ptr.totalLength(); + require(nodeEnd <= parentEnd, "ASN.1 node out of bounds"); + } + function _verifyKeyUsageExtension(bytes memory certificate, Asn1Ptr valuePtr, bool ca) internal pure { uint256 value = certificate.bitstringUintAt(valuePtr); // X.509 KeyUsage bits are MSB-first. bitstringUintAt keeps the first KeyUsage octet in the diff --git a/test/CertManager.t.sol b/test/CertManager.t.sol index cc40cfd..dacb3e7 100644 --- a/test/CertManager.t.sol +++ b/test/CertManager.t.sol @@ -30,6 +30,16 @@ contract CertManagerHarness is CertManager { function verifyBasicConstraints(bytes memory der, bool ca) external pure returns (int64) { return _verifyBasicConstraintsExtension(der, der.root(), ca); } + + function verifyExtensions(bytes memory der, bool ca) external pure returns (int64) { + return _verifyExtensions(der, der.root(), ca); + } + + function parseTbs(bytes memory cert, bool ca) external view { + Asn1Ptr root = cert.root(); + Asn1Ptr tbsPtr = cert.firstChildOf(root); + _parseTbs(cert, tbsPtr, ca); + } } contract CertManagerTest is Test { @@ -95,6 +105,35 @@ contract CertManagerTest is Test { certManagerHarness.verifyBasicConstraints(hex"30020400", false); } + function test_ExtensionsRejectDuplicateBasicConstraints() public { + vm.expectRevert("duplicate basicConstraints"); + certManagerHarness.verifyExtensions( + _extensions(bytes.concat(_basicConstraintsExtension(), _basicConstraintsExtension(), _keyUsageExtension())), + true + ); + } + + function test_ExtensionsRejectDuplicateKeyUsage() public { + vm.expectRevert("duplicate keyUsage"); + certManagerHarness.verifyExtensions( + _extensions(bytes.concat(_basicConstraintsExtension(), _keyUsageExtension(), _keyUsageExtension())), true + ); + } + + function test_ExtensionsRejectTrailingExtensionFields() public { + vm.expectRevert("trailing extension fields"); + certManagerHarness.verifyExtensions( + _extensions(bytes.concat(_basicConstraintsExtensionWithTrailingField(), _keyUsageExtension())), true + ); + } + + function test_ParseTbsRejectsTrailingSignedFields() public { + bytes memory mutated = _appendTbsTrailingField(CB1); + + vm.expectRevert("trailing tbs fields"); + certManagerHarness.parseTbs(mutated, true); + } + // 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. @@ -212,6 +251,61 @@ contract CertManagerTest is Test { _writeDerLength(result, sigRoot, _addDelta(sigRoot.length(), delta)); } + function _appendTbsTrailingField(bytes memory certificate) internal pure returns (bytes memory result) { + Asn1Ptr root = certificate.root(); + Asn1Ptr tbsPtr = certificate.firstChildOf(root); + bytes memory nullField = hex"0500"; + int256 delta = int256(nullField.length); + result = _insertBytes(certificate, tbsPtr.content() + tbsPtr.length(), nullField); + + _writeDerLength(result, root, _addDelta(root.length(), delta)); + _writeDerLength(result, tbsPtr, _addDelta(tbsPtr.length(), delta)); + } + + function _insertBytes(bytes memory input, uint256 offset, bytes memory inserted) + internal + pure + returns (bytes memory result) + { + result = new bytes(input.length + inserted.length); + for (uint256 i = 0; i < offset; ++i) { + result[i] = input[i]; + } + for (uint256 i = 0; i < inserted.length; ++i) { + result[offset + i] = inserted[i]; + } + for (uint256 i = offset; i < input.length; ++i) { + result[i + inserted.length] = input[i]; + } + } + + function _extensions(bytes memory extensionList) internal pure returns (bytes memory) { + return _derNode(0xa3, _derNode(0x30, extensionList)); + } + + function _basicConstraintsExtension() internal pure returns (bytes memory) { + return _derNode(0x30, bytes.concat(hex"0603551d13", hex"0101ff", _derNode(0x04, hex"30060101ff020100"))); + } + + function _basicConstraintsExtensionWithTrailingField() internal pure returns (bytes memory) { + return + _derNode(0x30, bytes.concat(hex"0603551d13", hex"0101ff", _derNode(0x04, hex"30060101ff020100"), hex"0500")); + } + + function _keyUsageExtension() internal pure returns (bytes memory) { + return _derNode(0x30, bytes.concat(hex"0603551d0f", hex"0101ff", _derNode(0x04, hex"03020186"))); + } + + function _derNode(bytes1 tag, bytes memory content) internal pure returns (bytes memory der) { + require(content.length < 128, "test: long-form length not supported"); + der = new bytes(2 + content.length); + der[0] = tag; + der[1] = bytes1(uint8(content.length)); + for (uint256 i = 0; i < content.length; ++i) { + der[2 + i] = content[i]; + } + } + function _certSignaturePtrs(bytes memory certificate) internal pure From 720310ce8c031dde5dd77958df8ced0d9da22abf Mon Sep 17 00:00:00 2001 From: Leopold Joy Date: Fri, 26 Jun 2026 23:05:32 +0100 Subject: [PATCH 2/2] refactor: centralize bounded ASN.1 traversal Co-authored-by: OpenCode --- src/CertManager.sol | 47 +++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/src/CertManager.sol b/src/CertManager.sol index dc31805..fb93b7f 100644 --- a/src/CertManager.sol +++ b/src/CertManager.sol @@ -371,27 +371,21 @@ contract CertManager is ICertManager { view returns (uint64 notAfter, int64 maxPathLen, bytes32 issuerHash, bytes32 subjectHash, bytes memory pubKey) { - Asn1Ptr issuerPtr = certificate.nextSiblingOf(sigAlgoPtr); - _requireAsn1NodeWithin(issuerPtr, tbsEnd); + Asn1Ptr issuerPtr = _nextSiblingWithin(certificate, sigAlgoPtr, tbsEnd); issuerHash = certificate.keccak(issuerPtr.content(), issuerPtr.length()); - Asn1Ptr validityPtr = certificate.nextSiblingOf(issuerPtr); - _requireAsn1NodeWithin(validityPtr, tbsEnd); - Asn1Ptr subjectPtr = certificate.nextSiblingOf(validityPtr); - _requireAsn1NodeWithin(subjectPtr, tbsEnd); + Asn1Ptr validityPtr = _nextSiblingWithin(certificate, issuerPtr, tbsEnd); + Asn1Ptr subjectPtr = _nextSiblingWithin(certificate, validityPtr, tbsEnd); subjectHash = certificate.keccak(subjectPtr.content(), subjectPtr.length()); - Asn1Ptr subjectPublicKeyInfoPtr = certificate.nextSiblingOf(subjectPtr); - _requireAsn1NodeWithin(subjectPublicKeyInfoPtr, tbsEnd); - Asn1Ptr extensionsPtr = certificate.nextSiblingOf(subjectPublicKeyInfoPtr); + Asn1Ptr subjectPublicKeyInfoPtr = _nextSiblingWithin(certificate, subjectPtr, tbsEnd); + Asn1Ptr extensionsPtr = _nextSiblingWithin(certificate, subjectPublicKeyInfoPtr, tbsEnd); if (certificate[extensionsPtr.header()] == 0x81) { // skip optional issuerUniqueID - _requireAsn1NodeWithin(extensionsPtr, tbsEnd); - extensionsPtr = certificate.nextSiblingOf(extensionsPtr); + extensionsPtr = _nextSiblingWithin(certificate, extensionsPtr, tbsEnd); } if (certificate[extensionsPtr.header()] == 0x82) { // skip optional subjectUniqueID - _requireAsn1NodeWithin(extensionsPtr, tbsEnd); - extensionsPtr = certificate.nextSiblingOf(extensionsPtr); + extensionsPtr = _nextSiblingWithin(certificate, extensionsPtr, tbsEnd); } require(_requireAsn1NodeWithin(extensionsPtr, tbsEnd) == tbsEnd, "trailing tbs fields"); @@ -446,26 +440,23 @@ contract CertManager is ICertManager { { require(certificate[extensionsPtr.header()] == 0xa3, "invalid extensions"); extensionsPtr = certificate.firstChildOf(extensionsPtr); - Asn1Ptr extensionPtr = certificate.firstChildOf(extensionsPtr); uint256 end = extensionsPtr.content() + extensionsPtr.length(); + Asn1Ptr extensionPtr = _firstChildWithin(certificate, extensionsPtr, end); bool basicConstraintsFound = false; bool keyUsageFound = false; maxPathLen = -1; while (true) { uint256 extensionEnd = _requireAsn1NodeWithin(extensionPtr, end); - Asn1Ptr oidPtr = certificate.firstChildOf(extensionPtr); - _requireAsn1NodeWithin(oidPtr, extensionEnd); + Asn1Ptr oidPtr = _firstChildWithin(certificate, extensionPtr, extensionEnd); bytes32 oid = certificate.keccak(oidPtr.content(), oidPtr.length()); - Asn1Ptr valuePtr = certificate.nextSiblingOf(oidPtr); - _requireAsn1NodeWithin(valuePtr, extensionEnd); + Asn1Ptr valuePtr = _nextSiblingWithin(certificate, oidPtr, extensionEnd); if (certificate[valuePtr.header()] == 0x01) { // skip optional critical bool require(valuePtr.length() == 1, "invalid critical bool value"); - valuePtr = certificate.nextSiblingOf(valuePtr); - _requireAsn1NodeWithin(valuePtr, extensionEnd); + valuePtr = _nextSiblingWithin(certificate, valuePtr, extensionEnd); } require(_requireAsn1NodeWithin(valuePtr, extensionEnd) == extensionEnd, "trailing extension fields"); @@ -489,7 +480,7 @@ contract CertManager is ICertManager { if (extensionEnd == end) { break; } - extensionPtr = certificate.nextSiblingOf(extensionPtr); + extensionPtr = _nextSiblingWithin(certificate, extensionPtr, end); } require(basicConstraintsFound, "basicConstraints not found"); @@ -552,6 +543,20 @@ contract CertManager is ICertManager { require(nodeEnd <= parentEnd, "ASN.1 node out of bounds"); } + function _firstChildWithin(bytes memory der, Asn1Ptr ptr, uint256 parentEnd) internal pure returns (Asn1Ptr child) { + child = der.firstChildOf(ptr); + _requireAsn1NodeWithin(child, parentEnd); + } + + function _nextSiblingWithin(bytes memory der, Asn1Ptr ptr, uint256 parentEnd) + internal + pure + returns (Asn1Ptr sibling) + { + sibling = der.nextSiblingOf(ptr); + _requireAsn1NodeWithin(sibling, parentEnd); + } + function _verifyKeyUsageExtension(bytes memory certificate, Asn1Ptr valuePtr, bool ca) internal pure { uint256 value = certificate.bitstringUintAt(valuePtr); // X.509 KeyUsage bits are MSB-first. bitstringUintAt keeps the first KeyUsage octet in the