From 8c09eadaf192d8bb94efcb3a5e5a905a4dbb605a Mon Sep 17 00:00:00 2001 From: Leopold Joy Date: Fri, 24 Jul 2026 21:30:16 +0100 Subject: [PATCH] fix: support sparse PCR banks Co-authored-by: OpenCode --- README.md | 9 +- docs/hinted-p384-nitro-attestation.md | 6 +- src/NitroValidator.sol | 31 +++++-- test/IndefiniteLengthCbor.t.sol | 116 +++++++++++++++++++++++--- 4 files changed, 140 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 3605b7c..6187fcf 100644 --- a/README.md +++ b/README.md @@ -107,10 +107,11 @@ pragma solidity ^0.8.0; import {NitroValidator} from "@nitro-validator/NitroValidator.sol"; import {CertManager} from "@nitro-validator/CertManager.sol"; -import {CborDecode} from "@nitro-validator/CborDecode.sol"; +import {CborDecode, CborElement, LibCborElement} from "@nitro-validator/CborDecode.sol"; contract Validator { using CborDecode for bytes; + using LibCborElement for CborElement; uint256 public constant MAX_AGE = 60 minutes; bytes32 public constant PCR0 = keccak256("some PCR0 value"); @@ -131,6 +132,8 @@ contract Validator { NitroValidator.Ptrs memory ptrs = validator.validateAttestationWithHints(attestationTbs, signature, attestationHints); + // `pcrs` is always a 32-slot bank. Sparse AWS maps leave omitted slots as CBOR null. + require(!ptrs.pcrs[0].isNull(), "missing pcr0"); bytes32 pcr0 = attestationTbs.keccak(ptrs.pcrs[0]); require(pcr0 == PCR0, "invalid pcr0 in attestation"); require(ptrs.timestamp / 1000 + MAX_AGE > block.timestamp, "attestation too old"); @@ -172,7 +175,9 @@ integrator (see [docs](docs/hinted-p384-nitro-attestation.md#integrator-responsi integrators must still never use attestation signatures as uniqueness keys; dedupe on canonical attestation fields instead. - **Enclave policy** — checking `pcrs` / `moduleID` against the enclave image(s) you trust is your - responsibility. + responsibility. `ptrs.pcrs` is a 32-slot bank indexed by PCR number; omitted AWS PCR entries + are explicit CBOR-null pointers, so consumers must check `isNull()` before reading or hashing a + slot. - **Revocation operations** — the contract enforces the on-chain revoked set, but an off-chain operator must monitor AWS CRLs and submit the affected certificate identity keys (`keccak256(issuerHash, serialHash)`, computed via `computeCertId` or directly from the CRL's diff --git a/docs/hinted-p384-nitro-attestation.md b/docs/hinted-p384-nitro-attestation.md index 08e7a04..9ae2563 100644 --- a/docs/hinted-p384-nitro-attestation.md +++ b/docs/hinted-p384-nitro-attestation.md @@ -602,8 +602,10 @@ deliberately left to the caller and must be handled in the consuming contract: attestation AWS never produced, but you must NOT use the raw signature (or `attestationTbs + signature`, or its hash) as a unique key — dedupe on canonical attestation fields (e.g. `moduleID + timestamp + nonce`). -- **Enclave-image / PCR policy.** The contract returns the parsed `pcrs` and - `moduleID`; deciding which enclave images you trust is application policy. +- **Enclave-image / PCR policy.** The contract returns `moduleID` and a 32-slot `pcrs` bank + indexed by PCR number; deciding which enclave images you trust is application policy. AWS may + omit PCRs from the map, so omitted slots are explicit CBOR-null pointers and consumers must + check `ptrs.pcrs[index].isNull()` before reading or hashing a slot. - **CRL monitoring.** `CertManager` enforces the certificate identity keys that have been marked revoked on-chain, but it does not fetch or parse AWS CRLs. A trusted off-chain operator must monitor AWS CRLs and submit `revokeCert` / `revokeCerts` transactions diff --git a/src/NitroValidator.sol b/src/NitroValidator.sol index bbb691f..8ed5730 100644 --- a/src/NitroValidator.sol +++ b/src/NitroValidator.sol @@ -44,6 +44,7 @@ contract NitroValidator { CborElement moduleID; uint64 timestamp; CborElement digest; + // Always a 32-slot PCR bank. Missing PCR indices are CBOR-null sentinels. CborElement[] pcrs; CborElement cert; CborElement[] cabundle; @@ -110,7 +111,8 @@ contract NitroValidator { /// so do not use `signature` (or its hash) as a uniqueness key — dedupe on attestation /// fields instead. /// - PCR / moduleID policy: the caller must check `ptrs.pcrs` / `ptrs.moduleID` against the - /// enclave image(s) they trust. + /// enclave image(s) they trust. `ptrs.pcrs` is a 32-slot bank indexed by PCR number; + /// omitted slots are CBOR-null sentinels and must be checked with `isNull()` before use. /// @param attestationTbs The COSE Sign1 to-be-signed bytes (from `decodeAttestationTbs`). /// @param signature The 96-byte (r||s) P-384 attestation signature. /// @param attestationSigHints Off-chain inverse hints for the attestation signature; re-verified @@ -126,7 +128,7 @@ contract NitroValidator { require(ptrs.timestamp > 0, "no timestamp"); require(ptrs.cabundle.length > 0, "no cabundle"); require(attestationTbs.keccak(ptrs.digest) == ATTESTATION_DIGEST, "invalid digest"); - require(1 <= ptrs.pcrs.length && ptrs.pcrs.length <= MAX_PCRS, "invalid pcrs"); + require(ptrs.pcrs.length == MAX_PCRS, "invalid pcrs"); require( ptrs.publicKey.isNull() || (1 <= ptrs.publicKey.length() && ptrs.publicKey.length() <= 1024), "invalid pub key" @@ -134,11 +136,15 @@ contract NitroValidator { require(ptrs.userData.isNull() || (ptrs.userData.length() <= 512), "invalid user data"); require(ptrs.nonce.isNull() || (ptrs.nonce.length() <= 512), "invalid nonce"); + uint256 pcrCount; for (uint256 i = 0; i < ptrs.pcrs.length; i++) { + if (ptrs.pcrs[i].isNull()) continue; + pcrCount++; require( ptrs.pcrs[i].length() == 32 || ptrs.pcrs[i].length() == 48 || ptrs.pcrs[i].length() == 64, "invalid pcr" ); } + require(pcrCount > 0, "invalid pcrs"); bytes memory cert = attestationTbs.slice(ptrs.cert); bytes[] memory cabundle = new bytes[](ptrs.cabundle.length); @@ -222,6 +228,7 @@ contract NitroValidator { uint256 entryCount = current.value(); // entry count for a definite-length map Ptrs memory ptrs; + ptrs.pcrs = _newPcrBank(); uint256 seenKeys; uint256 end = payload.end(); for (uint256 entry = 0;; entry++) { @@ -320,8 +327,8 @@ contract NitroValidator { } /// @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). + /// `keyPtr`. Returns a 32-slot bank 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 @@ -342,18 +349,28 @@ contract NitroValidator { count = current.value(); } require(count <= MAX_PCRS, "too many pcrs"); - pcrs = new CborElement[](count); + pcrs = _newPcrBank(); 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"); + require(key < MAX_PCRS, "invalid pcr key value"); + require(pcrs[key].isNull(), "duplicate pcr key"); current = tbs.nextByteString(current); pcrs[key] = current; } if (indefinite) current = _consumeBreak(tbs, current); } + /// @dev Returns the fixed-size PCR bank. A null sentinel makes absent indices safe to inspect + /// with {LibCborElement.isNull} without confusing them with a valid zero-length pointer. + function _newPcrBank() private pure returns (CborElement[] memory pcrs) { + pcrs = new CborElement[](MAX_PCRS); + CborElement empty = LibCborElement.toCborElement(0xf6, 0, 0); + for (uint256 i = 0; i < MAX_PCRS; i++) { + pcrs[i] = empty; + } + } + /// @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; diff --git a/test/IndefiniteLengthCbor.t.sol b/test/IndefiniteLengthCbor.t.sol index c9ee005..6a1a671 100644 --- a/test/IndefiniteLengthCbor.t.sol +++ b/test/IndefiniteLengthCbor.t.sol @@ -34,8 +34,8 @@ uint8 constant CBOR_MAP_AI30 = 0xbe; uint256 constant SYNTH_MODULE_ID_LEN = 4; // "test" uint256 constant SYNTH_DIGEST_LEN = 6; // "SHA384" uint64 constant SYNTH_TIMESTAMP = 1_000_000; -uint256 constant SYNTH_PCRS_COUNT = 1; uint256 constant SYNTH_PCR_LEN = 48; +uint256 constant PCR_BANK_SIZE = 32; uint256 constant SYNTH_CERT_LEN = 4; uint256 constant SYNTH_CABUNDLE_COUNT = 1; uint256 constant SYNTH_CABUNDLE_CERT_LEN = 4; @@ -364,8 +364,9 @@ contract NitroValidatorIndefiniteLengthTest is Test { assertEq(p.moduleID.length(), SYNTH_MODULE_ID_LEN, "module_id length"); assertEq(p.timestamp, SYNTH_TIMESTAMP, "timestamp"); assertEq(p.digest.length(), SYNTH_DIGEST_LEN, "digest length"); - assertEq(p.pcrs.length, SYNTH_PCRS_COUNT, "pcrs count"); + assertEq(p.pcrs.length, PCR_BANK_SIZE, "pcr bank size"); assertEq(p.pcrs[0].length(), SYNTH_PCR_LEN, "pcr[0] length"); + assertTrue(p.pcrs[1].isNull(), "pcr[1] absent"); assertEq(p.cert.length(), SYNTH_CERT_LEN, "cert length"); assertEq(p.cabundle.length, SYNTH_CABUNDLE_COUNT, "cabundle count"); assertEq(p.cabundle[0].length(), SYNTH_CABUNDLE_CERT_LEN, "cabundle[0] length"); @@ -379,10 +380,11 @@ contract NitroValidatorIndefiniteLengthTest is Test { assertEq(p.moduleID.length(), REAL_MODULE_ID_LEN, "module_id length"); assertGt(p.timestamp, 0, "timestamp > 0"); assertEq(p.digest.length(), REAL_DIGEST_LEN, "digest length"); - assertEq(p.pcrs.length, REAL_PCRS_COUNT, "pcrs count"); + assertEq(p.pcrs.length, PCR_BANK_SIZE, "pcr bank size"); for (uint256 i = 0; i < REAL_PCRS_COUNT; i++) { assertEq(p.pcrs[i].length(), REAL_PCR_LEN, "pcr length"); } + assertTrue(p.pcrs[REAL_PCRS_COUNT].isNull(), "next PCR absent"); assertEq(p.cert.length(), REAL_CERT_LEN, "cert length"); assertEq(p.cabundle.length, REAL_CABUNDLE_COUNT, "cabundle count"); // public_key is NON-null in this real attestation @@ -596,9 +598,8 @@ contract NitroValidatorIndefiniteLengthTest is Test { // EDGE CASES // ══════════════════════════════════════════════════════════ - /// @dev Empty indefinite-length map (0xBF 0xFF): returns zero-initialised - /// Ptrs without reverting. Downstream validateAttestation() would - /// catch missing required fields. + /// @dev Empty indefinite-length map (0xBF 0xFF): returns unset Ptrs without reverting. The PCR + /// bank is still initialized, and downstream validation catches missing required fields. function test_edge_emptyIndefiniteLengthMap() public view { bytes memory tbs = _buildTbs(abi.encodePacked(CBOR_MAP_INDEFINITE, CBOR_BREAK)); @@ -606,7 +607,8 @@ contract NitroValidatorIndefiniteLengthTest is Test { assertEq(p.moduleID.length(), 0, "module_id unset"); assertEq(p.timestamp, 0, "timestamp unset"); - assertEq(p.pcrs.length, 0, "pcrs unset"); + assertEq(p.pcrs.length, PCR_BANK_SIZE, "pcr bank initialized"); + assertTrue(p.pcrs[0].isNull(), "pcrs unset"); assertEq(p.cabundle.length, 0, "cabundle unset"); } @@ -620,9 +622,10 @@ contract NitroValidatorIndefiniteLengthTest is Test { // Parsed entries assertEq(p.moduleID.length(), SYNTH_MODULE_ID_LEN, "module_id parsed"); assertEq(p.digest.length(), SYNTH_DIGEST_LEN, "digest parsed"); - // Unparsed entries remain zero-initialised + // Unparsed scalar entries remain zero-initialised; the PCR bank remains null-initialized. assertEq(p.timestamp, 0, "timestamp not parsed"); - assertEq(p.pcrs.length, 0, "pcrs not parsed"); + assertEq(p.pcrs.length, PCR_BANK_SIZE, "pcr bank initialized"); + assertTrue(p.pcrs[0].isNull(), "pcrs not parsed"); assertEq(p.cert.length(), 0, "cert not parsed"); assertEq(p.cabundle.length, 0, "cabundle not parsed"); } @@ -722,7 +725,8 @@ contract NitroValidatorIndefiniteLengthTest is Test { assertEq(p.timestamp, SYNTH_TIMESTAMP, "timestamp parsed"); // pcrs: empty (indefinite-length map with no entries) - assertEq(p.pcrs.length, 0, "pcrs empty"); + assertEq(p.pcrs.length, PCR_BANK_SIZE, "pcr bank size"); + assertTrue(p.pcrs[0].isNull(), "pcrs empty"); // Fields after pcrs: now parsed — the inner break is consumed, not leaked assertEq(p.cert.length(), SYNTH_CERT_LEN, "cert parsed"); @@ -769,9 +773,10 @@ contract NitroValidatorIndefiniteLengthTest is Test { 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.length, PCR_BANK_SIZE, "pcr bank size"); assertEq(p.pcrs[0].length(), SYNTH_PCR_LEN, "pcr[0] length"); assertEq(p.pcrs[1].length(), SYNTH_PCR_LEN, "pcr[1] length"); + assertTrue(p.pcrs[2].isNull(), "pcr[2] absent"); // entries after pcrs still parsed assertEq(p.cert.length(), SYNTH_CERT_LEN, "cert parsed"); assertEq(p.cabundle.length, SYNTH_CABUNDLE_COUNT, "cabundle parsed"); @@ -836,6 +841,95 @@ contract NitroValidatorIndefiniteLengthTest is Test { _assertDuplicateKnownKeyReverts(duplicatePcrs); } + /// @dev AWS PCR map keys identify PCR slots, not their position in the map. Sparse maps must + /// preserve the key 8 slot and leave the omitted slots explicitly null. + function test_pcrs_sparseMapUsesPcrKeys() public view { + bytes memory pcrs = abi.encodePacked( + hex"6470637273", // key "pcrs" + hex"a6", // map(6) + hex"005830", + new bytes(SYNTH_PCR_LEN), // 0 + hex"015830", + new bytes(SYNTH_PCR_LEN), // 1 + hex"025830", + new bytes(SYNTH_PCR_LEN), // 2 + hex"035830", + new bytes(SYNTH_PCR_LEN), // 3 + hex"045830", + new bytes(SYNTH_PCR_LEN), // 4 + hex"085830", + new bytes(SYNTH_PCR_LEN) // 8 + ); + + NitroValidator.Ptrs memory p = validator.parseAttestation(_buildTbs(abi.encodePacked(hex"a1", pcrs))); + + assertEq(p.pcrs.length, PCR_BANK_SIZE, "pcr bank size"); + for (uint256 i = 0; i <= 4; ++i) { + assertEq(p.pcrs[i].length(), SYNTH_PCR_LEN, "dense PCR prefix"); + } + for (uint256 i = 5; i <= 7; ++i) { + assertTrue(p.pcrs[i].isNull(), "sparse PCR gap"); + } + assertEq(p.pcrs[8].length(), SYNTH_PCR_LEN, "sparse PCR key 8"); + for (uint256 i = 9; i < PCR_BANK_SIZE; ++i) { + assertTrue(p.pcrs[i].isNull(), "trailing PCR absent"); + } + } + + /// @dev A dense map remains indexed by PCR key and uses the same fixed bank representation. + function test_pcrs_denseMapUsesPcrKeys() public view { + bytes memory pcrs = abi.encodePacked( + hex"6470637273", // key "pcrs" + hex"a6", // map(6) + hex"005830", + new bytes(SYNTH_PCR_LEN), // 0 + hex"015830", + new bytes(SYNTH_PCR_LEN), // 1 + hex"025830", + new bytes(SYNTH_PCR_LEN), // 2 + hex"035830", + new bytes(SYNTH_PCR_LEN), // 3 + hex"045830", + new bytes(SYNTH_PCR_LEN), // 4 + hex"055830", + new bytes(SYNTH_PCR_LEN) // 5 + ); + + NitroValidator.Ptrs memory p = validator.parseAttestation(_buildTbs(abi.encodePacked(hex"a1", pcrs))); + + assertEq(p.pcrs.length, PCR_BANK_SIZE, "pcr bank size"); + for (uint256 i = 0; i < 6; ++i) { + assertEq(p.pcrs[i].length(), SYNTH_PCR_LEN, "dense PCR"); + } + assertTrue(p.pcrs[6].isNull(), "PCR after dense map absent"); + } + + function test_neg_duplicatePcrKey_reverts() public { + bytes memory pcrs = abi.encodePacked( + hex"6470637273", // key "pcrs" + hex"a2", // map(2) + hex"005830", + new bytes(SYNTH_PCR_LEN), // 0 + hex"005830", + new bytes(SYNTH_PCR_LEN) // duplicate 0 + ); + + vm.expectRevert("duplicate pcr key"); + validator.parseAttestation(_buildTbs(abi.encodePacked(hex"a1", pcrs))); + } + + function test_neg_outOfRangePcrKey_reverts() public { + bytes memory pcrs = abi.encodePacked( + hex"6470637273", // key "pcrs" + hex"a1", // map(1) + hex"18205830", + new bytes(SYNTH_PCR_LEN) // key 32 is outside 0..31 + ); + + vm.expectRevert("invalid pcr key value"); + validator.parseAttestation(_buildTbs(abi.encodePacked(hex"a1", pcrs))); + } + /// @dev Duplicate certificate must not overwrite the leaf certificate pointer. function test_neg_duplicateCertificate_reverts() public { bytes memory duplicateCertificate = abi.encodePacked(