Skip to content

Commit bae331a

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 e99197b + c504bd1 commit bae331a

5 files changed

Lines changed: 164 additions & 56 deletions

File tree

‎src/Asn1Decode.sol‎

Lines changed: 33 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -170,10 +170,8 @@ library Asn1Decode {
170170
* @return Uint value of node
171171
*/
172172
function uintAt(bytes memory der, Asn1Ptr ptr) internal pure returns (uint256) {
173-
if (der[ptr.header()] != 0x02) revert InvalidAsn1Type();
174-
if (der[ptr.content()] & 0x80 != 0) revert InvalidAsn1Value();
175-
uint256 len = ptr.length();
176-
return uint256(readBytesN(der, ptr.content(), len) >> (32 - len) * 8);
173+
(uint256 start, uint256 len) = positiveIntegerContent(der, ptr, 32);
174+
return uint256(readBytesN(der, start, len) >> (32 - len) * 8);
177175
}
178176

179177
/*
@@ -183,19 +181,40 @@ library Asn1Decode {
183181
* @return 384-bit uint encoded in uint128 and uint256
184182
*/
185183
function uint384At(bytes memory der, Asn1Ptr ptr) internal pure returns (uint128, uint256) {
184+
(uint256 start, uint256 valueLength) = positiveIntegerContent(der, ptr, 48);
185+
186+
if (valueLength > 32) {
187+
uint256 hiLen = valueLength - 32;
188+
return (
189+
uint128(uint256(readBytesN(der, start, hiLen) >> (32 - hiLen) * 8)),
190+
uint256(readBytesN(der, start + hiLen, 32))
191+
);
192+
}
193+
194+
return (0, uint256(readBytesN(der, start, valueLength) >> (32 - valueLength) * 8));
195+
}
196+
197+
function positiveIntegerContent(bytes memory der, Asn1Ptr ptr, uint256 maxValueLength)
198+
private
199+
pure
200+
returns (uint256 start, uint256 valueLength)
201+
{
186202
if (der[ptr.header()] != 0x02) revert InvalidAsn1Type();
187-
if (der[ptr.content()] & 0x80 != 0) revert InvalidAsn1Value();
188-
uint256 valueLength = ptr.length();
189-
uint256 start = ptr.content();
203+
valueLength = ptr.length();
204+
if (valueLength == 0) revert InvalidAsn1Length();
205+
start = ptr.content();
206+
190207
if (der[start] == 0) {
191-
start++;
192-
valueLength--;
208+
if (valueLength > 1) {
209+
if (der[start + 1] & 0x80 != 0x80) revert InvalidAsn1Value();
210+
start++;
211+
valueLength--;
212+
}
213+
} else {
214+
if (der[start] & 0x80 != 0) revert InvalidAsn1Value();
193215
}
194-
uint256 shift = 48 - valueLength;
195-
return (
196-
uint128(uint256(readBytesN(der, start, 16 - shift) >> (128 + shift * 8))),
197-
uint256(readBytesN(der, start + 16 - shift, 32))
198-
);
216+
217+
if (valueLength > maxValueLength) revert InvalidAsn1Length();
199218
}
200219

201220
/*

‎src/CertManager.sol‎

Lines changed: 24 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,7 @@ contract CertManager is ICertManager {
308308

309309
Asn1Ptr root = certificate.root();
310310
_requireAsn1Tag(certificate, root, 0x30);
311-
require(root.totalLength() == certificate.length, "invalid cert length");
311+
if (root.totalLength() != certificate.length) revert Asn1Decode.InvalidAsn1Length();
312312
Asn1Ptr tbsCertPtr = certificate.firstChildOf(root);
313313
_requireAsn1Tag(certificate, tbsCertPtr, 0x30);
314314
return certificate.keccak(tbsCertPtr.header(), tbsCertPtr.totalLength());
@@ -331,7 +331,7 @@ contract CertManager is ICertManager {
331331
bytes memory signatureHints
332332
) internal view returns (VerifiedCert memory cert, bytes32 identity) {
333333
Asn1Ptr root = certificate.root();
334-
require(root.totalLength() == certificate.length, "invalid cert length");
334+
if (root.totalLength() != certificate.length) revert Asn1Decode.InvalidAsn1Length();
335335
Asn1Ptr tbsCertPtr = certificate.firstChildOf(root);
336336
(uint64 notAfter, int64 maxPathLen, bytes32 issuerHash, bytes32 subjectHash, bytes memory pubKey) =
337337
_parseTbs(certificate, tbsCertPtr, ca);
@@ -372,27 +372,20 @@ contract CertManager is ICertManager {
372372
view
373373
returns (uint64 notAfter, int64 maxPathLen, bytes32 issuerHash, bytes32 subjectHash, bytes memory pubKey)
374374
{
375-
Asn1Ptr sigAlgoPtr = _verifyTbsHeader(certificate, ptr);
376-
377-
(notAfter, maxPathLen, issuerHash, subjectHash, pubKey) =
378-
_parseTbsInner(certificate, sigAlgoPtr, ca, ptr.content() + ptr.length());
379-
}
380-
381-
function _verifyTbsHeader(bytes memory certificate, Asn1Ptr ptr) internal pure returns (Asn1Ptr sigAlgoPtr) {
382375
_requireAsn1Tag(certificate, ptr, 0x30);
383376
Asn1Ptr versionPtr = certificate.firstChildOf(ptr);
384377
_requireAsn1Tag(certificate, versionPtr, 0xa0);
385-
Asn1Ptr vPtr = certificate.firstChildOf(versionPtr);
386-
Asn1Ptr serialPtr = certificate.nextSiblingOf(versionPtr);
387-
sigAlgoPtr = certificate.nextSiblingOf(serialPtr);
378+
Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(certificate.nextSiblingOf(versionPtr));
388379
_requireAsn1Tag(certificate, sigAlgoPtr, 0x30);
389380

390381
if (certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) != CERT_ALGO_OID) {
391382
revert InvalidCertAlgorithm();
392383
}
393-
uint256 version = certificate.uintAt(vPtr);
394384
// 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
395-
if (version != 2) revert InvalidCertVersion();
385+
if (certificate.uintAt(certificate.firstChildOf(versionPtr)) != 2) revert InvalidCertVersion();
386+
387+
(notAfter, maxPathLen, issuerHash, subjectHash, pubKey) =
388+
_parseTbsInner(certificate, sigAlgoPtr, ca, ptr.content() + ptr.length());
396389
}
397390

398391
function _parseTbsInner(bytes memory certificate, Asn1Ptr sigAlgoPtr, bool ca, uint256 tbsEnd)
@@ -420,7 +413,7 @@ contract CertManager is ICertManager {
420413
// skip optional subjectUniqueID
421414
extensionsPtr = certificate.nextSiblingOf(extensionsPtr);
422415
}
423-
if (_requireAsn1NodeWithin(extensionsPtr, tbsEnd) != tbsEnd) revert Asn1Decode.InvalidAsn1Length();
416+
if (extensionsPtr.content() + extensionsPtr.length() != tbsEnd) revert Asn1Decode.InvalidAsn1Length();
424417

425418
notAfter = _verifyValidity(certificate, validityPtr);
426419
maxPathLen = _verifyExtensions(certificate, extensionsPtr, ca);
@@ -484,21 +477,22 @@ contract CertManager is ICertManager {
484477
maxPathLen = -1;
485478

486479
while (true) {
487-
uint256 extensionEnd = _requireAsn1NodeWithin(extensionPtr, end);
480+
uint256 extensionEnd = extensionPtr.content() + extensionPtr.length();
481+
if (extensionEnd > end) revert InvalidExtension();
488482
_requireAsn1Tag(certificate, extensionPtr, 0x30);
489483
Asn1Ptr oidPtr = certificate.firstChildOf(extensionPtr);
490484
bytes32 oid = certificate.keccak(oidPtr.content(), oidPtr.length());
491485
bool recognized = oid == BASIC_CONSTRAINTS_OID || oid == KEY_USAGE_OID;
492486

493-
Asn1Ptr valuePtr = _nextSiblingWithin(certificate, oidPtr, extensionEnd);
487+
Asn1Ptr valuePtr = certificate.nextSiblingOf(oidPtr);
494488

495489
if (certificate[valuePtr.header()] == 0x01) {
496490
if (valuePtr.length() != 1) revert InvalidExtension();
497491
if (!recognized && certificate[valuePtr.content()] != 0x00) revert UnsupportedCriticalExtension();
498-
valuePtr = _nextSiblingWithin(certificate, valuePtr, extensionEnd);
492+
valuePtr = certificate.nextSiblingOf(valuePtr);
499493
}
500494

501-
if (_requireAsn1NodeWithin(valuePtr, extensionEnd) != extensionEnd) revert InvalidExtension();
495+
if (valuePtr.content() + valuePtr.length() != extensionEnd) revert InvalidExtension();
502496

503497
if (recognized) {
504498
valuePtr = certificate.octetString(valuePtr);
@@ -517,7 +511,7 @@ contract CertManager is ICertManager {
517511
if (extensionEnd == end) {
518512
break;
519513
}
520-
extensionPtr = _nextSiblingWithin(certificate, extensionPtr, end);
514+
extensionPtr = certificate.nextSiblingOf(extensionPtr);
521515
}
522516

523517
if (!basicConstraintsFound || !keyUsageFound || (!ca && maxPathLen != -1)) revert InvalidExtension();
@@ -573,28 +567,14 @@ contract CertManager is ICertManager {
573567
if (childEnd > parentEnd) revert InvalidBasicConstraints();
574568
}
575569

576-
function _requireAsn1NodeWithin(Asn1Ptr ptr, uint256 parentEnd) internal pure returns (uint256 nodeEnd) {
577-
nodeEnd = ptr.header() + ptr.totalLength();
578-
if (nodeEnd > parentEnd) revert Asn1Decode.InvalidAsn1Length();
579-
}
580-
581-
function _nextSiblingWithin(bytes memory der, Asn1Ptr ptr, uint256 parentEnd)
582-
internal
583-
pure
584-
returns (Asn1Ptr sibling)
585-
{
586-
sibling = der.nextSiblingOf(ptr);
587-
_requireAsn1NodeWithin(sibling, parentEnd);
588-
}
589-
590570
function _verifyKeyUsageExtension(bytes memory certificate, Asn1Ptr valuePtr, bool ca) internal pure {
591571
uint256 value = certificate.bitstringUintAt(valuePtr);
592572
// X.509 KeyUsage bits are MSB-first. bitstringUintAt keeps the first KeyUsage octet in the
593573
// low byte, so these masks continue to target the same bits for one- or multi-octet values.
594574
if (ca) {
595-
require(value & 0x04 == 0x04, "CertSign must be present");
575+
if (value & 0x04 != 0x04) revert InvalidExtension();
596576
} else {
597-
require(value & 0x80 == 0x80, "DigitalSignature must be present");
577+
if (value & 0x80 != 0x80) revert InvalidExtension();
598578
}
599579
}
600580

@@ -610,7 +590,7 @@ contract CertManager is ICertManager {
610590
revert InvalidCertAlgorithm();
611591
}
612592
Asn1Ptr sigPtr = certificate.nextSiblingOf(sigAlgoPtr);
613-
require(sigPtr.header() + sigPtr.totalLength() == certificate.length, "trailing cert fields");
593+
if (sigPtr.header() + sigPtr.totalLength() != certificate.length) revert Asn1Decode.InvalidAsn1Length();
614594

615595
bytes memory hash = Sha2Ext.sha384(certificate, ptr.header(), ptr.totalLength());
616596
bytes memory sigPacked = _certSignature(certificate, sigPtr);
@@ -621,8 +601,15 @@ contract CertManager is ICertManager {
621601
function _certSignature(bytes memory certificate, Asn1Ptr sigPtr) internal pure returns (bytes memory sigPacked) {
622602
Asn1Ptr sigBPtr = certificate.bitstring(sigPtr);
623603
Asn1Ptr sigRoot = certificate.rootOf(sigBPtr);
604+
_requireAsn1Tag(certificate, sigRoot, 0x30);
624605
Asn1Ptr sigRPtr = certificate.firstChildOf(sigRoot);
625606
Asn1Ptr sigSPtr = certificate.nextSiblingOf(sigRPtr);
607+
if (
608+
sigRoot.header() + sigRoot.totalLength() != sigBPtr.content() + sigBPtr.length()
609+
|| sigSPtr.header() + sigSPtr.totalLength() != sigRoot.content() + sigRoot.length()
610+
) {
611+
revert InvalidAsn1Tag();
612+
}
626613
(uint128 rhi, uint256 rlo) = certificate.uint384At(sigRPtr);
627614
(uint128 shi, uint256 slo) = certificate.uint384At(sigSPtr);
628615
sigPacked = abi.encodePacked(rhi, rlo, shi, slo);

‎test/Asn1Decode.t.sol‎

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,10 @@ contract Asn1Harness {
2020
return der.uintAt(der.root());
2121
}
2222

23+
function uint384AtRoot(bytes memory der) external pure returns (uint128 hi, uint256 lo) {
24+
return der.uint384At(der.root());
25+
}
26+
2327
function timestampAtRoot(bytes memory der) external pure returns (uint256) {
2428
return der.timestampAt(der.root());
2529
}
@@ -90,6 +94,20 @@ contract Asn1DecodeTest is Test {
9094
assertEq(h.uintAtRoot(hex"0203012345"), 0x012345); // INTEGER 0x012345
9195
}
9296

97+
function test_uintAt_requiredLeadingZero() public view {
98+
assertEq(h.uintAtRoot(hex"02020080"), 0x80);
99+
}
100+
101+
function test_uintAt_unnecessaryLeadingZero_reverts() public {
102+
vm.expectRevert(Asn1Decode.InvalidAsn1Value.selector);
103+
h.uintAtRoot(hex"0202007f");
104+
}
105+
106+
function test_uintAt_empty_reverts() public {
107+
vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector);
108+
h.uintAtRoot(hex"0200");
109+
}
110+
93111
function test_uintAt_notInteger_reverts() public {
94112
vm.expectRevert(Asn1Decode.InvalidAsn1Type.selector);
95113
h.uintAtRoot(hex"0401ff"); // OCTET STRING, not INTEGER
@@ -106,6 +124,22 @@ contract Asn1DecodeTest is Test {
106124
h.uintAtRoot(hex"02050000"); // claims 5 content bytes, only 2 present
107125
}
108126

127+
function test_uint384At_requiredLeadingZero() public view {
128+
(uint128 hi, uint256 lo) = h.uint384AtRoot(hex"02020080");
129+
assertEq(hi, 0);
130+
assertEq(lo, 0x80);
131+
}
132+
133+
function test_uint384At_unnecessaryLeadingZero_reverts() public {
134+
vm.expectRevert(Asn1Decode.InvalidAsn1Value.selector);
135+
h.uint384AtRoot(hex"0202007f");
136+
}
137+
138+
function test_uint384At_empty_reverts() public {
139+
vm.expectRevert(Asn1Decode.InvalidAsn1Length.selector);
140+
h.uint384AtRoot(hex"0200");
141+
}
142+
109143
// --- timestampAt ---
110144

111145
function test_timestamp_utcEpoch() public view {
@@ -209,11 +243,36 @@ contract Asn1DecodeTest is Test {
209243
}
210244

211245
function testFuzz_uintAt_positive(uint64 v) public view {
212-
// INTEGER with an explicit 0x00 sign byte so the value is always positive
213-
bytes memory der = abi.encodePacked(bytes1(0x02), bytes1(0x09), bytes1(0x00), bytes8(v));
246+
bytes memory der = _derEncodeUint64(v);
214247
assertEq(h.uintAtRoot(der), v);
215248
}
216249

250+
function _derEncodeUint64(uint64 v) internal pure returns (bytes memory) {
251+
if (v == 0) {
252+
return hex"020100";
253+
}
254+
255+
bytes8 raw = bytes8(v);
256+
uint256 offset;
257+
while (offset < 8 && raw[offset] == 0) {
258+
offset++;
259+
}
260+
261+
uint256 len = 8 - offset;
262+
bool needsPad = uint8(raw[offset]) >= 0x80;
263+
bytes memory der = new bytes(2 + len + (needsPad ? 1 : 0));
264+
der[0] = 0x02;
265+
der[1] = bytes1(uint8(der.length - 2));
266+
uint256 dst = 2;
267+
if (needsPad) {
268+
der[dst++] = 0x00;
269+
}
270+
for (uint256 i = offset; i < 8; ++i) {
271+
der[dst++] = raw[i];
272+
}
273+
return der;
274+
}
275+
217276
function _utcTime(string memory s) internal pure returns (bytes memory) {
218277
bytes memory b = bytes(s);
219278
require(b.length == 13, "test: UTCTime must be 13 chars");

‎test/CertManager.t.sol‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,38 @@ contract CertManagerTest is Test {
306306
cm.verifyCACertWithHints(rootTwin, rootHash, hints);
307307
}
308308

309+
function test_VerifyCACertWithHints_RejectsSignatureWrapperTagSubstitution() public {
310+
vm.warp(1775145600);
311+
CertManager cm = new CertManager(new P384Verifier());
312+
P384HintCollector collector = new P384HintCollector();
313+
314+
bytes32 rootHash = keccak256(CB0);
315+
bytes memory parentPubKey = cm.loadVerified(rootHash).pubKey;
316+
bytes memory hints = collector.collectCertSignatureHints(CB1, parentPubKey);
317+
318+
bytes memory mutated = bytes.concat(CB1);
319+
(,, Asn1Ptr sigRoot,) = _certSignaturePtrs(mutated);
320+
mutated[sigRoot.header()] = 0x31; // constructed SET with the same r/s children.
321+
322+
vm.expectRevert(CertManager.InvalidAsn1Tag.selector);
323+
cm.verifyCACertWithHints(mutated, rootHash, hints);
324+
}
325+
326+
function test_VerifyCACertWithHints_RejectsTrailingSignatureFields() public {
327+
vm.warp(1775145600);
328+
CertManager cm = new CertManager(new P384Verifier());
329+
P384HintCollector collector = new P384HintCollector();
330+
331+
bytes32 rootHash = keccak256(CB0);
332+
bytes memory parentPubKey = cm.loadVerified(rootHash).pubKey;
333+
bytes memory hints = collector.collectCertSignatureHints(CB1, parentPubKey);
334+
335+
bytes memory mutated = _appendSignatureTrailingField(CB1);
336+
337+
vm.expectRevert(CertManager.InvalidAsn1Tag.selector);
338+
cm.verifyCACertWithHints(mutated, rootHash, hints);
339+
}
340+
309341
function test_VerifyCACertWithHints_RejectsOuterTagSubstitution() public {
310342
vm.warp(1775145600);
311343
CertManager cm = new CertManager(new P384Verifier());
@@ -392,6 +424,17 @@ contract CertManagerTest is Test {
392424
_writeDerLength(result, tbsPtr, _addDelta(tbsPtr.length(), delta));
393425
}
394426

427+
function _appendSignatureTrailingField(bytes memory certificate) internal pure returns (bytes memory result) {
428+
(Asn1Ptr root, Asn1Ptr sigPtr, Asn1Ptr sigRoot,) = _certSignaturePtrs(certificate);
429+
bytes memory extraInteger = hex"020100";
430+
int256 delta = int256(extraInteger.length);
431+
result = _insertBytes(certificate, sigRoot.content() + sigRoot.length(), extraInteger);
432+
433+
_writeDerLength(result, root, _addDelta(root.length(), delta));
434+
_writeDerLength(result, sigPtr, _addDelta(sigPtr.length(), delta));
435+
_writeDerLength(result, sigRoot, _addDelta(sigRoot.length(), delta));
436+
}
437+
395438
function _insertBytes(bytes memory input, uint256 offset, bytes memory inserted)
396439
internal
397440
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)