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

Merged
leopoldjoy merged 1 commit into from
Jul 6, 2026
Merged

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

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

event CertVerified(bytes32 indexed certHash);
event CertRevoked(bytes32 indexed certHash);
Expand Down Expand Up @@ -438,7 +440,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 @@ -449,16 +451,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 @@ -476,9 +478,7 @@ 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)
Expand Down
47 changes: 47 additions & 0 deletions test/CertManager.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ contract CertManagerPubKeyHarness 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;
Expand All @@ -58,11 +68,13 @@ contract CertManagerTest is Test {
Asn1DecodeHarness public harness;
CertManagerHarness public certManagerHarness;
CertManagerPubKeyHarness public certManagerPubKeyHarness;
CertManagerExtensionsHarness public certManagerExtensionsHarness;

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

// 's' INTEGER from cabundle[3] (2026-04-02 attestation): DER-encoded with a 0x00
Expand Down Expand Up @@ -155,6 +167,31 @@ contract CertManagerTest is Test {
certManagerPubKeyHarness.parsePubKey(spki);
}

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 @@ -412,6 +449,16 @@ contract CertManagerTest is Test {
out[i] = bytes1(uint8(i + 1));
}
}

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