Skip to content

fix: reject ambiguous cert field consumption #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
leopoldjoy merged 6 commits into from
Jul 7, 2026
Merged

fix: reject ambiguous cert field consumption #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
77fd22b
fix: reject ambiguous cert field consumption
leopoldjoy Jun 26, 2026
720310c
refactor: centralize bounded ASN.1 traversal
leopoldjoy Jun 26, 2026
95247de
Merge origin/main into strict cert field consumption
leopoldjoy Jul 6, 2026
e99197b
Merge origin/main into strict cert field consumption
leopoldjoy Jul 7, 2026
bae331a
Merge origin/main into strict cert field consumption
leopoldjoy Jul 7, 2026
f61e327
fix: require extension sequence to fill wrapper
leopoldjoy Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 30 additions & 17 deletions src/CertManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ contract CertManager is ICertManager {
error InvalidExtension();
error InvalidBasicConstraints();
error InvalidSubjectPublicKey();
error InvalidCertAlgorithm();
error InvalidCertVersion();
error UnsupportedCriticalExtension();
error NotOwner();
error NotRevoker();
Expand Down Expand Up @@ -306,7 +308,7 @@ contract CertManager is ICertManager {

Asn1Ptr root = certificate.root();
_requireAsn1Tag(certificate, root, 0x30);
require(root.totalLength() == certificate.length, "invalid cert length");
if (root.totalLength() != certificate.length) revert Asn1Decode.InvalidAsn1Length();
Asn1Ptr tbsCertPtr = certificate.firstChildOf(root);
_requireAsn1Tag(certificate, tbsCertPtr, 0x30);
return certificate.keccak(tbsCertPtr.header(), tbsCertPtr.totalLength());
Expand All @@ -329,7 +331,7 @@ contract CertManager is ICertManager {
bytes memory signatureHints
) internal view returns (VerifiedCert memory cert, bytes32 identity) {
Asn1Ptr root = certificate.root();
require(root.totalLength() == certificate.length, "invalid cert length");
if (root.totalLength() != certificate.length) revert Asn1Decode.InvalidAsn1Length();
Asn1Ptr tbsCertPtr = certificate.firstChildOf(root);
(uint64 notAfter, int64 maxPathLen, bytes32 issuerHash, bytes32 subjectHash, bytes memory pubKey) =
_parseTbs(certificate, tbsCertPtr, ca);
Expand Down Expand Up @@ -373,20 +375,20 @@ contract CertManager is ICertManager {
_requireAsn1Tag(certificate, ptr, 0x30);
Asn1Ptr versionPtr = certificate.firstChildOf(ptr);
_requireAsn1Tag(certificate, versionPtr, 0xa0);
Asn1Ptr vPtr = certificate.firstChildOf(versionPtr);
Asn1Ptr serialPtr = certificate.nextSiblingOf(versionPtr);
Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(serialPtr);
Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(certificate.nextSiblingOf(versionPtr));
_requireAsn1Tag(certificate, sigAlgoPtr, 0x30);

require(certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) == CERT_ALGO_OID, "invalid cert sig algo");
uint256 version = certificate.uintAt(vPtr);
if (certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) != CERT_ALGO_OID) {
revert InvalidCertAlgorithm();
}
// 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");
if (certificate.uintAt(certificate.firstChildOf(versionPtr)) != 2) revert InvalidCertVersion();

(notAfter, maxPathLen, issuerHash, subjectHash, pubKey) = _parseTbsInner(certificate, sigAlgoPtr, ca);
(notAfter, maxPathLen, issuerHash, subjectHash, pubKey) =
_parseTbsInner(certificate, sigAlgoPtr, ca, ptr.content() + ptr.length());
}

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)
Expand All @@ -411,6 +413,7 @@ contract CertManager is ICertManager {
// skip optional subjectUniqueID
extensionsPtr = certificate.nextSiblingOf(extensionsPtr);
}
if (extensionsPtr.content() + extensionsPtr.length() != tbsEnd) revert Asn1Decode.InvalidAsn1Length();

notAfter = _verifyValidity(certificate, validityPtr);
maxPathLen = _verifyExtensions(certificate, extensionsPtr, ca);
Expand Down Expand Up @@ -465,40 +468,48 @@ contract CertManager is ICertManager {
returns (int64 maxPathLen)
{
if (certificate[extensionsPtr.header()] != 0xa3) revert InvalidExtension();
uint256 end = extensionsPtr.content() + extensionsPtr.length();
extensionsPtr = certificate.firstChildOf(extensionsPtr);
_requireAsn1Tag(certificate, extensionsPtr, 0x30);
if (extensionsPtr.content() + extensionsPtr.length() != end) revert InvalidExtension();
Asn1Ptr extensionPtr = certificate.firstChildOf(extensionsPtr);
uint256 end = extensionsPtr.content() + extensionsPtr.length();
bool basicConstraintsFound = false;
bool keyUsageFound = false;
maxPathLen = -1;

while (true) {
uint256 extensionEnd = extensionPtr.content() + extensionPtr.length();
if (extensionEnd > end) revert InvalidExtension();
_requireAsn1Tag(certificate, extensionPtr, 0x30);
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;

Asn1Ptr valuePtr = certificate.nextSiblingOf(oidPtr);

if (certificate[valuePtr.header()] == 0x01) {
if (valuePtr.length() != 1) revert InvalidExtension();
if (!recognized && certificate[valuePtr.content()] != 0x00) revert UnsupportedCriticalExtension();
valuePtr = certificate.nextSiblingOf(valuePtr);
}

if (valuePtr.content() + valuePtr.length() != extensionEnd) revert InvalidExtension();

if (recognized) {
valuePtr = certificate.octetString(valuePtr);

if (oid == BASIC_CONSTRAINTS_OID) {
if (basicConstraintsFound) revert InvalidExtension();
basicConstraintsFound = true;
maxPathLen = _verifyBasicConstraintsExtension(certificate, valuePtr, ca);
} else {
if (keyUsageFound) revert InvalidExtension();
keyUsageFound = true;
_verifyKeyUsageExtension(certificate, valuePtr, ca);
}
}

if (extensionPtr.content() + extensionPtr.length() == end) {
if (extensionEnd == end) {
break;
}
extensionPtr = certificate.nextSiblingOf(extensionPtr);
Expand Down Expand Up @@ -562,9 +573,9 @@ contract CertManager is ICertManager {
// X.509 KeyUsage bits are MSB-first. bitstringUintAt keeps the first KeyUsage octet in the
// low byte, so these masks continue to target the same bits for one- or multi-octet values.
if (ca) {
require(value & 0x04 == 0x04, "CertSign must be present");
if (value & 0x04 != 0x04) revert InvalidExtension();
} else {
require(value & 0x80 == 0x80, "DigitalSignature must be present");
if (value & 0x80 != 0x80) revert InvalidExtension();
}
}

Expand All @@ -576,9 +587,11 @@ contract CertManager is ICertManager {
) internal view {
Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(ptr);
_requireAsn1Tag(certificate, sigAlgoPtr, 0x30);
require(certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) == CERT_ALGO_OID, "invalid cert sig algo");
if (certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) != CERT_ALGO_OID) {
revert InvalidCertAlgorithm();
}
Asn1Ptr sigPtr = certificate.nextSiblingOf(sigAlgoPtr);
require(sigPtr.header() + sigPtr.totalLength() == certificate.length, "trailing cert fields");
if (sigPtr.header() + sigPtr.totalLength() != certificate.length) revert Asn1Decode.InvalidAsn1Length();

bytes memory hash = Sha2Ext.sha384(certificate, ptr.header(), ptr.totalLength());
bytes memory sigPacked = _certSignature(certificate, sigPtr);
Expand Down
96 changes: 86 additions & 10 deletions test/CertManager.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,6 @@ contract CertManagerPubKeyHarness is CertManager {
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 CertManagerExtensionsHarness is CertManager {
Expand Down Expand Up @@ -127,6 +119,53 @@ contract CertManagerTest is Test {
certManagerHarness.verifyBasicConstraints(hex"30020400", false);
}

function test_ExtensionsRejectDuplicateBasicConstraints() public {
vm.expectRevert(CertManager.InvalidExtension.selector);
certManagerExtensionsHarness.verifyExtensions(
_extensions(bytes.concat(_basicConstraintsExtension(), _basicConstraintsExtension(), _keyUsageExtension())),
true
);
}

function test_ExtensionsRejectDuplicateKeyUsage() public {
vm.expectRevert(CertManager.InvalidExtension.selector);
certManagerExtensionsHarness.verifyExtensions(
_extensions(bytes.concat(_basicConstraintsExtension(), _keyUsageExtension(), _keyUsageExtension())), true
);
}

function test_ExtensionsRejectTrailingExtensionFields() public {
vm.expectRevert(CertManager.InvalidExtension.selector);
certManagerExtensionsHarness.verifyExtensions(
_extensions(bytes.concat(_basicConstraintsExtensionWithTrailingField(), _keyUsageExtension())), true
);
}

function test_ExtensionsRejectTrailingWrapperFields() public {
bytes memory inner = _derNode(0x30, bytes.concat(_basicConstraintsExtension(), _keyUsageExtension()));

vm.expectRevert(CertManager.InvalidExtension.selector);
certManagerExtensionsHarness.verifyExtensions(_derNode(0xa3, bytes.concat(inner, hex"0500")), true);
}

function test_ExtensionsRejectInnerSequencePastWrapper() public {
bytes memory inner = _derNode(0x30, bytes.concat(_basicConstraintsExtension(), _keyUsageExtension()));
bytes memory der = bytes.concat(bytes1(0xa3), bytes1(uint8(inner.length - 1)), inner);

vm.expectRevert(CertManager.InvalidExtension.selector);
certManagerExtensionsHarness.verifyExtensions(der, true);
}

function test_ParseTbsRejectsTrailingSignedFields() public {
vm.warp(1775145600);
CertManager cm = new CertManager(new P384Verifier());

bytes memory mutated = _appendTbsTrailingField(CB1);

vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector);
cm.verifyCACertWithHints(mutated, keccak256(CB0), "");
}

function test_ParsePubKeyAcceptsUncompressedP384Point() public view {
bytes memory pubKey = _patternBytes(96);
bytes memory spki = abi.encodePacked(hex"3076301006072a8648ce3d020106052b8104002203620004", pubKey);
Expand All @@ -137,10 +176,9 @@ contract CertManagerTest is Test {
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);
certManagerPubKeyHarness.parsePubKey(spki);
}

function test_ParsePubKeyRejectsOversizedP384Point() public {
Expand Down Expand Up @@ -370,6 +408,17 @@ 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 _appendSignatureTrailingField(bytes memory certificate) internal pure returns (bytes memory result) {
(Asn1Ptr root, Asn1Ptr sigPtr, Asn1Ptr sigRoot,) = _certSignaturePtrs(certificate);
bytes memory extraInteger = hex"020100";
Expand Down Expand Up @@ -398,6 +447,33 @@ contract CertManagerTest is Test {
}
}

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
Expand Down
6 changes: 3 additions & 3 deletions test/hinted/HintedNitroAttestation.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ contract HintedNitroAttestationTest is Test {
NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs);
(bytes memory caCert, bytes32 parentHash,) = _firstNonRootCA(attestationTbs, ptrs);

vm.expectRevert("invalid cert length");
vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector);
certManager.verifyCACertWithHints(abi.encodePacked(caCert, bytes1(0x00)), parentHash, "");
}

Expand Down Expand Up @@ -511,7 +511,7 @@ contract HintedNitroAttestationTest is Test {
bytes memory shadowRootHints =
hintCollector.collectCertSignatureHints(shadowRoot, certManager.loadVerified(rootHash).pubKey);

vm.expectRevert("invalid cert length");
vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector);
certManager.verifyCACertWithHints(shadowRoot, rootHash, shadowRootHints);
assertEq(certManager.loadVerified(shadowRootHash).pubKey.length, 0, "shadow root must not cache");

Expand All @@ -525,7 +525,7 @@ contract HintedNitroAttestationTest is Test {
NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs);
(bytes memory caCert, bytes32 parentHash,) = _firstNonRootCA(attestationTbs, ptrs);

vm.expectRevert("trailing cert fields");
vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector);
certManager.verifyCACertWithHints(_appendInsideOuterSequence(caCert, bytes1(0x00)), parentHash, "");
}

Expand Down
Loading