Skip to content

Commit e99197b

Browse files
leopoldjoyOpenCode
leopoldjoy
and
OpenCode
committed
Merge origin/main into strict cert field consumption
Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
2 parents 95247de + 338bfd2 commit e99197b

2 files changed

Lines changed: 91 additions & 35 deletions

File tree

‎src/CertManager.sol‎

Lines changed: 47 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,12 @@ contract CertManager is ICertManager {
1717
using LibAsn1Ptr for Asn1Ptr;
1818
using LibBytes for bytes;
1919

20+
error InvalidAsn1Tag();
2021
error InvalidExtension();
2122
error InvalidBasicConstraints();
2223
error InvalidSubjectPublicKey();
24+
error InvalidCertAlgorithm();
25+
error InvalidCertVersion();
2326
error UnsupportedCriticalExtension();
2427
error NotOwner();
2528
error NotRevoker();
@@ -174,7 +177,10 @@ contract CertManager is ICertManager {
174177
/// {revokeCerts}; the same value is recorded on-chain when the cert is first verified, so a
175178
/// revocation applies to every byte-encoding of that certificate. Reverts on malformed DER.
176179
function computeCertId(bytes memory cert) external pure returns (bytes32) {
177-
Asn1Ptr tbsCertPtr = cert.firstChildOf(cert.root());
180+
Asn1Ptr root = cert.root();
181+
_requireAsn1Tag(cert, root, 0x30);
182+
Asn1Ptr tbsCertPtr = cert.firstChildOf(root);
183+
_requireAsn1Tag(cert, tbsCertPtr, 0x30);
178184
return _certIdentity(cert, tbsCertPtr);
179185
}
180186

@@ -301,11 +307,17 @@ contract CertManager is ICertManager {
301307
}
302308

303309
Asn1Ptr root = certificate.root();
310+
_requireAsn1Tag(certificate, root, 0x30);
304311
require(root.totalLength() == certificate.length, "invalid cert length");
305312
Asn1Ptr tbsCertPtr = certificate.firstChildOf(root);
313+
_requireAsn1Tag(certificate, tbsCertPtr, 0x30);
306314
return certificate.keccak(tbsCertPtr.header(), tbsCertPtr.totalLength());
307315
}
308316

317+
function _requireAsn1Tag(bytes memory der, Asn1Ptr ptr, bytes1 tag) internal pure {
318+
if (der[ptr.header()] != tag) revert InvalidAsn1Tag();
319+
}
320+
309321
function _isPinnedRootAlias(bytes32 certHash, VerifiedCert memory cert) internal pure returns (bool) {
310322
return certHash != ROOT_CA_CERT_HASH && cert.ca && cert.subjectHash == ROOT_CA_CERT_SUBJECT_HASH
311323
&& cert.pubKey.length == ROOT_CA_CERT_PUB_KEY.length
@@ -367,38 +379,48 @@ contract CertManager is ICertManager {
367379
}
368380

369381
function _verifyTbsHeader(bytes memory certificate, Asn1Ptr ptr) internal pure returns (Asn1Ptr sigAlgoPtr) {
382+
_requireAsn1Tag(certificate, ptr, 0x30);
370383
Asn1Ptr versionPtr = certificate.firstChildOf(ptr);
384+
_requireAsn1Tag(certificate, versionPtr, 0xa0);
371385
Asn1Ptr vPtr = certificate.firstChildOf(versionPtr);
372-
sigAlgoPtr = certificate.nextSiblingOf(certificate.nextSiblingOf(versionPtr));
386+
Asn1Ptr serialPtr = certificate.nextSiblingOf(versionPtr);
387+
sigAlgoPtr = certificate.nextSiblingOf(serialPtr);
388+
_requireAsn1Tag(certificate, sigAlgoPtr, 0x30);
373389

374-
require(certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) == CERT_ALGO_OID, "invalid cert sig algo");
390+
if (certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) != CERT_ALGO_OID) {
391+
revert InvalidCertAlgorithm();
392+
}
375393
uint256 version = certificate.uintAt(vPtr);
376394
// 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
377-
require(version == 2, "version should be 3");
395+
if (version != 2) revert InvalidCertVersion();
378396
}
379397

380398
function _parseTbsInner(bytes memory certificate, Asn1Ptr sigAlgoPtr, bool ca, uint256 tbsEnd)
381399
internal
382400
view
383401
returns (uint64 notAfter, int64 maxPathLen, bytes32 issuerHash, bytes32 subjectHash, bytes memory pubKey)
384402
{
385-
Asn1Ptr issuerPtr = _nextSiblingWithin(certificate, sigAlgoPtr, tbsEnd);
403+
Asn1Ptr issuerPtr = certificate.nextSiblingOf(sigAlgoPtr);
404+
_requireAsn1Tag(certificate, issuerPtr, 0x30);
386405
issuerHash = certificate.keccak(issuerPtr.content(), issuerPtr.length());
387-
Asn1Ptr validityPtr = _nextSiblingWithin(certificate, issuerPtr, tbsEnd);
388-
Asn1Ptr subjectPtr = _nextSiblingWithin(certificate, validityPtr, tbsEnd);
406+
Asn1Ptr validityPtr = certificate.nextSiblingOf(issuerPtr);
407+
_requireAsn1Tag(certificate, validityPtr, 0x30);
408+
Asn1Ptr subjectPtr = certificate.nextSiblingOf(validityPtr);
409+
_requireAsn1Tag(certificate, subjectPtr, 0x30);
389410
subjectHash = certificate.keccak(subjectPtr.content(), subjectPtr.length());
390-
Asn1Ptr subjectPublicKeyInfoPtr = _nextSiblingWithin(certificate, subjectPtr, tbsEnd);
391-
Asn1Ptr extensionsPtr = _nextSiblingWithin(certificate, subjectPublicKeyInfoPtr, tbsEnd);
411+
Asn1Ptr subjectPublicKeyInfoPtr = certificate.nextSiblingOf(subjectPtr);
412+
_requireAsn1Tag(certificate, subjectPublicKeyInfoPtr, 0x30);
413+
Asn1Ptr extensionsPtr = certificate.nextSiblingOf(subjectPublicKeyInfoPtr);
392414

393415
if (certificate[extensionsPtr.header()] == 0x81) {
394416
// skip optional issuerUniqueID
395-
extensionsPtr = _nextSiblingWithin(certificate, extensionsPtr, tbsEnd);
417+
extensionsPtr = certificate.nextSiblingOf(extensionsPtr);
396418
}
397419
if (certificate[extensionsPtr.header()] == 0x82) {
398420
// skip optional subjectUniqueID
399-
extensionsPtr = _nextSiblingWithin(certificate, extensionsPtr, tbsEnd);
421+
extensionsPtr = certificate.nextSiblingOf(extensionsPtr);
400422
}
401-
require(_requireAsn1NodeWithin(extensionsPtr, tbsEnd) == tbsEnd, "trailing tbs fields");
423+
if (_requireAsn1NodeWithin(extensionsPtr, tbsEnd) != tbsEnd) revert Asn1Decode.InvalidAsn1Length();
402424

403425
notAfter = _verifyValidity(certificate, validityPtr);
404426
maxPathLen = _verifyExtensions(certificate, extensionsPtr, ca);
@@ -411,6 +433,7 @@ contract CertManager is ICertManager {
411433
returns (bytes memory subjectPubKey)
412434
{
413435
Asn1Ptr pubKeyAlgoPtr = certificate.firstChildOf(subjectPublicKeyInfoPtr);
436+
_requireAsn1Tag(certificate, pubKeyAlgoPtr, 0x30);
414437
Asn1Ptr pubKeyAlgoIdPtr = certificate.firstChildOf(pubKeyAlgoPtr);
415438
Asn1Ptr algoParamsPtr = certificate.nextSiblingOf(pubKeyAlgoIdPtr);
416439
Asn1Ptr subjectPublicKeyPtr = certificate.nextSiblingOf(pubKeyAlgoPtr);
@@ -453,15 +476,17 @@ contract CertManager is ICertManager {
453476
{
454477
if (certificate[extensionsPtr.header()] != 0xa3) revert InvalidExtension();
455478
extensionsPtr = certificate.firstChildOf(extensionsPtr);
479+
_requireAsn1Tag(certificate, extensionsPtr, 0x30);
456480
uint256 end = extensionsPtr.content() + extensionsPtr.length();
457-
Asn1Ptr extensionPtr = _firstChildWithin(certificate, extensionsPtr, end);
481+
Asn1Ptr extensionPtr = certificate.firstChildOf(extensionsPtr);
458482
bool basicConstraintsFound = false;
459483
bool keyUsageFound = false;
460484
maxPathLen = -1;
461485

462486
while (true) {
463487
uint256 extensionEnd = _requireAsn1NodeWithin(extensionPtr, end);
464-
Asn1Ptr oidPtr = _firstChildWithin(certificate, extensionPtr, extensionEnd);
488+
_requireAsn1Tag(certificate, extensionPtr, 0x30);
489+
Asn1Ptr oidPtr = certificate.firstChildOf(extensionPtr);
465490
bytes32 oid = certificate.keccak(oidPtr.content(), oidPtr.length());
466491
bool recognized = oid == BASIC_CONSTRAINTS_OID || oid == KEY_USAGE_OID;
467492

@@ -473,17 +498,17 @@ contract CertManager is ICertManager {
473498
valuePtr = _nextSiblingWithin(certificate, valuePtr, extensionEnd);
474499
}
475500

476-
require(_requireAsn1NodeWithin(valuePtr, extensionEnd) == extensionEnd, "trailing extension fields");
501+
if (_requireAsn1NodeWithin(valuePtr, extensionEnd) != extensionEnd) revert InvalidExtension();
477502

478503
if (recognized) {
479504
valuePtr = certificate.octetString(valuePtr);
480505

481506
if (oid == BASIC_CONSTRAINTS_OID) {
482-
require(!basicConstraintsFound, "duplicate basicConstraints");
507+
if (basicConstraintsFound) revert InvalidExtension();
483508
basicConstraintsFound = true;
484509
maxPathLen = _verifyBasicConstraintsExtension(certificate, valuePtr, ca);
485510
} else {
486-
require(!keyUsageFound, "duplicate keyUsage");
511+
if (keyUsageFound) revert InvalidExtension();
487512
keyUsageFound = true;
488513
_verifyKeyUsageExtension(certificate, valuePtr, ca);
489514
}
@@ -550,12 +575,7 @@ contract CertManager is ICertManager {
550575

551576
function _requireAsn1NodeWithin(Asn1Ptr ptr, uint256 parentEnd) internal pure returns (uint256 nodeEnd) {
552577
nodeEnd = ptr.header() + ptr.totalLength();
553-
require(nodeEnd <= parentEnd, "ASN.1 node out of bounds");
554-
}
555-
556-
function _firstChildWithin(bytes memory der, Asn1Ptr ptr, uint256 parentEnd) internal pure returns (Asn1Ptr child) {
557-
child = der.firstChildOf(ptr);
558-
_requireAsn1NodeWithin(child, parentEnd);
578+
if (nodeEnd > parentEnd) revert Asn1Decode.InvalidAsn1Length();
559579
}
560580

561581
function _nextSiblingWithin(bytes memory der, Asn1Ptr ptr, uint256 parentEnd)
@@ -585,7 +605,10 @@ contract CertManager is ICertManager {
585605
bytes memory signatureHints
586606
) internal view {
587607
Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(ptr);
588-
require(certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) == CERT_ALGO_OID, "invalid cert sig algo");
608+
_requireAsn1Tag(certificate, sigAlgoPtr, 0x30);
609+
if (certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) != CERT_ALGO_OID) {
610+
revert InvalidCertAlgorithm();
611+
}
589612
Asn1Ptr sigPtr = certificate.nextSiblingOf(sigAlgoPtr);
590613
require(sigPtr.header() + sigPtr.totalLength() == certificate.length, "trailing cert fields");
591614

‎test/CertManager.t.sol‎

Lines changed: 44 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,12 @@ 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+
}
3334

34-
function verifyExtensions(bytes memory der, bool ca) external pure returns (int64) {
35-
return _verifyExtensions(der, der.root(), ca);
36-
}
35+
contract CertManagerTbsHarness is CertManager {
36+
using Asn1Decode for bytes;
37+
38+
constructor() CertManager(new P384Verifier()) {}
3739

3840
function parseTbs(bytes memory cert, bool ca) external view {
3941
Asn1Ptr root = cert.root();
@@ -77,12 +79,14 @@ contract CertManagerTest is Test {
7779

7880
Asn1DecodeHarness public harness;
7981
CertManagerHarness public certManagerHarness;
82+
CertManagerTbsHarness public certManagerTbsHarness;
8083
CertManagerPubKeyHarness public certManagerPubKeyHarness;
8184
CertManagerExtensionsHarness public certManagerExtensionsHarness;
8285

8386
function setUp() public {
8487
harness = new Asn1DecodeHarness();
8588
certManagerHarness = new CertManagerHarness();
89+
certManagerTbsHarness = new CertManagerTbsHarness();
8690
certManagerPubKeyHarness = new CertManagerPubKeyHarness();
8791
certManagerExtensionsHarness = new CertManagerExtensionsHarness();
8892
}
@@ -138,32 +142,32 @@ contract CertManagerTest is Test {
138142
}
139143

140144
function test_ExtensionsRejectDuplicateBasicConstraints() public {
141-
vm.expectRevert("duplicate basicConstraints");
142-
certManagerHarness.verifyExtensions(
145+
vm.expectRevert(CertManager.InvalidExtension.selector);
146+
certManagerExtensionsHarness.verifyExtensions(
143147
_extensions(bytes.concat(_basicConstraintsExtension(), _basicConstraintsExtension(), _keyUsageExtension())),
144148
true
145149
);
146150
}
147151

148152
function test_ExtensionsRejectDuplicateKeyUsage() public {
149-
vm.expectRevert("duplicate keyUsage");
150-
certManagerHarness.verifyExtensions(
153+
vm.expectRevert(CertManager.InvalidExtension.selector);
154+
certManagerExtensionsHarness.verifyExtensions(
151155
_extensions(bytes.concat(_basicConstraintsExtension(), _keyUsageExtension(), _keyUsageExtension())), true
152156
);
153157
}
154158

155159
function test_ExtensionsRejectTrailingExtensionFields() public {
156-
vm.expectRevert("trailing extension fields");
157-
certManagerHarness.verifyExtensions(
160+
vm.expectRevert(CertManager.InvalidExtension.selector);
161+
certManagerExtensionsHarness.verifyExtensions(
158162
_extensions(bytes.concat(_basicConstraintsExtensionWithTrailingField(), _keyUsageExtension())), true
159163
);
160164
}
161165

162166
function test_ParseTbsRejectsTrailingSignedFields() public {
163167
bytes memory mutated = _appendTbsTrailingField(CB1);
164168

165-
vm.expectRevert("trailing tbs fields");
166-
certManagerHarness.parseTbs(mutated, true);
169+
vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector);
170+
certManagerTbsHarness.parseTbs(mutated, true);
167171
}
168172

169173
function test_ParsePubKeyAcceptsUncompressedP384Point() public view {
@@ -302,6 +306,35 @@ contract CertManagerTest is Test {
302306
cm.verifyCACertWithHints(rootTwin, rootHash, hints);
303307
}
304308

309+
function test_VerifyCACertWithHints_RejectsOuterTagSubstitution() public {
310+
vm.warp(1775145600);
311+
CertManager cm = new CertManager(new P384Verifier());
312+
313+
bytes32 rootHash = keccak256(CB0);
314+
bytes memory mutated = bytes.concat(CB1);
315+
mutated[0] = 0x31; // constructed SET with the same children is not an X.509 Certificate SEQUENCE.
316+
317+
vm.expectRevert(CertManager.InvalidAsn1Tag.selector);
318+
cm.verifyCACertWithHints(mutated, rootHash, "");
319+
}
320+
321+
function test_VerifyCACertWithHints_RejectsTbsAlgorithmTagSubstitution() public {
322+
vm.warp(1775145600);
323+
CertManager cm = new CertManager(new P384Verifier());
324+
325+
bytes32 rootHash = keccak256(CB0);
326+
bytes memory mutated = bytes.concat(CB1);
327+
Asn1Ptr root = mutated.root();
328+
Asn1Ptr tbsPtr = mutated.firstChildOf(root);
329+
Asn1Ptr versionPtr = mutated.firstChildOf(tbsPtr);
330+
Asn1Ptr serialPtr = mutated.nextSiblingOf(versionPtr);
331+
Asn1Ptr sigAlgoPtr = mutated.nextSiblingOf(serialPtr);
332+
mutated[sigAlgoPtr.header()] = 0x31; // constructed, but not AlgorithmIdentifier SEQUENCE.
333+
334+
vm.expectRevert(CertManager.InvalidAsn1Tag.selector);
335+
cm.verifyCACertWithHints(mutated, rootHash, "");
336+
}
337+
305338
function _verifyCA(CertManager cm, P384HintCollector collector, bytes memory cert, bytes32 parentHash)
306339
internal
307340
returns (bytes32)

0 commit comments

Comments
 (0)