Skip to content

fix: reject unknown critical cert extensions #43

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
Open

fix: reject unknown critical cert extensions #43

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
44 changes: 23 additions & 21 deletions src/CertManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ contract CertManager is ICertManager {
using LibAsn1Ptr for Asn1Ptr;
using LibBytes for bytes;

error InvalidExtension();
error InvalidBasicConstraints();
error UnsupportedCriticalExtension();

event CertVerified(bytes32 indexed certHash);
event CertRevoked(bytes32 indexed certHash);
event CertUnrevoked(bytes32 indexed certHash);
Expand Down Expand Up @@ -433,7 +437,7 @@ contract CertManager is ICertManager {
pure
returns (int64 maxPathLen)
{
require(certificate[extensionsPtr.header()] == 0xa3, "invalid extensions");
if (certificate[extensionsPtr.header()] != 0xa3) revert InvalidExtension();
extensionsPtr = certificate.firstChildOf(extensionsPtr);
Asn1Ptr extensionPtr = certificate.firstChildOf(extensionsPtr);
uint256 end = extensionsPtr.content() + extensionsPtr.length();
Expand All @@ -444,16 +448,16 @@ contract CertManager is ICertManager {
while (true) {
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;

if (oid == BASIC_CONSTRAINTS_OID || oid == KEY_USAGE_OID) {
Asn1Ptr valuePtr = certificate.nextSiblingOf(oidPtr);

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) {
if (valuePtr.length() != 1) revert InvalidExtension();
if (!recognized && certificate[valuePtr.content()] != 0x00) revert UnsupportedCriticalExtension();
valuePtr = certificate.nextSiblingOf(valuePtr);
}

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

if (oid == BASIC_CONSTRAINTS_OID) {
Expand All @@ -471,17 +475,15 @@ contract CertManager is ICertManager {
extensionPtr = certificate.nextSiblingOf(extensionPtr);
}

require(basicConstraintsFound, "basicConstraints not found");
require(keyUsageFound, "keyUsage not found");
require(ca || maxPathLen == -1, "maxPathLen must be undefined for client cert");
if (!basicConstraintsFound || !keyUsageFound || (!ca && maxPathLen != -1)) revert InvalidExtension();
}

function _verifyBasicConstraintsExtension(bytes memory certificate, Asn1Ptr valuePtr, bool ca)
internal
pure
returns (int64 maxPathLen)
{
require(certificate[valuePtr.header()] == 0x30, "invalid basicConstraints");
if (certificate[valuePtr.header()] != 0x30) revert InvalidBasicConstraints();

maxPathLen = -1;
bool isCA;
Expand All @@ -493,37 +495,37 @@ 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;
}

basicConstraintsPtr = certificate.nextSiblingOf(basicConstraintsPtr);
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 {
Expand Down
57 changes: 52 additions & 5 deletions test/CertManager.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,29 @@ contract CertManagerHarness is CertManager {
}
}

contract CertManagerExtensionsHarness is CertManager {
using Asn1Decode for bytes;

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

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

contract CertManagerTest is Test {
using Asn1Decode for bytes;
using LibAsn1Ptr for Asn1Ptr;
using LibBytes for bytes;

Asn1DecodeHarness public harness;
CertManagerHarness public certManagerHarness;
CertManagerExtensionsHarness public certManagerExtensionsHarness;

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

// 's' INTEGER from cabundle[3] (2026-04-02 attestation): DER-encoded with a 0x00
Expand All @@ -63,7 +75,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);
}

Expand All @@ -76,25 +88,50 @@ 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_VerifyExtensionsAllowsUnknownNonCriticalExtension() public view {
bytes memory unknownNameConstraints = hex"30090603551d1e04023000";

assertEq(
int256(certManagerExtensionsHarness.verifyExtensions(_clientExtensionsWith(unknownNameConstraints), false)),
-1
);
}

function test_VerifyExtensionsAllowsUnknownCriticalFalseExtension() public view {
bytes memory unknownNameConstraints = hex"300c0603551d1e01010004023000";

assertEq(
int256(certManagerExtensionsHarness.verifyExtensions(_clientExtensionsWith(unknownNameConstraints), false)),
-1
);
}

function test_VerifyExtensionsRejectsUnknownCriticalExtension() public {
bytes memory unknownNameConstraints = hex"300c0603551d1e0101ff04023000";

vm.expectRevert(CertManager.UnsupportedCriticalExtension.selector);
certManagerExtensionsHarness.verifyExtensions(_clientExtensionsWith(unknownNameConstraints), 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 Expand Up @@ -345,6 +382,16 @@ contract CertManagerTest is Test {

return der;
}

function _clientExtensionsWith(bytes memory extraExtension) internal pure returns (bytes memory) {
bytes memory body =
abi.encodePacked(hex"300c0603551d130101ff04023000", hex"300e0603551d0f0101ff040403020780", extraExtension);

return
abi.encodePacked(
bytes1(0xa3), bytes1(uint8(body.length + 2)), bytes1(0x30), bytes1(uint8(body.length)), body
);
}
}

/// @dev Exposes the internal revocation-chain walk and lets tests seed the `verifiedParent`
Expand Down
Loading