diff --git a/CHANGELOG.md b/CHANGELOG.md index 29b1b31..1dc2d37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ All notable changes to this project are documented here. The format is based on many certificates revoked, and the owner can rotate the revoker or undo accidental revocations. - `CertManager.computeCertId(certDER)`: returns a certificate's `(issuer, serial)` revocation identity key. +- `CborDecode.skipValue`: a generic CBOR data-item skipper used to walk over values of unknown shape. ### Changed - Certificate verification and cached reuse now reject revoked certificates and revoked cached @@ -24,6 +25,13 @@ All notable changes to this project are documented here. The format is based on is never parsed on-chain), while non-root revocation remains delegated to the revoker role. - Cold certificate verification rejects submitted cert bytes with trailing data or fields after the signature. +- Attestation parser is now forward-compatible with AWS COSE payload format changes: unrecognised + map keys are skipped instead of reverting, and the payload map plus the nested `pcrs` map and + `cabundle` array are accepted in both definite- and indefinite-length CBOR encodings. Malformed + encodings still revert (reserved headers, odd-length maps, mismatched indefinite-string chunks, a + missing break marker). These are liveness-only changes — the whole payload is still verified + against AWS's COSE signature, so an ignored or re-encoded field can never change the accept + decision. ### Fixed - Reject non-canonical P-384 public key coordinates greater than or equal to the field prime `p`. diff --git a/README.md b/README.md index 1c6b346..1bf8312 100644 --- a/README.md +++ b/README.md @@ -172,6 +172,12 @@ integrator (see [docs](docs/hinted-p384-nitro-attestation.md#integrator-responsi operator must monitor AWS CRLs and submit the affected certificate identity keys (`keccak256(issuerHash, serialHash)`, computed via `computeCertId` or directly from the CRL's issuer/serial entries). +- **Forward compatibility** — the attestation parser tolerates AWS evolving the COSE payload format: + unrecognised map keys are skipped (not rejected), and the payload map plus the nested `pcrs` map + and `cabundle` array are accepted in both definite- and indefinite-length CBOR encodings. This is + safe because the entire to-be-signed payload is checked against AWS's COSE signature, so unknown or + re-encoded content is signed and ignoring it can only ever drop a field the contract does not read + — it can never change the accept decision. ## Build diff --git a/docs/hinted-p384-nitro-attestation.md b/docs/hinted-p384-nitro-attestation.md index 69177f0..f71b3cb 100644 --- a/docs/hinted-p384-nitro-attestation.md +++ b/docs/hinted-p384-nitro-attestation.md @@ -414,6 +414,20 @@ its X.509 validity period has not expired. metadata was cached previously; it does not imply the cert is currently trusted, unexpired, or unrevoked. +**First-verified parent pinning.** A cached cert is pinned to the parent it was first +verified under: cold verification records `verifiedParent[certHash]` once, and every +later warm reuse requires the caller to present that exact `parentCertHash` (a mismatch +reverts with `parent cert mismatch`). This is a deliberate, conservative binding — warm +reuse skips signature verification, so it must reflect the precise chain that was +cryptographically checked, not merely *a* valid same-subject issuer. The liveness +consequence is that if the same certificate is genuinely issued under two different CA +objects (for example a same-key CA renewal that produces new DER bytes, hence a new +parent hash), the cached leaf keeps verifying only through its first parent; a second +caller chaining it through the renewed parent must wait for the cached entry to expire. +For AWS Nitro this is effectively a non-issue because leaf certificates are short-lived +(~3h) and expire long before their issuing CA is rotated, so the binding self-heals; it +is documented here as a known edge rather than a fixed bug. + **Warm-only guard.** `validateAttestationWithHints` re-runs the cabundle checks with an *empty* hint stream. Cached certs return before signature verification; a missing cert sends the empty stream into P384 verification and reverts with @@ -573,6 +587,31 @@ deliberately left to the caller and must be handled in the consuming contract: (`keccak256(issuerHash, serialHash)`, via `computeCertId` or computed directly from the CRL entry). +### Forward compatibility (attestation format changes) + +`_parseAttestation` is deliberately tolerant of AWS evolving the COSE payload, so a +benign format change cannot brick verification until a contract upgrade: + +- **Unknown keys are skipped, not rejected.** A map key the parser does not recognise + has its value skipped with a generic CBOR walk (`CborDecode.skipValue`) and parsing + continues. Previously an unrecognised key reverted (`"invalid attestation key"`), + which meant any new field AWS added would have permanently rejected every attestation. +- **Definite and indefinite length are both accepted.** The outer payload map and the + nested `pcrs` map and `cabundle` array are each parsed in either definite-length form + or indefinite-length form (`0xBF` / `0x9F` … `0xFF`). Previously the nested containers + assumed a definite count, so an encoder switch to indefinite length would have + silently produced an empty `pcrs` / `cabundle` (and, via a leaked inner break marker, + truncated the rest of the map). + +Both behaviours are **liveness-only and cannot cause a false accept**: the entire +to-be-signed payload is hashed and checked against AWS's COSE signature in +`validateAttestationWithHints`. Skipped or re-encoded content is therefore signed by +AWS, and a field the parser ignores cannot influence the accept decision — at most it +is not surfaced to the caller. Fields the contract *does* read (`pcrs`, `cabundle`, +`certificate`, `moduleID`, …) are parsed exactly as before. A malformed or truncated +encoding still reverts (out-of-bounds read or unterminated indefinite container), it +just never silently mis-parses. + ## 11. On-chain demo A Base Sepolia run of the §6 cold sequence needs: an RPC URL, a funded broadcaster diff --git a/src/CborDecode.sol b/src/CborDecode.sol index d60f8f8..d6340d0 100644 --- a/src/CborDecode.sol +++ b/src/CborDecode.sol @@ -94,6 +94,103 @@ library CborDecode { return elementAt(cbor, ptr.end(), 0x80, true); } + // Returns the index immediately after the complete CBOR data item that starts at `ix`, + // descending into nested arrays/maps and following indefinite-length containers to their + // 0xFF break marker. Unlike `elementAt`, this accepts every major type (ints, strings, + // arrays, maps, tags, simple/float values), so it can skip over values whose shape is not + // known ahead of time — e.g. the value of an unrecognised attestation key. Out-of-bounds + // reads (including a truncated indefinite-length container with no break marker) revert. + function skipValue(bytes memory cbor, uint256 ix) internal pure returns (uint256) { + uint8 b = uint8(cbor[ix]); + uint8 major = b >> 5; + uint8 ai = b & 0x1f; + + uint256 header; + uint64 arg; + bool indefinite; + if (ai < 24) { + header = 1; + arg = ai; + } else if (ai == 24) { + header = 2; + arg = uint8(cbor[ix + 1]); + } else if (ai == 25) { + header = 3; + arg = cbor.readUint16(ix + 1); + } else if (ai == 26) { + header = 5; + arg = cbor.readUint32(ix + 1); + } else if (ai == 27) { + header = 9; + arg = cbor.readUint64(ix + 1); + } else if (ai == 31) { + header = 1; + indefinite = true; + } else { + // additional information 28..30 are reserved per RFC 8949 + revert("invalid cbor additional info"); + } + + uint256 p = ix + header; + + if (major == 0 || major == 1) { + // unsigned / negative integer: the value lives entirely in the header + require(!indefinite, "invalid integer encoding"); + return p; + } else if (major == 2 || major == 3) { + // byte string / text string + if (indefinite) { + // an indefinite-length string is a sequence of definite-length chunks of the SAME + // major type (RFC 8949 §3.2.3); reject any other chunk rather than skipping it + while (uint8(cbor[p]) != 0xff) { + uint8 chunk = uint8(cbor[p]); + require(chunk >> 5 == major && (chunk & 0x1f) != 31, "invalid indefinite string chunk"); + p = skipValue(cbor, p); + } + return p + 1; + } + require(p + arg <= cbor.length, "cbor string out of bounds"); + return p + arg; + } else if (major == 4) { + // array + if (indefinite) { + while (uint8(cbor[p]) != 0xff) { + p = skipValue(cbor, p); + } + return p + 1; + } + for (uint64 i = 0; i < arg; i++) { + p = skipValue(cbor, p); + } + return p; + } else if (major == 5) { + // map: each entry is a key item followed by a value item + if (indefinite) { + // a map must have an even number of items (key/value pairs); a dangling key before + // the break marker is malformed and must revert + while (uint8(cbor[p]) != 0xff) { + p = skipValue(cbor, p); // key + require(uint8(cbor[p]) != 0xff, "odd cbor map item count"); + p = skipValue(cbor, p); // value + } + return p + 1; + } + for (uint64 i = 0; i < arg; i++) { + p = skipValue(cbor, p); + p = skipValue(cbor, p); + } + return p; + } else if (major == 6) { + // tag: a single tagged data item follows the header + require(!indefinite, "invalid tag encoding"); + return skipValue(cbor, p); + } else { + // major == 7: simple value / float; ai==31 (break) is not a standalone value + require(!indefinite, "unexpected break"); + return p; + } + } + function elementAt(bytes memory cbor, uint256 ix, uint8 expectedType, bool required) internal pure diff --git a/src/CertManager.sol b/src/CertManager.sol index 305d528..1a5c0e2 100644 --- a/src/CertManager.sol +++ b/src/CertManager.sol @@ -50,7 +50,13 @@ contract CertManager is ICertManager { // certHash -> VerifiedCert mapping(bytes32 => bytes) public verified; - // certHash -> parent cert hash used during cold verification + // certHash -> parent cert hash used during cold verification. + // A cached cert is pinned to the parent it was FIRST verified under: warm reuse requires the + // caller to present this exact parent (see the mismatch check in `_verifyCert`). This is + // intentional — warm reuse skips signature verification, so it must reflect the precise chain + // that was cryptographically checked. A same-key CA renewal (new DER, new parent hash) cannot + // re-bind an already-cached descendant until its cache entry expires; harmless for Nitro because + // leaves are short-lived. See docs/hinted-p384-nitro-attestation.md "First-verified parent pinning". mapping(bytes32 => bytes32) internal verifiedParent; // certHash -> revocation identity key (keccak256(issuerHash, serialHash)), recorded at cold // verification so the warm path and parent-chain walk can check revocation without re-parsing. diff --git a/src/NitroValidator.sol b/src/NitroValidator.sol index e87eae1..06c1a6e 100644 --- a/src/NitroValidator.sol +++ b/src/NitroValidator.sol @@ -188,17 +188,36 @@ contract NitroValidator { LibBytes.memcpy(dest + 13 + rawProtectedLength, payloadSrc, rawPayloadLength); } + /// @dev Parses the COSE payload into pointers, without copying. Forward-compatibility notes: + /// - Unknown map keys are skipped, not rejected, so AWS adding new attestation fields does + /// not brick verification. This is safe because the whole TBS is later checked against + /// AWS's COSE signature, so unknown content is signed and ignoring it cannot change the + /// accept decision. + /// - The outer payload map and the nested `pcrs` map / `cabundle` array are each accepted in + /// both definite-length and indefinite-length CBOR form. function _parseAttestation(bytes memory attestationTbs) internal pure returns (Ptrs memory) { require(attestationTbs.keccak(0, 18) == ATTESTATION_TBS_PREFIX, "invalid attestation prefix"); CborElement payload = attestationTbs.byteStringAt(18); - CborElement current = attestationTbs.mapAt(payload.start()); + uint256 mapHeaderIx = payload.start(); + CborElement current = attestationTbs.mapAt(mapHeaderIx); + bool outerIndefinite = _isIndefinite(attestationTbs, mapHeaderIx); + uint256 entryCount = current.value(); // entry count for a definite-length map Ptrs memory ptrs; uint256 end = payload.end(); - while (current.end() < end) { - // Break marker (0xFF) terminates indefinite-length maps - if (uint8(attestationTbs[current.end()]) == 0xff) break; + for (uint256 entry = 0;; entry++) { + if (outerIndefinite) { + // An indefinite-length map is terminated only by a 0xFF break marker. Require it to + // be present (do not silently stop at the payload end on a missing break), and stop + // exclusively on it. + require(current.end() < end, "missing break marker"); + if (uint8(attestationTbs[current.end()]) == 0xff) break; + } else { + // A definite-length map ends after exactly `entryCount` entries; a stray 0xFF must + // not terminate it early (it would be parsed as a key and rejected as a non-string). + if (entry == entryCount) break; + } current = attestationTbs.nextTextString(current); bytes32 keyHash = attestationTbs.keccak(current); if (keyHash == MODULE_ID_KEY) { @@ -223,28 +242,98 @@ contract NitroValidator { current = attestationTbs.nextPositiveInt(current); ptrs.timestamp = uint64(current.value()); } else if (keyHash == CABUNDLE_KEY) { - current = attestationTbs.nextArray(current); - ptrs.cabundle = new CborElement[](current.value()); - for (uint256 i = 0; i < ptrs.cabundle.length; i++) { - current = attestationTbs.nextByteString(current); - ptrs.cabundle[i] = current; - } + (ptrs.cabundle, current) = _parseCabundle(attestationTbs, current); } else if (keyHash == PCRS_KEY) { - current = attestationTbs.nextMap(current); - ptrs.pcrs = new CborElement[](current.value()); - for (uint256 i = 0; i < ptrs.pcrs.length; i++) { - current = attestationTbs.nextPositiveInt(current); - uint256 key = current.value(); - require(key < ptrs.pcrs.length, "invalid pcr key value"); - require(CborElement.unwrap(ptrs.pcrs[key]) == 0, "duplicate pcr key"); - current = attestationTbs.nextByteString(current); - ptrs.pcrs[key] = current; - } + (ptrs.pcrs, current) = _parsePcrs(attestationTbs, current); } else { - revert("invalid attestation key"); + // Forward-compatibility: skip (rather than reject) keys this parser does not + // recognise. The entire TBS is covered by AWS's COSE signature verified in + // {validateAttestationWithHints}, so an unknown key cannot be injected without + // invalidating that signature, and ignoring it can only ever drop a field we do + // not read — never change the accept decision. Rejecting unknown keys instead + // would brick verification the moment AWS adds a new attestation field. + uint256 nextIx = attestationTbs.skipValue(current.end()); + current = LibCborElement.toCborElement(0x00, nextIx, 0); } } return ptrs; } + + /// @dev Parses the `cabundle` array (definite- or indefinite-length) starting from the key + /// element `keyPtr`. Returns the parsed cert pointers and the cursor positioned after the + /// array (past the break marker, for indefinite encoding). + function _parseCabundle(bytes memory tbs, CborElement keyPtr) + private + pure + returns (CborElement[] memory cabundle, CborElement current) + { + uint256 headerIx = keyPtr.end(); + current = tbs.nextArray(keyPtr); + bool indefinite = _isIndefinite(tbs, headerIx); + uint256 count = indefinite ? _countIndefiniteItems(tbs, current.end()) : current.value(); + cabundle = new CborElement[](count); + for (uint256 i = 0; i < count; i++) { + current = tbs.nextByteString(current); + cabundle[i] = current; + } + if (indefinite) current = _consumeBreak(tbs, current); + } + + /// @dev Parses the `pcrs` map (definite- or indefinite-length) starting from the key element + /// `keyPtr`. Returns the parsed pcr pointers (indexed by pcr key) and the cursor positioned + /// after the map (past the break marker, for indefinite encoding). + function _parsePcrs(bytes memory tbs, CborElement keyPtr) + private + pure + returns (CborElement[] memory pcrs, CborElement current) + { + uint256 headerIx = keyPtr.end(); + current = tbs.nextMap(keyPtr); + bool indefinite = _isIndefinite(tbs, headerIx); + uint256 count; + if (indefinite) { + // each map entry is a key/value pair, so a well-formed indefinite map holds an even + // number of items (2 per pcr); an odd count is malformed and must revert rather than + // silently drop the trailing item. + uint256 items = _countIndefiniteItems(tbs, current.end()); + require(items % 2 == 0, "invalid pcrs map"); + count = items / 2; + } else { + count = current.value(); + } + pcrs = new CborElement[](count); + for (uint256 i = 0; i < count; i++) { + current = tbs.nextPositiveInt(current); + uint256 key = current.value(); + require(key < count, "invalid pcr key value"); + require(CborElement.unwrap(pcrs[key]) == 0, "duplicate pcr key"); + current = tbs.nextByteString(current); + pcrs[key] = current; + } + if (indefinite) current = _consumeBreak(tbs, current); + } + + /// @dev True if the CBOR container header at `headerIx` uses indefinite-length encoding (ai=31). + function _isIndefinite(bytes memory cbor, uint256 headerIx) private pure returns (bool) { + return (uint8(cbor[headerIx]) & 0x1f) == 31; + } + + /// @dev Counts the data items of an indefinite-length container whose first item starts at `ix`, + /// stopping at the 0xFF break marker. Reverts on truncated input (no break before the end). + function _countIndefiniteItems(bytes memory cbor, uint256 ix) private pure returns (uint256 count) { + while (uint8(cbor[ix]) != 0xff) { + ix = cbor.skipValue(ix); + count++; + } + } + + /// @dev Verifies the byte immediately after `ptr` is the 0xFF break marker and returns a + /// zero-length element positioned just past it, so the caller's cursor skips the consumed + /// break. Reverts if the marker is absent (e.g. a nested indefinite container that did not + /// close where the fill loop ended), turning a malformed encoding into a revert. + function _consumeBreak(bytes memory cbor, CborElement ptr) private pure returns (CborElement) { + require(uint8(cbor[ptr.end()]) == 0xff, "expected break marker"); + return LibCborElement.toCborElement(0x00, ptr.end() + 1, 0); + } } diff --git a/test/IndefiniteLengthCbor.t.sol b/test/IndefiniteLengthCbor.t.sol index 2f31395..a74b531 100644 --- a/test/IndefiniteLengthCbor.t.sol +++ b/test/IndefiniteLengthCbor.t.sol @@ -74,6 +74,10 @@ contract CborDecodeHarness { CborElement e = cbor.nextArray(prev); return (e.cborType(), e.start(), e.value()); } + + function skipValue(bytes memory cbor, uint256 ix) external pure returns (uint256) { + return cbor.skipValue(ix); + } } /// @notice Exposes NitroValidator._parseAttestation (internal pure) for testing. @@ -209,6 +213,108 @@ contract CborDecodeIndefiniteLengthTest is Test { function test_mapAt_reservedAI30_reverts() public { _assertMapReservedReverts(abi.encodePacked(CBOR_MAP_AI30)); } + + // ── skipValue: spans a complete data item of any major type ─── + + /// @dev Small unsigned int (ai<24) spans 1 byte. + function test_skipValue_tinyInt() public view { + assertEq(harness.skipValue(hex"0a", 0), 1, "uint 10"); + } + + /// @dev 1-byte-argument int (ai=24) spans 2 bytes. + function test_skipValue_int_ai24() public view { + assertEq(harness.skipValue(hex"1820", 0), 2, "uint 32"); + } + + /// @dev Byte string spans header + content. + function test_skipValue_byteString() public view { + assertEq(harness.skipValue(hex"43aabbcc", 0), 4, "bytes(3)"); + } + + /// @dev Text string spans header + content. + function test_skipValue_textString() public view { + assertEq(harness.skipValue(hex"6474657374", 0), 5, "\"test\""); + } + + /// @dev Definite array recurses through all elements. + function test_skipValue_definiteArray() public view { + // [1, [2, 3], 4] + assertEq(harness.skipValue(hex"830182020304", 0), 6, "array(3) with nested array"); + } + + /// @dev Definite map recurses through key/value pairs. + function test_skipValue_definiteMap() public view { + // {1: 2, 3: bytes(2)} + assertEq(harness.skipValue(hex"a201020342aabb", 0), 7, "map(2)"); + } + + /// @dev Indefinite array stops at break and consumes it. + function test_skipValue_indefiniteArray() public view { + // [_ 1, 2 ] + assertEq(harness.skipValue(hex"9f0102ff", 0), 4, "indefinite array"); + } + + /// @dev Indefinite map stops at break and consumes it. + function test_skipValue_indefiniteMap() public view { + // {_ 1: 2 } + assertEq(harness.skipValue(hex"bf0102ff", 0), 4, "indefinite map"); + } + + /// @dev null / simple values span 1 byte. + function test_skipValue_null() public view { + assertEq(harness.skipValue(hex"f6", 0), 1, "null"); + } + + /// @dev Tag spans the tag header plus the tagged item. + function test_skipValue_tag() public view { + // tag(0) "..." — 0xC0 then a 1-byte int + assertEq(harness.skipValue(hex"c001", 0), 2, "tag + int"); + } + + /// @dev Reserved additional-info (28) reverts. + function test_skipValue_reservedAI_reverts() public { + vm.expectRevert("invalid cbor additional info"); + harness.skipValue(hex"1c", 0); + } + + /// @dev A bare break code is not a standalone value. + function test_skipValue_bareBreak_reverts() public { + vm.expectRevert("unexpected break"); + harness.skipValue(hex"ff", 0); + } + + /// @dev Truncated indefinite container (no break) reverts on out-of-bounds read. + function test_skipValue_truncatedIndefinite_reverts() public { + vm.expectRevert(); + harness.skipValue(hex"9f0102", 0); + } + + /// @dev Indefinite text string with a valid same-type chunk ("a") is accepted. + function test_skipValue_indefiniteTextString() public view { + // 0x7F (indefinite text), 0x61 0x61 ("a"), 0xFF + assertEq(harness.skipValue(hex"7f6161ff", 0), 4, "indefinite text string"); + } + + /// @dev Indefinite string whose chunk is not a definite string of the same major reverts. + function test_skipValue_indefiniteStringNonStringChunk_reverts() public { + // 0x7F (indefinite text) followed by 0x01 (an integer, not a text chunk) + vm.expectRevert("invalid indefinite string chunk"); + harness.skipValue(hex"7f01ff", 0); + } + + /// @dev Indefinite byte string whose chunk is a text string (wrong major) reverts. + function test_skipValue_indefiniteByteStringWrongMajorChunk_reverts() public { + // 0x5F (indefinite bytes) followed by 0x61 0x61 (a *text* string chunk) + vm.expectRevert("invalid indefinite string chunk"); + harness.skipValue(hex"5f6161ff", 0); + } + + /// @dev Indefinite map with an odd number of items (dangling key) reverts. + function test_skipValue_indefiniteMapOddItems_reverts() public { + // 0xBF (indefinite map), key 0x01, then break — no value for the key + vm.expectRevert("odd cbor map item count"); + harness.skipValue(hex"bf01ff", 0); + } } // ────────────────────────────────────────────────────────────── @@ -547,10 +653,10 @@ contract NitroValidatorIndefiniteLengthTest is Test { // ── Inner indefinite-length structures ─────────────────── /// @dev Inner PCRs map as empty indefinite-length (0xBF 0xFF) inside a - /// definite-length outer map. The inner 0xFF break marker is picked up - /// by the outer while-loop's break check (NitroValidator.sol:157), - /// causing silent early termination. Entries after pcrs are not parsed. - function test_edge_innerIndefinitePcrsEmpty_outerBreakTriggered() public view { + /// definite-length outer map. The inner break marker is consumed by the + /// pcrs handler, so the outer loop continues and entries after pcrs are + /// parsed normally (L2 forward-compatibility fix). + function test_edge_innerIndefinitePcrsEmpty_parsesAndContinues() public view { bytes memory part1 = abi.encodePacked( hex"696d6f64756c655f6964", hex"6474657374", // module_id: "test" @@ -583,36 +689,98 @@ contract NitroValidatorIndefiniteLengthTest is Test { assertEq(p.digest.length(), SYNTH_DIGEST_LEN, "digest parsed"); assertEq(p.timestamp, SYNTH_TIMESTAMP, "timestamp parsed"); - // pcrs: empty (indefinite-length map -> value=0) + // pcrs: empty (indefinite-length map with no entries) assertEq(p.pcrs.length, 0, "pcrs empty"); - // Fields after pcrs: NOT parsed — inner 0xFF triggers outer break - assertEq(p.cert.length(), 0, "cert not parsed"); - assertEq(p.cabundle.length, 0, "cabundle not parsed"); + // Fields after pcrs: now parsed — the inner break is consumed, not leaked + assertEq(p.cert.length(), SYNTH_CERT_LEN, "cert parsed"); + assertEq(p.cabundle.length, SYNTH_CABUNDLE_COUNT, "cabundle parsed"); + assertEq(p.cabundle[0].length(), SYNTH_CABUNDLE_CERT_LEN, "cabundle[0] parsed"); + assertTrue(p.publicKey.isNull(), "public_key parsed"); + assertTrue(p.userData.isNull(), "user_data parsed"); + assertTrue(p.nonce.isNull(), "nonce parsed"); + } + + /// @dev Inner non-empty indefinite-length pcrs map ({0:48B, 1:48B} as 0xBF ... 0xFF) + /// inside a definite-length outer map parses both PCR entries and continues. + function test_edge_innerIndefinitePcrsNonEmpty_parses() public view { + bytes memory part1 = abi.encodePacked( + hex"696d6f64756c655f6964", + hex"6474657374", // module_id: "test" + hex"66646967657374", + hex"66534841333834", // digest: "SHA384" + hex"6974696d657374616d70", + hex"1a000f4240" // timestamp: 1_000_000 + ); + // pcrs: indefinite map {0: 48B, 1: 48B} + bytes memory pcrs = abi.encodePacked( + hex"6470637273", // key "pcrs" + CBOR_MAP_INDEFINITE, + hex"005830", + new bytes(SYNTH_PCR_LEN), // 0 -> 48B + hex"015830", + new bytes(SYNTH_PCR_LEN), // 1 -> 48B + CBOR_BREAK + ); + bytes memory part3 = abi.encodePacked( + hex"6b6365727469666963617465", + hex"4400000000", // certificate: bytes(4) + hex"68636162756e646c65", + hex"814400000000", // cabundle: [bytes(4)] + hex"6a7075626c69635f6b6579", + hex"f6", + hex"69757365725f64617461", + hex"f6", + hex"656e6f6e6365", + hex"f6" + ); + bytes memory tbs = _buildTbs(abi.encodePacked(hex"a9", part1, pcrs, part3)); + NitroValidator.Ptrs memory p = validator.parseAttestation(tbs); + + assertEq(p.pcrs.length, 2, "two pcrs parsed"); + assertEq(p.pcrs[0].length(), SYNTH_PCR_LEN, "pcr[0] length"); + assertEq(p.pcrs[1].length(), SYNTH_PCR_LEN, "pcr[1] length"); + // entries after pcrs still parsed + assertEq(p.cert.length(), SYNTH_CERT_LEN, "cert parsed"); + assertEq(p.cabundle.length, SYNTH_CABUNDLE_COUNT, "cabundle parsed"); } // ══════════════════════════════════════════════════════════ // NEGATIVE TESTS // ══════════════════════════════════════════════════════════ - /// @dev Unknown key in a definite-length map reverts. - function test_neg_unknownKeyDefinite_reverts() public { - bytes memory badEntry = abi.encodePacked( - hex"6362616400", // key "bad" + padding byte to avoid 0xFF false hit - hex"6474657374" // val "test" + /// @dev Unknown key in a definite-length map is skipped; known fields still parse + /// (L1 forward-compatibility fix). The unknown value here is a nested map, to + /// exercise the recursive skipValue path. + function test_unknownKeyDefinite_isSkipped() public view { + // 9 known entries + 1 unknown ("extra" -> {1: 2}) = 10-entry definite map (0xAA). + bytes memory unknownEntry = abi.encodePacked( + hex"656578747261", // key "extra" (text, 5B) + hex"a10102" // val {1: 2} (nested 1-entry map) ); - vm.expectRevert("invalid attestation key"); - validator.parseAttestation(_buildTbs(abi.encodePacked(hex"a1", badEntry))); + bytes memory tbs = _buildTbs(abi.encodePacked(hex"aa", _entries(), unknownEntry)); + _assertSyntheticFields(validator.parseAttestation(tbs)); } - /// @dev Unknown key in an indefinite-length map also reverts. - function test_neg_unknownKeyIndefinite_reverts() public { - bytes memory badEntry = abi.encodePacked( + /// @dev Unknown key in an indefinite-length map is skipped; known fields still parse. + /// The unknown value here is an array, a different skipValue branch. + function test_unknownKeyIndefinite_isSkipped() public view { + bytes memory unknownEntry = abi.encodePacked( + hex"63626164", // key "bad" (text, 3B) + hex"820102" // val [1, 2] (array(2): 0x82, 0x01, 0x02) + ); + bytes memory tbs = _buildTbs(abi.encodePacked(CBOR_MAP_INDEFINITE, _entries(), unknownEntry, CBOR_BREAK)); + _assertSyntheticFields(validator.parseAttestation(tbs)); + } + + /// @dev Unknown key whose value is a byte string is skipped by length. + function test_unknownKeyByteStringValue_isSkipped() public view { + bytes memory unknownEntry = abi.encodePacked( hex"63626164", // key "bad" - hex"6474657374" // val "test" + hex"43aabbcc" // val bytes(3) = 0xAABBCC ); - vm.expectRevert("invalid attestation key"); - validator.parseAttestation(_buildTbs(abi.encodePacked(CBOR_MAP_INDEFINITE, badEntry, CBOR_BREAK))); + bytes memory tbs = _buildTbs(abi.encodePacked(hex"aa", _entries(), unknownEntry)); + _assertSyntheticFields(validator.parseAttestation(tbs)); } /// @dev Indefinite-length map without a trailing 0xFF break marker. @@ -626,18 +794,60 @@ contract NitroValidatorIndefiniteLengthTest is Test { } /// @dev Indefinite-length outer map containing a non-empty indefinite-length - /// inner cabundle array. The inner array elements are not consumed by - /// the cabundle loop (value=0 for indefinite), so the parser tries to - /// interpret the inner byte-string element as an outer text-string key, - /// reverting with "unexpected type". - function test_neg_nestedIndefiniteNonEmptyArray_reverts() public { + /// inner cabundle array now parses every element and continues past the + /// inner break marker (L2 forward-compatibility fix). + function test_nestedIndefiniteNonEmptyArray_parses() public view { bytes memory entries = abi.encodePacked( hex"68636162756e646c65", // key "cabundle" - hex"9f", // indefinite-length array - hex"4400000000", // one bstr element (not consumed by inner loop) - hex"ff" // inner array break + CBOR_ARRAY_INDEFINITE, // indefinite-length array + hex"4401020304", // element 0: bytes(4) + hex"4405060708", // element 1: bytes(4) + CBOR_BREAK // inner array break ); - vm.expectRevert("unexpected type"); - validator.parseAttestation(_buildTbs(abi.encodePacked(CBOR_MAP_INDEFINITE, entries, CBOR_BREAK))); + bytes memory tbs = _buildTbs(abi.encodePacked(CBOR_MAP_INDEFINITE, entries, CBOR_BREAK)); + NitroValidator.Ptrs memory p = validator.parseAttestation(tbs); + assertEq(p.cabundle.length, 2, "two cabundle elements parsed"); + assertEq(p.cabundle[0].length(), 4, "cabundle[0] length"); + assertEq(p.cabundle[1].length(), 4, "cabundle[1] length"); + } + + /// @dev Indefinite-length inner pcrs map with an odd item count (dangling key) reverts + /// instead of floor-dividing and under-parsing. + function test_neg_innerIndefinitePcrsOddItems_reverts() public { + bytes memory pcrs = abi.encodePacked( + hex"6470637273", // key "pcrs" + CBOR_MAP_INDEFINITE, + hex"005830", + new bytes(SYNTH_PCR_LEN), // 0 -> 48B + hex"01", // dangling key 1 with no value + CBOR_BREAK + ); + bytes memory tbs = _buildTbs(abi.encodePacked(hex"a1", pcrs)); + vm.expectRevert("invalid pcrs map"); + validator.parseAttestation(tbs); + } + + /// @dev Indefinite-length outer map with no trailing 0xFF break marker reverts (a missing + /// break must not be silently accepted by running into the payload end). + function test_neg_indefiniteOuterMapMissingBreak_reverts() public { + bytes memory tbs = _buildTbs(abi.encodePacked(CBOR_MAP_INDEFINITE, _partialEntries())); + vm.expectRevert("missing break marker"); + validator.parseAttestation(tbs); + } + + /// @dev Empty indefinite-length inner cabundle array ([0x9F, 0xFF]) parses as an + /// empty cabundle and the outer loop continues. + function test_nestedIndefiniteEmptyArray_parses() public view { + bytes memory entries = abi.encodePacked( + hex"68636162756e646c65", // key "cabundle" + CBOR_ARRAY_INDEFINITE, + CBOR_BREAK, + hex"66646967657374", + hex"66534841333834" // digest: "SHA384" after cabundle + ); + bytes memory tbs = _buildTbs(abi.encodePacked(CBOR_MAP_INDEFINITE, entries, CBOR_BREAK)); + NitroValidator.Ptrs memory p = validator.parseAttestation(tbs); + assertEq(p.cabundle.length, 0, "cabundle empty"); + assertEq(p.digest.length(), SYNTH_DIGEST_LEN, "digest after empty cabundle parsed"); } }