Skip to content

fix: bound BasicConstraints ASN.1 parsing #41

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
leanthebean wants to merge 1 commit into
base: main
Choose a base branch
from
+82 −8
Open

fix: bound BasicConstraints ASN.1 parsing #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
46 changes: 38 additions & 8 deletions src/CertManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,18 +453,48 @@ contract CertManager is ICertManager {
pure
returns (int64 maxPathLen)
{
require(certificate[valuePtr.header()] == 0x30, "invalid basicConstraints");

maxPathLen = -1;
Asn1Ptr basicConstraintsPtr = certificate.firstChildOf(valuePtr);
bool isCA;
if (certificate[basicConstraintsPtr.header()] == 0x01) {
require(basicConstraintsPtr.length() == 1, "invalid isCA bool value");
isCA = certificate[basicConstraintsPtr.content()] == 0xff;
basicConstraintsPtr = certificate.nextSiblingOf(basicConstraintsPtr);
uint256 end = valuePtr.content() + valuePtr.length();
uint256 cursor = valuePtr.content();

if (cursor < end) {
Asn1Ptr basicConstraintsPtr = certificate.firstChildOf(valuePtr);
cursor = _requireAsn1ChildWithin(basicConstraintsPtr, end);

if (certificate[basicConstraintsPtr.header()] == 0x01) {
require(basicConstraintsPtr.length() == 1, "invalid isCA bool value");
isCA = certificate[basicConstraintsPtr.content()] == 0xff;

if (cursor == end) {
require(ca == isCA, "isCA must be true for CA certs");
return maxPathLen;
}

basicConstraintsPtr = certificate.nextSiblingOf(basicConstraintsPtr);
cursor = _requireAsn1ChildWithin(basicConstraintsPtr, end);
}

require(ca == isCA, "isCA must be true for CA certs");

if (certificate[basicConstraintsPtr.header()] == 0x02) {
maxPathLen = int64(uint64(certificate.uintAt(basicConstraintsPtr)));

@leopoldjoy leopoldjoy Jun 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reject zero-length pathLenConstraint before calling uintAt; otherwise an in-bounds 02 00 child still makes uintAt read ptr.content() outside the BasicConstraints SEQUENCE and can accept the malformed INTEGER as 0.

} else {
revert("invalid basicConstraints field");
}

require(cursor == end, "trailing basicConstraints fields");
return maxPathLen;
}

require(ca == isCA, "isCA must be true for CA certs");
if (certificate[basicConstraintsPtr.header()] == 0x02) {
maxPathLen = int64(uint64(certificate.uintAt(basicConstraintsPtr)));
}
}

function _requireAsn1ChildWithin(Asn1Ptr ptr, uint256 parentEnd) internal pure returns (uint256 childEnd) {
childEnd = ptr.header() + ptr.totalLength();
require(childEnd <= parentEnd, "basicConstraints out of bounds");
}

function _verifyKeyUsageExtension(bytes memory certificate, Asn1Ptr valuePtr, bool ca) internal pure {
Expand Down
44 changes: 44 additions & 0 deletions test/CertManager.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,23 @@ contract Asn1DecodeHarness {
}
}

contract CertManagerHarness is CertManager {
using Asn1Decode for bytes;

constructor() CertManager(new P384Verifier()) {}

function verifyBasicConstraints(bytes memory der, bool ca) external pure returns (int64) {
return _verifyBasicConstraintsExtension(der, der.root(), ca);
}
}

contract CertManagerTest is Test {
Asn1DecodeHarness public harness;
CertManagerHarness public certManagerHarness;

function setUp() public {
harness = new Asn1DecodeHarness();
certManagerHarness = new CertManagerHarness();
}

// 's' INTEGER from cabundle[3] (2026-04-02 attestation): DER-encoded with a 0x00
Expand All @@ -41,6 +53,38 @@ contract CertManagerTest is Test {
assertEq(lo, 0xa2eda9c549dc01460f5fe650814ebe0e7ee855d3bcffde95afd2e82e21df0eac);
}

function test_BasicConstraintsEmptySequenceIsClientCert() public view {
assertEq(int256(certManagerHarness.verifyBasicConstraints(hex"3000", false)), -1);
}

function test_BasicConstraintsEmptySequenceRejectsCACert() public {
vm.expectRevert("isCA must be true for CA certs");
certManagerHarness.verifyBasicConstraints(hex"3000", true);
}

function test_BasicConstraintsAcceptsCAWithoutPathLen() public view {
assertEq(int256(certManagerHarness.verifyBasicConstraints(hex"30030101ff", true)), -1);
}

function test_BasicConstraintsAcceptsCAWithPathLen() public view {
assertEq(int256(certManagerHarness.verifyBasicConstraints(hex"30060101ff020100", true)), 0);
}

function test_BasicConstraintsRejectsOutOfBoundsChild() public {
vm.expectRevert("basicConstraints out of bounds");
certManagerHarness.verifyBasicConstraints(hex"3003020200", false);
}

function test_BasicConstraintsRejectsTrailingFields() public {
vm.expectRevert("trailing basicConstraints fields");
certManagerHarness.verifyBasicConstraints(hex"30090101ff020100020100", true);
}

function test_BasicConstraintsRejectsUnknownField() public {
vm.expectRevert("invalid basicConstraints field");
certManagerHarness.verifyBasicConstraints(hex"30020400", 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.
Expand Down
Loading