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

Open
leopoldjoy wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: reject ambiguous cert field consumption #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 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
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
79 changes: 55 additions & 24 deletions src/CertManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,40 +349,45 @@ 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);
Asn1Ptr issuerPtr = _nextSiblingWithin(certificate, sigAlgoPtr, tbsEnd);
issuerHash = certificate.keccak(issuerPtr.content(), issuerPtr.length());
Asn1Ptr validityPtr = certificate.nextSiblingOf(issuerPtr);
Asn1Ptr subjectPtr = certificate.nextSiblingOf(validityPtr);
Asn1Ptr validityPtr = _nextSiblingWithin(certificate, issuerPtr, tbsEnd);
Asn1Ptr subjectPtr = _nextSiblingWithin(certificate, validityPtr, tbsEnd);
subjectHash = certificate.keccak(subjectPtr.content(), subjectPtr.length());
Asn1Ptr subjectPublicKeyInfoPtr = certificate.nextSiblingOf(subjectPtr);
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
extensionsPtr = certificate.nextSiblingOf(extensionsPtr);
extensionsPtr = _nextSiblingWithin(certificate, extensionsPtr, tbsEnd);
}
if (certificate[extensionsPtr.header()] == 0x82) {
// skip optional subjectUniqueID
extensionsPtr = certificate.nextSiblingOf(extensionsPtr);
extensionsPtr = _nextSiblingWithin(certificate, extensionsPtr, tbsEnd);
}
require(_requireAsn1NodeWithin(extensionsPtr, tbsEnd) == tbsEnd, "trailing tbs fields");

notAfter = _verifyValidity(certificate, validityPtr);
maxPathLen = _verifyExtensions(certificate, extensionsPtr, ca);
Expand Down Expand Up @@ -435,40 +440,47 @@ 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) {
Asn1Ptr oidPtr = certificate.firstChildOf(extensionPtr);
uint256 extensionEnd = _requireAsn1NodeWithin(extensionPtr, end);
Asn1Ptr oidPtr = _firstChildWithin(certificate, extensionPtr, 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 = _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);
}
if (certificate[valuePtr.header()] == 0x01) {
// skip optional critical bool
require(valuePtr.length() == 1, "invalid critical bool value");
valuePtr = _nextSiblingWithin(certificate, valuePtr, extensionEnd);
}

require(_requireAsn1NodeWithin(valuePtr, extensionEnd) == extensionEnd, "trailing extension fields");

valuePtr = certificate.octetString(valuePtr);
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);
extensionPtr = _nextSiblingWithin(certificate, extensionPtr, end);
}

require(basicConstraintsFound, "basicConstraints not found");
Expand Down Expand Up @@ -526,6 +538,25 @@ 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 _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
Expand Down
94 changes: 94 additions & 0 deletions test/CertManager.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
Loading