Skip to content

Commit 05294ec

Browse files
leopoldjoyOpenCode
leopoldjoy
and
OpenCode
authored
fix: reject ambiguous cert field consumption (#54)
* fix: reject ambiguous cert field consumption Co-authored-by: OpenCode <opencode-noreply@coinbase.com> * refactor: centralize bounded ASN.1 traversal Co-authored-by: OpenCode <opencode-noreply@coinbase.com> * fix: require extension sequence to fill wrapper Co-authored-by: OpenCode <opencode-noreply@coinbase.com> --------- Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
1 parent fe7ef73 commit 05294ec

3 files changed

Lines changed: 119 additions & 30 deletions

File tree

‎src/CertManager.sol‎

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ contract CertManager is ICertManager {
2121
error InvalidExtension();
2222
error InvalidBasicConstraints();
2323
error InvalidSubjectPublicKey();
24+
error InvalidCertAlgorithm();
25+
error InvalidCertVersion();
2426
error UnsupportedCriticalExtension();
2527
error NotOwner();
2628
error NotRevoker();
@@ -306,7 +308,7 @@ contract CertManager is ICertManager {
306308

307309
Asn1Ptr root = certificate.root();
308310
_requireAsn1Tag(certificate, root, 0x30);
309-
require(root.totalLength() == certificate.length, "invalid cert length");
311+
if (root.totalLength() != certificate.length) revert Asn1Decode.InvalidAsn1Length();
310312
Asn1Ptr tbsCertPtr = certificate.firstChildOf(root);
311313
_requireAsn1Tag(certificate, tbsCertPtr, 0x30);
312314
return certificate.keccak(tbsCertPtr.header(), tbsCertPtr.totalLength());
@@ -329,7 +331,7 @@ contract CertManager is ICertManager {
329331
bytes memory signatureHints
330332
) internal view returns (VerifiedCert memory cert, bytes32 identity) {
331333
Asn1Ptr root = certificate.root();
332-
require(root.totalLength() == certificate.length, "invalid cert length");
334+
if (root.totalLength() != certificate.length) revert Asn1Decode.InvalidAsn1Length();
333335
Asn1Ptr tbsCertPtr = certificate.firstChildOf(root);
334336
(uint64 notAfter, int64 maxPathLen, bytes32 issuerHash, bytes32 subjectHash, bytes memory pubKey) =
335337
_parseTbs(certificate, tbsCertPtr, ca);
@@ -373,20 +375,20 @@ contract CertManager is ICertManager {
373375
_requireAsn1Tag(certificate, ptr, 0x30);
374376
Asn1Ptr versionPtr = certificate.firstChildOf(ptr);
375377
_requireAsn1Tag(certificate, versionPtr, 0xa0);
376-
Asn1Ptr vPtr = certificate.firstChildOf(versionPtr);
377-
Asn1Ptr serialPtr = certificate.nextSiblingOf(versionPtr);
378-
Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(serialPtr);
378+
Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(certificate.nextSiblingOf(versionPtr));
379379
_requireAsn1Tag(certificate, sigAlgoPtr, 0x30);
380380

381-
require(certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) == CERT_ALGO_OID, "invalid cert sig algo");
382-
uint256 version = certificate.uintAt(vPtr);
381+
if (certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) != CERT_ALGO_OID) {
382+
revert InvalidCertAlgorithm();
383+
}
383384
// 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
384-
require(version == 2, "version should be 3");
385+
if (certificate.uintAt(certificate.firstChildOf(versionPtr)) != 2) revert InvalidCertVersion();
385386

386-
(notAfter, maxPathLen, issuerHash, subjectHash, pubKey) = _parseTbsInner(certificate, sigAlgoPtr, ca);
387+
(notAfter, maxPathLen, issuerHash, subjectHash, pubKey) =
388+
_parseTbsInner(certificate, sigAlgoPtr, ca, ptr.content() + ptr.length());
387389
}
388390

389-
function _parseTbsInner(bytes memory certificate, Asn1Ptr sigAlgoPtr, bool ca)
391+
function _parseTbsInner(bytes memory certificate, Asn1Ptr sigAlgoPtr, bool ca, uint256 tbsEnd)
390392
internal
391393
view
392394
returns (uint64 notAfter, int64 maxPathLen, bytes32 issuerHash, bytes32 subjectHash, bytes memory pubKey)
@@ -411,6 +413,7 @@ contract CertManager is ICertManager {
411413
// skip optional subjectUniqueID
412414
extensionsPtr = certificate.nextSiblingOf(extensionsPtr);
413415
}
416+
if (extensionsPtr.content() + extensionsPtr.length() != tbsEnd) revert Asn1Decode.InvalidAsn1Length();
414417

415418
notAfter = _verifyValidity(certificate, validityPtr);
416419
maxPathLen = _verifyExtensions(certificate, extensionsPtr, ca);
@@ -465,40 +468,48 @@ contract CertManager is ICertManager {
465468
returns (int64 maxPathLen)
466469
{
467470
if (certificate[extensionsPtr.header()] != 0xa3) revert InvalidExtension();
471+
uint256 end = extensionsPtr.content() + extensionsPtr.length();
468472
extensionsPtr = certificate.firstChildOf(extensionsPtr);
469473
_requireAsn1Tag(certificate, extensionsPtr, 0x30);
474+
if (extensionsPtr.content() + extensionsPtr.length() != end) revert InvalidExtension();
470475
Asn1Ptr extensionPtr = certificate.firstChildOf(extensionsPtr);
471-
uint256 end = extensionsPtr.content() + extensionsPtr.length();
472476
bool basicConstraintsFound = false;
473477
bool keyUsageFound = false;
474478
maxPathLen = -1;
475479

476480
while (true) {
481+
uint256 extensionEnd = extensionPtr.content() + extensionPtr.length();
482+
if (extensionEnd > end) revert InvalidExtension();
477483
_requireAsn1Tag(certificate, extensionPtr, 0x30);
478484
Asn1Ptr oidPtr = certificate.firstChildOf(extensionPtr);
479485
bytes32 oid = certificate.keccak(oidPtr.content(), oidPtr.length());
480-
Asn1Ptr valuePtr = certificate.nextSiblingOf(oidPtr);
481486
bool recognized = oid == BASIC_CONSTRAINTS_OID || oid == KEY_USAGE_OID;
482487

488+
Asn1Ptr valuePtr = certificate.nextSiblingOf(oidPtr);
489+
483490
if (certificate[valuePtr.header()] == 0x01) {
484491
if (valuePtr.length() != 1) revert InvalidExtension();
485492
if (!recognized && certificate[valuePtr.content()] != 0x00) revert UnsupportedCriticalExtension();
486493
valuePtr = certificate.nextSiblingOf(valuePtr);
487494
}
488495

496+
if (valuePtr.content() + valuePtr.length() != extensionEnd) revert InvalidExtension();
497+
489498
if (recognized) {
490499
valuePtr = certificate.octetString(valuePtr);
491500

492501
if (oid == BASIC_CONSTRAINTS_OID) {
502+
if (basicConstraintsFound) revert InvalidExtension();
493503
basicConstraintsFound = true;
494504
maxPathLen = _verifyBasicConstraintsExtension(certificate, valuePtr, ca);
495505
} else {
506+
if (keyUsageFound) revert InvalidExtension();
496507
keyUsageFound = true;
497508
_verifyKeyUsageExtension(certificate, valuePtr, ca);
498509
}
499510
}
500511

501-
if (extensionPtr.content() + extensionPtr.length() == end) {
512+
if (extensionEnd == end) {
502513
break;
503514
}
504515
extensionPtr = certificate.nextSiblingOf(extensionPtr);
@@ -562,9 +573,9 @@ contract CertManager is ICertManager {
562573
// X.509 KeyUsage bits are MSB-first. bitstringUintAt keeps the first KeyUsage octet in the
563574
// low byte, so these masks continue to target the same bits for one- or multi-octet values.
564575
if (ca) {
565-
require(value & 0x04 == 0x04, "CertSign must be present");
576+
if (value & 0x04 != 0x04) revert InvalidExtension();
566577
} else {
567-
require(value & 0x80 == 0x80, "DigitalSignature must be present");
578+
if (value & 0x80 != 0x80) revert InvalidExtension();
568579
}
569580
}
570581

@@ -576,9 +587,11 @@ contract CertManager is ICertManager {
576587
) internal view {
577588
Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(ptr);
578589
_requireAsn1Tag(certificate, sigAlgoPtr, 0x30);
579-
require(certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) == CERT_ALGO_OID, "invalid cert sig algo");
590+
if (certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) != CERT_ALGO_OID) {
591+
revert InvalidCertAlgorithm();
592+
}
580593
Asn1Ptr sigPtr = certificate.nextSiblingOf(sigAlgoPtr);
581-
require(sigPtr.header() + sigPtr.totalLength() == certificate.length, "trailing cert fields");
594+
if (sigPtr.header() + sigPtr.totalLength() != certificate.length) revert Asn1Decode.InvalidAsn1Length();
582595

583596
bytes memory hash = Sha2Ext.sha384(certificate, ptr.header(), ptr.totalLength());
584597
bytes memory sigPacked = _certSignature(certificate, sigPtr);

‎test/CertManager.t.sol‎

Lines changed: 86 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -40,14 +40,6 @@ contract CertManagerPubKeyHarness is CertManager {
4040
function parsePubKey(bytes memory subjectPublicKeyInfo) external pure returns (bytes memory) {
4141
return _parsePubKey(subjectPublicKeyInfo, subjectPublicKeyInfo.root());
4242
}
43-
44-
function parsePubKeyAt(bytes memory certificate, uint256 header, uint256 content, uint256 length)
45-
external
46-
pure
47-
returns (bytes memory)
48-
{
49-
return _parsePubKey(certificate, LibAsn1Ptr.toAsn1Ptr(header, content, length));
50-
}
5143
}
5244

5345
contract CertManagerExtensionsHarness is CertManager {
@@ -127,6 +119,53 @@ contract CertManagerTest is Test {
127119
certManagerHarness.verifyBasicConstraints(hex"30020400", false);
128120
}
129121

122+
function test_ExtensionsRejectDuplicateBasicConstraints() public {
123+
vm.expectRevert(CertManager.InvalidExtension.selector);
124+
certManagerExtensionsHarness.verifyExtensions(
125+
_extensions(bytes.concat(_basicConstraintsExtension(), _basicConstraintsExtension(), _keyUsageExtension())),
126+
true
127+
);
128+
}
129+
130+
function test_ExtensionsRejectDuplicateKeyUsage() public {
131+
vm.expectRevert(CertManager.InvalidExtension.selector);
132+
certManagerExtensionsHarness.verifyExtensions(
133+
_extensions(bytes.concat(_basicConstraintsExtension(), _keyUsageExtension(), _keyUsageExtension())), true
134+
);
135+
}
136+
137+
function test_ExtensionsRejectTrailingExtensionFields() public {
138+
vm.expectRevert(CertManager.InvalidExtension.selector);
139+
certManagerExtensionsHarness.verifyExtensions(
140+
_extensions(bytes.concat(_basicConstraintsExtensionWithTrailingField(), _keyUsageExtension())), true
141+
);
142+
}
143+
144+
function test_ExtensionsRejectTrailingWrapperFields() public {
145+
bytes memory inner = _derNode(0x30, bytes.concat(_basicConstraintsExtension(), _keyUsageExtension()));
146+
147+
vm.expectRevert(CertManager.InvalidExtension.selector);
148+
certManagerExtensionsHarness.verifyExtensions(_derNode(0xa3, bytes.concat(inner, hex"0500")), true);
149+
}
150+
151+
function test_ExtensionsRejectInnerSequencePastWrapper() public {
152+
bytes memory inner = _derNode(0x30, bytes.concat(_basicConstraintsExtension(), _keyUsageExtension()));
153+
bytes memory der = bytes.concat(bytes1(0xa3), bytes1(uint8(inner.length - 1)), inner);
154+
155+
vm.expectRevert(CertManager.InvalidExtension.selector);
156+
certManagerExtensionsHarness.verifyExtensions(der, true);
157+
}
158+
159+
function test_ParseTbsRejectsTrailingSignedFields() public {
160+
vm.warp(1775145600);
161+
CertManager cm = new CertManager(new P384Verifier());
162+
163+
bytes memory mutated = _appendTbsTrailingField(CB1);
164+
165+
vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector);
166+
cm.verifyCACertWithHints(mutated, keccak256(CB0), "");
167+
}
168+
130169
function test_ParsePubKeyAcceptsUncompressedP384Point() public view {
131170
bytes memory pubKey = _patternBytes(96);
132171
bytes memory spki = abi.encodePacked(hex"3076301006072a8648ce3d020106052b8104002203620004", pubKey);
@@ -137,10 +176,9 @@ contract CertManagerTest is Test {
137176
function test_ParsePubKeyRejectsCompressedP384Point() public {
138177
bytes memory compressedKey = _patternBytes(48);
139178
bytes memory spki = abi.encodePacked(hex"3046301006072a8648ce3d020106052b8104002203320002", compressedKey);
140-
bytes memory paddedCertificate = abi.encodePacked(new bytes(128), spki);
141179

142180
vm.expectRevert(CertManager.InvalidSubjectPublicKey.selector);
143-
certManagerPubKeyHarness.parsePubKeyAt(paddedCertificate, 128, 130, 0x46);
181+
certManagerPubKeyHarness.parsePubKey(spki);
144182
}
145183

146184
function test_ParsePubKeyRejectsOversizedP384Point() public {
@@ -370,6 +408,17 @@ contract CertManagerTest is Test {
370408
_writeDerLength(result, sigRoot, _addDelta(sigRoot.length(), delta));
371409
}
372410

411+
function _appendTbsTrailingField(bytes memory certificate) internal pure returns (bytes memory result) {
412+
Asn1Ptr root = certificate.root();
413+
Asn1Ptr tbsPtr = certificate.firstChildOf(root);
414+
bytes memory nullField = hex"0500";
415+
int256 delta = int256(nullField.length);
416+
result = _insertBytes(certificate, tbsPtr.content() + tbsPtr.length(), nullField);
417+
418+
_writeDerLength(result, root, _addDelta(root.length(), delta));
419+
_writeDerLength(result, tbsPtr, _addDelta(tbsPtr.length(), delta));
420+
}
421+
373422
function _appendSignatureTrailingField(bytes memory certificate) internal pure returns (bytes memory result) {
374423
(Asn1Ptr root, Asn1Ptr sigPtr, Asn1Ptr sigRoot,) = _certSignaturePtrs(certificate);
375424
bytes memory extraInteger = hex"020100";
@@ -398,6 +447,33 @@ contract CertManagerTest is Test {
398447
}
399448
}
400449

450+
function _extensions(bytes memory extensionList) internal pure returns (bytes memory) {
451+
return _derNode(0xa3, _derNode(0x30, extensionList));
452+
}
453+
454+
function _basicConstraintsExtension() internal pure returns (bytes memory) {
455+
return _derNode(0x30, bytes.concat(hex"0603551d13", hex"0101ff", _derNode(0x04, hex"30060101ff020100")));
456+
}
457+
458+
function _basicConstraintsExtensionWithTrailingField() internal pure returns (bytes memory) {
459+
return
460+
_derNode(0x30, bytes.concat(hex"0603551d13", hex"0101ff", _derNode(0x04, hex"30060101ff020100"), hex"0500"));
461+
}
462+
463+
function _keyUsageExtension() internal pure returns (bytes memory) {
464+
return _derNode(0x30, bytes.concat(hex"0603551d0f", hex"0101ff", _derNode(0x04, hex"03020186")));
465+
}
466+
467+
function _derNode(bytes1 tag, bytes memory content) internal pure returns (bytes memory der) {
468+
require(content.length < 128, "test: long-form length not supported");
469+
der = new bytes(2 + content.length);
470+
der[0] = tag;
471+
der[1] = bytes1(uint8(content.length));
472+
for (uint256 i = 0; i < content.length; ++i) {
473+
der[2 + i] = content[i];
474+
}
475+
}
476+
401477
function _certSignaturePtrs(bytes memory certificate)
402478
internal
403479
pure

‎test/hinted/HintedNitroAttestation.t.sol‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,7 @@ contract HintedNitroAttestationTest is Test {
479479
NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs);
480480
(bytes memory caCert, bytes32 parentHash,) = _firstNonRootCA(attestationTbs, ptrs);
481481

482-
vm.expectRevert("invalid cert length");
482+
vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector);
483483
certManager.verifyCACertWithHints(abi.encodePacked(caCert, bytes1(0x00)), parentHash, "");
484484
}
485485

@@ -511,7 +511,7 @@ contract HintedNitroAttestationTest is Test {
511511
bytes memory shadowRootHints =
512512
hintCollector.collectCertSignatureHints(shadowRoot, certManager.loadVerified(rootHash).pubKey);
513513

514-
vm.expectRevert("invalid cert length");
514+
vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector);
515515
certManager.verifyCACertWithHints(shadowRoot, rootHash, shadowRootHints);
516516
assertEq(certManager.loadVerified(shadowRootHash).pubKey.length, 0, "shadow root must not cache");
517517

@@ -525,7 +525,7 @@ contract HintedNitroAttestationTest is Test {
525525
NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs);
526526
(bytes memory caCert, bytes32 parentHash,) = _firstNonRootCA(attestationTbs, ptrs);
527527

528-
vm.expectRevert("trailing cert fields");
528+
vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector);
529529
certManager.verifyCACertWithHints(_appendInsideOuterSequence(caCert, bytes1(0x00)), parentHash, "");
530530
}
531531

0 commit comments

Comments
 (0)