Harden attestation CBOR parsing

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
 

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
 

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).

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.