Skip to content

Commit 77fd22b

Browse files
leopoldjoyOpenCode
leopoldjoy
and
OpenCode
committed
fix: reject ambiguous cert field consumption
Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
1 parent c0a9f21 commit 77fd22b

2 files changed

Lines changed: 134 additions & 14 deletions

File tree

‎src/CertManager.sol‎

Lines changed: 40 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -349,40 +349,51 @@ contract CertManager is ICertManager {
349349
view
350350
returns (uint64 notAfter, int64 maxPathLen, bytes32 issuerHash, bytes32 subjectHash, bytes memory pubKey)
351351
{
352+
Asn1Ptr sigAlgoPtr = _verifyTbsHeader(certificate, ptr);
353+
354+
(notAfter, maxPathLen, issuerHash, subjectHash, pubKey) =
355+
_parseTbsInner(certificate, sigAlgoPtr, ca, ptr.content() + ptr.length());
356+
}
357+
358+
function _verifyTbsHeader(bytes memory certificate, Asn1Ptr ptr) internal pure returns (Asn1Ptr sigAlgoPtr) {
352359
Asn1Ptr versionPtr = certificate.firstChildOf(ptr);
353360
Asn1Ptr vPtr = certificate.firstChildOf(versionPtr);
354-
Asn1Ptr serialPtr = certificate.nextSiblingOf(versionPtr);
355-
Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(serialPtr);
361+
sigAlgoPtr = certificate.nextSiblingOf(certificate.nextSiblingOf(versionPtr));
356362

357363
require(certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) == CERT_ALGO_OID, "invalid cert sig algo");
358364
uint256 version = certificate.uintAt(vPtr);
359365
// 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
360366
require(version == 2, "version should be 3");
361-
362-
(notAfter, maxPathLen, issuerHash, subjectHash, pubKey) = _parseTbsInner(certificate, sigAlgoPtr, ca);
363367
}
364368

365-
function _parseTbsInner(bytes memory certificate, Asn1Ptr sigAlgoPtr, bool ca)
369+
function _parseTbsInner(bytes memory certificate, Asn1Ptr sigAlgoPtr, bool ca, uint256 tbsEnd)
366370
internal
367371
view
368372
returns (uint64 notAfter, int64 maxPathLen, bytes32 issuerHash, bytes32 subjectHash, bytes memory pubKey)
369373
{
370374
Asn1Ptr issuerPtr = certificate.nextSiblingOf(sigAlgoPtr);
375+
_requireAsn1NodeWithin(issuerPtr, tbsEnd);
371376
issuerHash = certificate.keccak(issuerPtr.content(), issuerPtr.length());
372377
Asn1Ptr validityPtr = certificate.nextSiblingOf(issuerPtr);
378+
_requireAsn1NodeWithin(validityPtr, tbsEnd);
373379
Asn1Ptr subjectPtr = certificate.nextSiblingOf(validityPtr);
380+
_requireAsn1NodeWithin(subjectPtr, tbsEnd);
374381
subjectHash = certificate.keccak(subjectPtr.content(), subjectPtr.length());
375382
Asn1Ptr subjectPublicKeyInfoPtr = certificate.nextSiblingOf(subjectPtr);
383+
_requireAsn1NodeWithin(subjectPublicKeyInfoPtr, tbsEnd);
376384
Asn1Ptr extensionsPtr = certificate.nextSiblingOf(subjectPublicKeyInfoPtr);
377385

378386
if (certificate[extensionsPtr.header()] == 0x81) {
379387
// skip optional issuerUniqueID
388+
_requireAsn1NodeWithin(extensionsPtr, tbsEnd);
380389
extensionsPtr = certificate.nextSiblingOf(extensionsPtr);
381390
}
382391
if (certificate[extensionsPtr.header()] == 0x82) {
383392
// skip optional subjectUniqueID
393+
_requireAsn1NodeWithin(extensionsPtr, tbsEnd);
384394
extensionsPtr = certificate.nextSiblingOf(extensionsPtr);
385395
}
396+
require(_requireAsn1NodeWithin(extensionsPtr, tbsEnd) == tbsEnd, "trailing tbs fields");
386397

387398
notAfter = _verifyValidity(certificate, validityPtr);
388399
maxPathLen = _verifyExtensions(certificate, extensionsPtr, ca);
@@ -442,30 +453,40 @@ contract CertManager is ICertManager {
442453
maxPathLen = -1;
443454

444455
while (true) {
456+
uint256 extensionEnd = _requireAsn1NodeWithin(extensionPtr, end);
445457
Asn1Ptr oidPtr = certificate.firstChildOf(extensionPtr);
458+
_requireAsn1NodeWithin(oidPtr, extensionEnd);
446459
bytes32 oid = certificate.keccak(oidPtr.content(), oidPtr.length());
447460

448-
if (oid == BASIC_CONSTRAINTS_OID || oid == KEY_USAGE_OID) {
449-
Asn1Ptr valuePtr = certificate.nextSiblingOf(oidPtr);
461+
Asn1Ptr valuePtr = certificate.nextSiblingOf(oidPtr);
462+
_requireAsn1NodeWithin(valuePtr, extensionEnd);
450463

451-
if (certificate[valuePtr.header()] == 0x01) {
452-
// skip optional critical bool
453-
require(valuePtr.length() == 1, "invalid critical bool value");
454-
valuePtr = certificate.nextSiblingOf(valuePtr);
455-
}
464+
if (certificate[valuePtr.header()] == 0x01) {
465+
// skip optional critical bool
466+
require(valuePtr.length() == 1, "invalid critical bool value");
467+
valuePtr = certificate.nextSiblingOf(valuePtr);
468+
_requireAsn1NodeWithin(valuePtr, extensionEnd);
469+
}
456470

457-
valuePtr = certificate.octetString(valuePtr);
471+
require(_requireAsn1NodeWithin(valuePtr, extensionEnd) == extensionEnd, "trailing extension fields");
472+
473+
if (oid == BASIC_CONSTRAINTS_OID || oid == KEY_USAGE_OID) {
474+
Asn1Ptr valueNodePtr = valuePtr;
475+
476+
valuePtr = certificate.octetString(valueNodePtr);
458477

459478
if (oid == BASIC_CONSTRAINTS_OID) {
479+
require(!basicConstraintsFound, "duplicate basicConstraints");
460480
basicConstraintsFound = true;
461481
maxPathLen = _verifyBasicConstraintsExtension(certificate, valuePtr, ca);
462482
} else {
483+
require(!keyUsageFound, "duplicate keyUsage");
463484
keyUsageFound = true;
464485
_verifyKeyUsageExtension(certificate, valuePtr, ca);
465486
}
466487
}
467488

468-
if (extensionPtr.content() + extensionPtr.length() == end) {
489+
if (extensionEnd == end) {
469490
break;
470491
}
471492
extensionPtr = certificate.nextSiblingOf(extensionPtr);
@@ -526,6 +547,11 @@ contract CertManager is ICertManager {
526547
require(childEnd <= parentEnd, "basicConstraints out of bounds");
527548
}
528549

550+
function _requireAsn1NodeWithin(Asn1Ptr ptr, uint256 parentEnd) internal pure returns (uint256 nodeEnd) {
551+
nodeEnd = ptr.header() + ptr.totalLength();
552+
require(nodeEnd <= parentEnd, "ASN.1 node out of bounds");
553+
}
554+
529555
function _verifyKeyUsageExtension(bytes memory certificate, Asn1Ptr valuePtr, bool ca) internal pure {
530556
uint256 value = certificate.bitstringUintAt(valuePtr);
531557
// X.509 KeyUsage bits are MSB-first. bitstringUintAt keeps the first KeyUsage octet in the

‎test/CertManager.t.sol‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,16 @@ contract CertManagerHarness is CertManager {
3030
function verifyBasicConstraints(bytes memory der, bool ca) external pure returns (int64) {
3131
return _verifyBasicConstraintsExtension(der, der.root(), ca);
3232
}
33+
34+
function verifyExtensions(bytes memory der, bool ca) external pure returns (int64) {
35+
return _verifyExtensions(der, der.root(), ca);
36+
}
37+
38+
function parseTbs(bytes memory cert, bool ca) external view {
39+
Asn1Ptr root = cert.root();
40+
Asn1Ptr tbsPtr = cert.firstChildOf(root);
41+
_parseTbs(cert, tbsPtr, ca);
42+
}
3343
}
3444

3545
contract CertManagerTest is Test {
@@ -95,6 +105,35 @@ contract CertManagerTest is Test {
95105
certManagerHarness.verifyBasicConstraints(hex"30020400", false);
96106
}
97107

108+
function test_ExtensionsRejectDuplicateBasicConstraints() public {
109+
vm.expectRevert("duplicate basicConstraints");
110+
certManagerHarness.verifyExtensions(
111+
_extensions(bytes.concat(_basicConstraintsExtension(), _basicConstraintsExtension(), _keyUsageExtension())),
112+
true
113+
);
114+
}
115+
116+
function test_ExtensionsRejectDuplicateKeyUsage() public {
117+
vm.expectRevert("duplicate keyUsage");
118+
certManagerHarness.verifyExtensions(
119+
_extensions(bytes.concat(_basicConstraintsExtension(), _keyUsageExtension(), _keyUsageExtension())), true
120+
);
121+
}
122+
123+
function test_ExtensionsRejectTrailingExtensionFields() public {
124+
vm.expectRevert("trailing extension fields");
125+
certManagerHarness.verifyExtensions(
126+
_extensions(bytes.concat(_basicConstraintsExtensionWithTrailingField(), _keyUsageExtension())), true
127+
);
128+
}
129+
130+
function test_ParseTbsRejectsTrailingSignedFields() public {
131+
bytes memory mutated = _appendTbsTrailingField(CB1);
132+
133+
vm.expectRevert("trailing tbs fields");
134+
certManagerHarness.parseTbs(mutated, true);
135+
}
136+
98137
// Cert chain from the 2026-04-02 ~15:35 UTC dev attestation that produced the live revert.
99138
// CB0 is the AWS Nitro root (keccak256(CB0) == CertManager.ROOT_CA_CERT_HASH, pinned in the
100139
// constructor), so the chain is verified starting from CB1.
@@ -212,6 +251,61 @@ contract CertManagerTest is Test {
212251
_writeDerLength(result, sigRoot, _addDelta(sigRoot.length(), delta));
213252
}
214253

254+
function _appendTbsTrailingField(bytes memory certificate) internal pure returns (bytes memory result) {
255+
Asn1Ptr root = certificate.root();
256+
Asn1Ptr tbsPtr = certificate.firstChildOf(root);
257+
bytes memory nullField = hex"0500";
258+
int256 delta = int256(nullField.length);
259+
result = _insertBytes(certificate, tbsPtr.content() + tbsPtr.length(), nullField);
260+
261+
_writeDerLength(result, root, _addDelta(root.length(), delta));
262+
_writeDerLength(result, tbsPtr, _addDelta(tbsPtr.length(), delta));
263+
}
264+
265+
function _insertBytes(bytes memory input, uint256 offset, bytes memory inserted)
266+
internal
267+
pure
268+
returns (bytes memory result)
269+
{
270+
result = new bytes(input.length + inserted.length);
271+
for (uint256 i = 0; i < offset; ++i) {
272+
result[i] = input[i];
273+
}
274+
for (uint256 i = 0; i < inserted.length; ++i) {
275+
result[offset + i] = inserted[i];
276+
}
277+
for (uint256 i = offset; i < input.length; ++i) {
278+
result[i + inserted.length] = input[i];
279+
}
280+
}
281+
282+
function _extensions(bytes memory extensionList) internal pure returns (bytes memory) {
283+
return _derNode(0xa3, _derNode(0x30, extensionList));
284+
}
285+
286+
function _basicConstraintsExtension() internal pure returns (bytes memory) {
287+
return _derNode(0x30, bytes.concat(hex"0603551d13", hex"0101ff", _derNode(0x04, hex"30060101ff020100")));
288+
}
289+
290+
function _basicConstraintsExtensionWithTrailingField() internal pure returns (bytes memory) {
291+
return
292+
_derNode(0x30, bytes.concat(hex"0603551d13", hex"0101ff", _derNode(0x04, hex"30060101ff020100"), hex"0500"));
293+
}
294+
295+
function _keyUsageExtension() internal pure returns (bytes memory) {
296+
return _derNode(0x30, bytes.concat(hex"0603551d0f", hex"0101ff", _derNode(0x04, hex"03020186")));
297+
}
298+
299+
function _derNode(bytes1 tag, bytes memory content) internal pure returns (bytes memory der) {
300+
require(content.length < 128, "test: long-form length not supported");
301+
der = new bytes(2 + content.length);
302+
der[0] = tag;
303+
der[1] = bytes1(uint8(content.length));
304+
for (uint256 i = 0; i < content.length; ++i) {
305+
der[2 + i] = content[i];
306+
}
307+
}
308+
215309
function _certSignaturePtrs(bytes memory certificate)
216310
internal
217311
pure

0 commit comments

Comments
 (0)