diff --git a/README.md b/README.md index 1bf8312..b142c57 100644 --- a/README.md +++ b/README.md @@ -174,10 +174,12 @@ integrator (see [docs](docs/hinted-p384-nitro-attestation.md#integrator-responsi 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. + and `cabundle` array are accepted in both definite- and indefinite-length CBOR encodings. Recognised + top-level keys are single-assignment and the signed payload must be fully consumed, so duplicate + parsed fields or trailing signed-but-unparsed bytes revert. 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 f71b3cb..422c2ef 100644 --- a/docs/hinted-p384-nitro-attestation.md +++ b/docs/hinted-p384-nitro-attestation.md @@ -602,15 +602,22 @@ benign format change cannot brick verification until a contract upgrade: 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 +- **Recognised fields are single-assignment.** Duplicate top-level keys that affect the + returned pointers (`pcrs`, `cabundle`, `certificate`, `moduleID`, and the other parsed + attestation fields) are rejected instead of using last-write-wins semantics. Duplicate + unknown keys are still skipped for forward compatibility. +- **The signed payload must be fully consumed.** Definite-length maps cannot leave + trailing bytes after the declared entries, indefinite-length maps cannot leave bytes + after the break marker, and the COSE payload byte string must be the final item in + `attestationTbs`. + +These 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. +`certificate`, `moduleID`, …) must appear at most once. A malformed, truncated, or +partially unparsed encoding reverts instead of silently mis-parsing. ## 11. On-chain demo diff --git a/src/NitroValidator.sol b/src/NitroValidator.sol index 8073aee..79c571a 100644 --- a/src/NitroValidator.sol +++ b/src/NitroValidator.sol @@ -27,6 +27,16 @@ contract NitroValidator { bytes32 public constant NONCE_KEY = 0x7ab1577440dd7bedf920cb6de2f9fc6bf7ba98c78c85a3fa1f8311aac95e1759; // keccak256(bytes("nonce")) bytes32 public constant PCRS_KEY = 0x61585f8bc67a4b6d5891a4639a074964ac66fc2241dc0b36c157dc101325367a; // keccak256(bytes("pcrs")) + uint256 private constant MODULE_ID_SEEN = 1 << 0; + uint256 private constant DIGEST_SEEN = 1 << 1; + uint256 private constant CERTIFICATE_SEEN = 1 << 2; + uint256 private constant PUBLIC_KEY_SEEN = 1 << 3; + uint256 private constant USER_DATA_SEEN = 1 << 4; + uint256 private constant NONCE_SEEN = 1 << 5; + uint256 private constant TIMESTAMP_SEEN = 1 << 6; + uint256 private constant CABUNDLE_SEEN = 1 << 7; + uint256 private constant PCRS_SEEN = 1 << 8; + struct Ptrs { CborElement moduleID; uint64 timestamp; @@ -195,16 +205,20 @@ contract NitroValidator { /// 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. + /// - Recognised top-level keys are single-assignment and the payload must be fully consumed, + /// so duplicate security-critical fields or signed-but-unparsed trailing bytes revert. 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); + require(payload.end() == attestationTbs.length, "trailing tbs data"); 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 seenKeys; uint256 end = payload.end(); for (uint256 entry = 0;; entry++) { if (outerIndefinite) { @@ -227,29 +241,38 @@ contract NitroValidator { current = attestationTbs.nextTextString(current); bytes32 keyHash = attestationTbs.keccak(current); if (keyHash == MODULE_ID_KEY) { + seenKeys = _markAttestationKeySeen(seenKeys, MODULE_ID_SEEN); current = attestationTbs.nextTextString(current); ptrs.moduleID = current; } else if (keyHash == DIGEST_KEY) { + seenKeys = _markAttestationKeySeen(seenKeys, DIGEST_SEEN); current = attestationTbs.nextTextString(current); ptrs.digest = current; } else if (keyHash == CERTIFICATE_KEY) { + seenKeys = _markAttestationKeySeen(seenKeys, CERTIFICATE_SEEN); current = attestationTbs.nextByteString(current); ptrs.cert = current; } else if (keyHash == PUBLIC_KEY_KEY) { + seenKeys = _markAttestationKeySeen(seenKeys, PUBLIC_KEY_SEEN); current = attestationTbs.nextByteStringOrNull(current); ptrs.publicKey = current; } else if (keyHash == USER_DATA_KEY) { + seenKeys = _markAttestationKeySeen(seenKeys, USER_DATA_SEEN); current = attestationTbs.nextByteStringOrNull(current); ptrs.userData = current; } else if (keyHash == NONCE_KEY) { + seenKeys = _markAttestationKeySeen(seenKeys, NONCE_SEEN); current = attestationTbs.nextByteStringOrNull(current); ptrs.nonce = current; } else if (keyHash == TIMESTAMP_KEY) { + seenKeys = _markAttestationKeySeen(seenKeys, TIMESTAMP_SEEN); current = attestationTbs.nextPositiveInt(current); ptrs.timestamp = uint64(current.value()); } else if (keyHash == CABUNDLE_KEY) { + seenKeys = _markAttestationKeySeen(seenKeys, CABUNDLE_SEEN); (ptrs.cabundle, current) = _parseCabundle(attestationTbs, current); } else if (keyHash == PCRS_KEY) { + seenKeys = _markAttestationKeySeen(seenKeys, PCRS_SEEN); (ptrs.pcrs, current) = _parsePcrs(attestationTbs, current); } else { // Forward-compatibility: skip (rather than reject) keys this parser does not @@ -266,6 +289,11 @@ contract NitroValidator { return ptrs; } + function _markAttestationKeySeen(uint256 seenKeys, uint256 keyFlag) private pure returns (uint256) { + require((seenKeys & keyFlag) == 0, "duplicate attestation key"); + return seenKeys | keyFlag; + } + /// @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). diff --git a/test/IndefiniteLengthCbor.t.sol b/test/IndefiniteLengthCbor.t.sol index 2838352..8741dd7 100644 --- a/test/IndefiniteLengthCbor.t.sol +++ b/test/IndefiniteLengthCbor.t.sol @@ -366,6 +366,12 @@ contract NitroValidatorIndefiniteLengthTest is Test { assertTrue(p.nonce.isNull(), "nonce null"); } + function _assertDuplicateKnownKeyReverts(bytes memory duplicateEntry) internal { + bytes memory tbs = _buildTbs(abi.encodePacked(hex"aa", _entries(), duplicateEntry)); + vm.expectRevert("duplicate attestation key"); + validator.parseAttestation(tbs); + } + // ── TBS construction helpers ───────────────────────────── /// @dev Wraps raw CBOR map bytes into a valid attestation-TBS envelope. @@ -783,6 +789,79 @@ contract NitroValidatorIndefiniteLengthTest is Test { _assertSyntheticFields(validator.parseAttestation(tbs)); } + /// @dev Repeated unknown keys are still skipped for forward compatibility. Only duplicate + /// recognised fields are rejected, because those fields affect the returned Ptrs. + function test_unknownDuplicateKeys_areSkipped() public view { + bytes memory unknownEntry = abi.encodePacked( + hex"656578747261", // key "extra" + hex"a10102" // val {1: 2} + ); + bytes memory tbs = _buildTbs(abi.encodePacked(hex"ab", _entries(), unknownEntry, unknownEntry)); + _assertSyntheticFields(validator.parseAttestation(tbs)); + } + + /// @dev Duplicate pcrs must not overwrite the first parsed measurement set. + function test_neg_duplicatePcrs_reverts() public { + bytes memory duplicatePcrs = abi.encodePacked( + hex"6470637273", // key "pcrs" + hex"a1005830", + new bytes(SYNTH_PCR_LEN) + ); + _assertDuplicateKnownKeyReverts(duplicatePcrs); + } + + /// @dev Duplicate certificate must not overwrite the leaf certificate pointer. + function test_neg_duplicateCertificate_reverts() public { + bytes memory duplicateCertificate = abi.encodePacked( + hex"6b6365727469666963617465", // key "certificate" + hex"4401020304" // val bytes(4) + ); + _assertDuplicateKnownKeyReverts(duplicateCertificate); + } + + /// @dev Duplicate cabundle must not overwrite the CA bundle pointer array. + function test_neg_duplicateCabundle_reverts() public { + bytes memory duplicateCabundle = abi.encodePacked( + hex"68636162756e646c65", // key "cabundle" + hex"814401020304" // val [bytes(4)] + ); + _assertDuplicateKnownKeyReverts(duplicateCabundle); + } + + function test_neg_duplicateRequiredScalars_revert() public { + _assertDuplicateKnownKeyReverts(abi.encodePacked(hex"696d6f64756c655f6964", hex"6474657374")); + _assertDuplicateKnownKeyReverts(abi.encodePacked(hex"66646967657374", hex"66534841333834")); + _assertDuplicateKnownKeyReverts(abi.encodePacked(hex"6974696d657374616d70", hex"1a000f4240")); + } + + function test_neg_duplicateOptionalFields_revert() public { + _assertDuplicateKnownKeyReverts(abi.encodePacked(hex"6a7075626c69635f6b6579", hex"f6")); + _assertDuplicateKnownKeyReverts(abi.encodePacked(hex"69757365725f64617461", hex"f6")); + _assertDuplicateKnownKeyReverts(abi.encodePacked(hex"656e6f6e6365", hex"f6")); + } + + /// @dev Definite maps must consume the full payload map; declared-count parsing cannot leave + /// signed bytes after the final parsed value. + function test_neg_definiteOuterMapTrailingData_reverts() public { + bytes memory tbs = _buildTbs(abi.encodePacked(hex"a9", _entries(), hex"00")); + vm.expectRevert("trailing payload bytes"); + validator.parseAttestation(tbs); + } + + /// @dev Indefinite maps must consume the break marker as the final payload byte. + function test_neg_indefiniteOuterMapTrailingDataAfterBreak_reverts() public { + bytes memory tbs = _buildTbs(abi.encodePacked(CBOR_MAP_INDEFINITE, _entries(), CBOR_BREAK, hex"00")); + vm.expectRevert("trailing payload bytes"); + validator.parseAttestation(tbs); + } + + /// @dev The COSE payload byte string must be the last item in attestationTbs. + function test_neg_tbsTrailingBytesAfterPayload_reverts() public { + bytes memory tbs = abi.encodePacked(_buildTbs(abi.encodePacked(hex"a9", _entries())), hex"00"); + vm.expectRevert("trailing tbs data"); + validator.parseAttestation(tbs); + } + /// @dev Indefinite-length map without a trailing 0xFF break marker. /// Parser reads past valid entries into trailing garbage, reverting /// when it encounters a non-text-string byte as the next key.