From 9a573e9b640b3c4b825f0de38d57bb9905e6cd49 Mon Sep 17 00:00:00 2001 From: Elena Nadolinski Date: Thu, 18 Jun 2026 13:26:50 -0700 Subject: [PATCH] fix: reject duplicate attestation fields Reject duplicate recognized top-level attestation keys in NitroValidator instead of using last-write-wins semantics for pcrs, certificate, cabundle, and other parsed fields. Require the COSE payload byte string and outer payload map to be fully consumed so signed trailing bytes cannot be silently ignored. Keep unknown keys skippable for AWS forward compatibility. Add regression coverage for duplicate parsed fields, unknown duplicate compatibility, and trailing signed bytes. --- README.md | 10 ++-- docs/hinted-p384-nitro-attestation.md | 17 ++++-- src/NitroValidator.sol | 28 ++++++++++ test/IndefiniteLengthCbor.t.sol | 79 +++++++++++++++++++++++++++ 4 files changed, 125 insertions(+), 9 deletions(-) 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.