diff --git a/README.md b/README.md index 69d72b0..df397c9 100644 --- a/README.md +++ b/README.md @@ -143,6 +143,11 @@ integrator (see [docs](docs/hinted-p384-nitro-attestation.md#integrator-responsi responsibility. - **Revocation** — not supported (consistent with AWS's documented validation process, linked above). +- **Forward-compatibility** — the attestation parser uses a fixed key whitelist (unknown keys + revert) and does not support nested indefinite-length `pcrs`/`cabundle`. A future change to the + AWS attestation format would make verification revert (never a false accept) until the contract + is upgraded. AWS emits the known, definite-length format today. See + [docs §10](docs/hinted-p384-nitro-attestation.md#forward-compatibility-attestation-format-changes). ## Build diff --git a/docs/hinted-p384-nitro-attestation.md b/docs/hinted-p384-nitro-attestation.md index efaf1cd..3933133 100644 --- a/docs/hinted-p384-nitro-attestation.md +++ b/docs/hinted-p384-nitro-attestation.md @@ -503,6 +503,30 @@ can never silently diverge. must stay in sync with the verifier's execution order. Its DER/CBOR parsing should be reviewed for robustness. +### Forward-compatibility (attestation format changes) + +The attestation parser accepts a strict, fixed shape: a known whitelist of CBOR keys, and +definite-length or outer-indefinite-length maps. The two consequences below are deliberate +parser behaviour and are **liveness-only** — a wrong or unrecognised shape can only cause a +revert (a genuine attestation failing to verify), never a false accept. They are recorded here +because the project does not otherwise document them as an accepted trade-off: each would brick +verification after a future AWS attestation-format change, and resolving it needs a contract +upgrade. + +- **Unknown attestation key → revert (`"invalid attestation key"`).** If AWS adds a new field to + the attestation document, every attestation carrying it becomes unverifiable. Tolerating unknown + keys would be safe — all fields sit under AWS's COSE signature, so ignoring an unrecognised one + cannot forge anything — but the parser has no generic CBOR skip routine. Current behaviour is + pinned by `test_neg_unknownKey{Definite,Indefinite}_reverts`; the desired forward-compatible + behaviour is captured (skipped) in `test_unknownKey_forwardCompat_tolerated`. +- **Nested indefinite-length `pcrs` / `cabundle` → revert / empty parse.** Outer-map + indefinite-length encoding is supported (via the `0xFF` break marker); nested indefinite-length + containers are not. Pinned by `test_edge_innerIndefinitePcrsEmpty_outerBreakTriggered` and + `test_neg_nestedIndefiniteNonEmptyArray_reverts`; desired behaviour (skipped) in + `test_nestedIndefinitePcrs_forwardCompat_parsed`. + +AWS currently emits definite-length CBOR with the known field set, so neither is triggered today. + ### Integrator responsibilities (what the contract does NOT enforce) Verification proves an attestation is genuine and well-formed. The following are diff --git a/src/NitroValidator.sol b/src/NitroValidator.sol index a760895..4b230ad 100644 --- a/src/NitroValidator.sol +++ b/src/NitroValidator.sol @@ -188,6 +188,13 @@ contract NitroValidator { LibBytes.memcpy(dest + 13 + rawProtectedLength, payloadSrc, rawPayloadLength); } + /// @dev Parses the COSE payload into field pointers using a strict shape: any unrecognised + /// CBOR key reverts ("invalid attestation key"), and nested indefinite-length + /// `pcrs`/`cabundle` are not supported (outer-map indefinite-length IS). These are + /// liveness-only constraints — they can only cause a revert, never a false accept — but + /// they make verification forward-INCOMPATIBLE with attestation-format changes: a new AWS + /// field would require a contract upgrade. Tolerating unknown keys would be safe (every + /// field is under AWS's COSE signature). See docs §10 "Forward-compatibility". function _parseAttestation(bytes memory attestationTbs) internal pure returns (Ptrs memory) { require(attestationTbs.keccak(0, 18) == ATTESTATION_TBS_PREFIX, "invalid attestation prefix"); diff --git a/test/CertManager.t.sol b/test/CertManager.t.sol index 60ba0c6..8e32339 100644 --- a/test/CertManager.t.sol +++ b/test/CertManager.t.sol @@ -80,6 +80,21 @@ contract CertManagerTest is Test { return cm.verifyCACertWithHints(cert, parentHash, hints); } + // Cache-griefing liveness edge: `verifiedParent[certHash]` is written once on the cold + // path (gated on cert.pubKey.length == 0) and never updated. If AWS renews an intermediate + // CA with the SAME signing key but a new validity window, the renewed cert has different + // DER bytes -> a different keccak256 -> a different parentCertHash in the chain. A leaf + // already cached under the old parent then permanently reverts "parent cert mismatch" + // against the renewed parent, with no admin override. (The wrong-parent revert mechanism + // itself is covered by HintedNitroAttestationTest.test_Hinted*RejectsCachedParentMismatch.) + // + // SKIPPED: realising this needs a genuine same-key *renewed* CA cert, which requires an + // AWS signing operation and is unavailable as a static fixture. Documented as a known + // liveness edge; see security review redteam/r3-cache-replay-malleability.md (Claim 2). + function test_CacheGriefingSameKeyCaRenewalBricksCachedLeaf() public { + vm.skip(true, "needs a same-key-renewed AWS CA fixture (off-chain signing); documents verifiedParent first-writer-wins"); + } + function testFuzz_uint384At_LeadingZeros(uint8 numZeros, uint128 hiSeed, uint256 loSeed) public view { numZeros = uint8(bound(numZeros, 0, 16)); diff --git a/test/IndefiniteLengthCbor.t.sol b/test/IndefiniteLengthCbor.t.sol index 90b0059..5c32ebf 100644 --- a/test/IndefiniteLengthCbor.t.sol +++ b/test/IndefiniteLengthCbor.t.sol @@ -614,4 +614,57 @@ contract NitroValidatorIndefiniteLengthTest is Test { vm.expectRevert("unexpected type"); validator.parseAttestation(_buildTbs(abi.encodePacked(CBOR_MAP_INDEFINITE, entries, CBOR_BREAK))); } + + // ══════════════════════════════════════════════════════════ + // FORWARD-COMPATIBILITY — known latent liveness risks (SKIPPED) + // + // The two reverts above are deliberate parser behaviour, but the project does not + // document the forward-compatibility consequence as an accepted trade-off. They are + // liveness-only (a wrong/unknown shape can only revert, never false-accept), yet a + // future change to the AWS attestation format would brick verification until the + // contract is upgraded. The tests below encode the DESIRED forward-compatible + // behaviour and are skipped until the parser supports it (see docs §10 + // "Forward-compatibility"). + // ══════════════════════════════════════════════════════════ + + /// @dev SKIPPED. Today an unknown attestation key reverts (pinned by + /// test_neg_unknownKeyDefinite_reverts). Tolerating it would be SAFE — every field is + /// under AWS's COSE signature, so ignoring an unrecognised key cannot forge anything; + /// the parser simply lacks a generic CBOR skip. If AWS adds any new field, every such + /// attestation becomes unverifiable until a contract upgrade. + function test_unknownKey_forwardCompat_tolerated() public { + vm.skip(true, "latent liveness risk: unknown attestation key reverts; needs a generic CBOR skip to tolerate new AWS fields"); + + // Desired: a known field still parses and the unknown key is ignored. + bytes memory entries = abi.encodePacked( + hex"696d6f64756c655f6964", // key "module_id" + hex"6474657374", // val "test" + hex"63626164", // key "bad" (unknown) + hex"6474657374" // val "test" + ); + NitroValidator.Ptrs memory p = validator.parseAttestation(_buildTbs(abi.encodePacked(hex"a2", entries))); + assertEq(p.moduleID.length(), SYNTH_MODULE_ID_LEN, "known field parsed; unknown key ignored"); + } + + /// @dev SKIPPED. Outer-map indefinite-length is supported, but a NESTED indefinite-length + /// pcrs map is not (pinned by test_edge_innerIndefinitePcrsEmpty_outerBreakTriggered and + /// test_neg_nestedIndefiniteNonEmptyArray_reverts). If AWS emitted nested + /// indefinite-length containers, verification would brick. This encodes the desired parse. + function test_nestedIndefinitePcrs_forwardCompat_parsed() public { + vm.skip(true, "latent liveness risk: nested indefinite-length pcrs/cabundle unsupported; would brick on an AWS encoding change"); + + // Desired: a nested indefinite-length pcrs map {0: bytes(48)} parses to one PCR. + bytes memory entries = abi.encodePacked( + hex"696d6f64756c655f6964", // key "module_id" + hex"6474657374", // val "test" + hex"6470637273", // key "pcrs" + CBOR_MAP_INDEFINITE, // nested indefinite-length map + hex"00", // key 0 + hex"5830", // val bytes(48) header + new bytes(48), // 48 zero bytes + CBOR_BREAK // inner break + ); + NitroValidator.Ptrs memory p = validator.parseAttestation(_buildTbs(abi.encodePacked(hex"a2", entries))); + assertEq(p.pcrs.length, 1, "nested indefinite-length pcrs should yield 1 PCR"); + } } diff --git a/test/hinted/HintedNitroAttestation.t.sol b/test/hinted/HintedNitroAttestation.t.sol index 44f35a2..0ae7fdc 100644 --- a/test/hinted/HintedNitroAttestation.t.sol +++ b/test/hinted/HintedNitroAttestation.t.sol @@ -10,6 +10,64 @@ import {CborDecode} from "../../src/CborDecode.sol"; import {P384Verifier} from "../../src/P384Verifier.sol"; import {Sha2Ext} from "../../src/Sha2Ext.sol"; import {P384HintCollector} from "../helpers/HintedNitroTestHelpers.sol"; +import {U384} from "../../src/vendor/ECDSA384.sol"; + +/// @dev Thin wrapper exposing U384's internal modinv / moddivAssign so hint-soundness +/// properties (non-canonical inverses, modulus confusion) can be tested in isolation. +contract U384TestWrapper { + /// @dev Canonical b^{-1} mod m via the non-hinted Fermat path; returns 48-byte big-endian. + function computeModinvNoHint(bytes memory bBytes, bytes memory mBytes) + external + view + returns (bytes memory result) + { + uint256 m = U384.init(mBytes); + uint256 b = U384.init(bBytes); + uint256 call_ = U384.initCall(m); + result = _u384ToBytes(U384.modinv(call_, b, m)); + } + + /// @dev modinv(b, m) consuming a single 48-byte hint; reverts "bad inverse hint" unless + /// b * hint == 1 (mod m). Returns the raw (possibly non-canonical) hint value. + function callModinvWithHint(bytes memory bBytes, bytes memory mBytes, bytes memory hint) + external + view + returns (bytes memory result) + { + require(hint.length == 48, "hint must be 48 bytes"); + uint256 m = U384.init(mBytes); + uint256 b = U384.init(bBytes); + uint256 call_ = U384.initCallWithHints(m, hint); + result = _u384ToBytes(U384.modinv(call_, b, m)); + } + + /// @dev a/b mod p via moddivAssign using a single 48-byte hint for b^{-1} (p is the baked + /// MODEXP modulus). Reverts "bad inverse hint" if b * hint != 1 (mod p). + function callModdivAssignWithHint( + bytes memory aBytes, + bytes memory bBytes, + bytes memory pBytes, + bytes memory hint + ) external view returns (bytes memory result) { + require(hint.length == 48, "hint must be 48 bytes"); + uint256 p = U384.init(pBytes); + uint256 a = U384.init(aBytes); + uint256 b = U384.init(bBytes); + uint256 call_ = U384.initCallWithHints(p, hint); + U384.moddivAssign(call_, a, b); + result = _u384ToBytes(a); + } + + /// @dev Serialise a U384 pointer (64-byte slot) to 48-byte big-endian bytes. + function _u384ToBytes(uint256 ptr) internal pure returns (bytes memory result) { + result = new bytes(48); + assembly { + let dst := add(result, 0x20) + mstore(dst, shl(128, mload(ptr))) + mstore(add(dst, 0x10), mload(add(ptr, 0x20))) + } + } +} contract NitroValidatorParseHarness is NitroValidator { constructor(CertManager certManager, P384Verifier p384Verifier) NitroValidator(certManager, p384Verifier) {} @@ -31,6 +89,20 @@ contract HintedNitroAttestationTest is Test { NitroValidatorParseHarness parser; P384Verifier p384Verifier; P384HintCollector hintCollector; + U384TestWrapper u384; + + // P-384 field modulus p (48-byte big-endian). + bytes constant CURVE_P = + hex"fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffeffffffff0000000000000000ffffffff"; + // P-384 group order n (48-byte big-endian). + bytes constant CURVE_N = + hex"ffffffffffffffffffffffffffffffffffffffffffffffffc7634d81f4372ddf581a0db248b0a77aecec196accc52973"; + // 7 + p (7 < 2^128, so this fits in 48 bytes): a non-canonical representative of 7 mod p. + bytes constant SEVEN_PLUS_P = + hex"fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffeffffffff000000000000000100000006"; + // P-384 order split into (hi128, lo256) for forming the malleable twin (r, n-s). + uint128 constant N_HI = type(uint128).max; + uint256 constant N_LO = 0xffffffffffffffffc7634d81f4372ddf581a0db248b0a77aecec196accc52973; struct SequenceSummary { ICertManager.VerifiedCert leaf; @@ -52,6 +124,159 @@ contract HintedNitroAttestationTest is Test { validator = new NitroValidator(certManager, p384Verifier); parser = new NitroValidatorParseHarness(certManager, p384Verifier); hintCollector = new P384HintCollector(); + u384 = new U384TestWrapper(); + } + + // ───────────────────────────────────────────────────────────────────────────── + // Hint-verification soundness & signature semantics (from the security review) + // ───────────────────────────────────────────────────────────────────────────── + + /// @dev A non-canonical inverse hint (inv + p, still < 2^384) passes the on-chain + /// `b·inv ≡ 1 (mod p)` check AND yields the identical reduced result as the + /// canonical hint, so unreduced hints can never change the accept decision. + /// Exercised at the U384 level: real 384-bit inverses are ~2^384 and their + /// `inv + p` would overflow 48 bytes, so a small inverse (7) is used to make the + /// non-canonical representative constructible. + function test_UnreducedInverseHintIsSound() public view { + bytes memory seven = new bytes(48); + seven[47] = 0x07; + bytes memory one = new bytes(48); + one[47] = 0x01; + + // b = 7^{-1} mod p, so the canonical inverse of b is 7. + bytes memory b = u384.computeModinvNoHint(seven, CURVE_P); + + // modinv accepts both the canonical hint (7) and the non-canonical (7 + p), returning each verbatim. + assertEq(u384.callModinvWithHint(b, CURVE_P, seven), seven, "canonical hint accepted"); + assertEq(u384.callModinvWithHint(b, CURVE_P, SEVEN_PLUS_P), SEVEN_PLUS_P, "unreduced hint accepted"); + + // moddivAssign(1, b) = 1/b mod p = 7 for BOTH hints — identical reduced result. + bytes memory canon = u384.callModdivAssignWithHint(one, b, CURVE_P, seven); + bytes memory unreduced = u384.callModdivAssignWithHint(one, b, CURVE_P, SEVEN_PLUS_P); + assertEq(canon, seven, "canonical hint computes 7"); + assertEq(unreduced, seven, "unreduced hint computes 7"); + assertEq(canon, unreduced, "unreduced hint yields identical result -> cannot affect accept"); + } + + /// @dev A hint that is a valid inverse modulo n is rejected when consumed where modulo p + /// is expected. Guards against modulus confusion between the mod-n (scalar) and + /// mod-p (field) inversion paths. + function test_ModulusConfusionHintReverts() public { + bytes memory seven = new bytes(48); + seven[47] = 0x07; + + bytes memory inv7modN = u384.computeModinvNoHint(seven, CURVE_N); + // Correct modulus: accepted. + assertEq(u384.callModinvWithHint(seven, CURVE_N, inv7modN), inv7modN, "mod-n hint valid for mod-n"); + // Wrong modulus: 7 * (7^{-1} mod n) mod p != 1 -> rejected. + vm.expectRevert("bad inverse hint"); + u384.callModinvWithHint(seven, CURVE_P, inv7modN); + } + + /// @dev With a corrupted attestation signature, NO attacker-chosen hint stream forces + /// acceptance: empty / garbage / the original-valid / padded hints all revert. `s` + /// is corrupted (not `r`) so range checks pass and the first scalar inversion's hint + /// check is the gate that fires. + function test_CorruptedSignatureNeverAccepts() public { + bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); + (bytes memory attestationTbs, bytes memory signature) = validator.decodeAttestationTbs(attestation); + ICertManager.VerifiedCert memory leaf = _cacheCertBundleWithHints(attestationTbs); + bytes memory hash = Sha2Ext.sha384(attestationTbs, 0, attestationTbs.length); + + bytes memory validHints = hintCollector.collectVerifyHints(hash, signature, leaf.pubKey); + + bytes memory corrupted = new bytes(signature.length); + for (uint256 i = 0; i < signature.length; ++i) { + corrupted[i] = signature[i]; + } + corrupted[48] = bytes1(uint8(corrupted[48]) ^ 0x01); // flip MSB of s + + vm.expectRevert("inverse hint underflow"); + p384Verifier.verifyP384SignatureWithHints(hash, corrupted, leaf.pubKey, ""); + + vm.expectRevert("bad inverse hint"); + p384Verifier.verifyP384SignatureWithHints(hash, corrupted, leaf.pubKey, new bytes(48)); + + vm.expectRevert("bad inverse hint"); + p384Verifier.verifyP384SignatureWithHints(hash, corrupted, leaf.pubKey, validHints); + + vm.expectRevert("bad inverse hint"); + p384Verifier.verifyP384SignatureWithHints( + hash, corrupted, leaf.pubKey, abi.encodePacked(validHints, new bytes(48)) + ); + } + + /// @dev Signature malleability is intentional (CURVE_LOW_S_MAX = n-1): for a valid (r, s) + /// the twin (r, n-s) also verifies. Both are accepted and decode to the identical + /// attestation, so integrators must NOT use raw signature bytes as a uniqueness key. + function test_MalleableTwinSignatureAccepted() public { + bytes memory attestationTbs; + bytes memory sig; + { + bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); + (attestationTbs, sig) = validator.decodeAttestationTbs(attestation); + } + ICertManager.VerifiedCert memory leaf = _cacheCertBundleWithHints(attestationTbs); + bytes memory hash = Sha2Ext.sha384(attestationTbs, 0, attestationTbs.length); + + bytes memory twin = _computeTwinSig(sig); + assertTrue(keccak256(sig) != keccak256(twin), "twin must differ from original"); + + bytes memory hints = hintCollector.collectVerifyHints(hash, sig, leaf.pubKey); + bytes memory twinHints = hintCollector.collectVerifyHints(hash, twin, leaf.pubKey); + + assertTrue(p384Verifier.verifyP384SignatureWithHints(hash, sig, leaf.pubKey, hints), "original verifies"); + assertTrue( + p384Verifier.verifyP384SignatureWithHints(hash, twin, leaf.pubKey, twinHints), + "malleable twin (r, n-s) also verifies" + ); + + NitroValidator.Ptrs memory p1 = validator.validateAttestationWithHints(attestationTbs, sig, hints); + NitroValidator.Ptrs memory p2 = validator.validateAttestationWithHints(attestationTbs, twin, twinHints); + assertEq(p1.timestamp, p2.timestamp, "both twins decode to the same attestation"); + } + + /// @dev validateAttestationWithHints has no on-chain anti-replay: the same (tbs, sig) + /// verifies repeatedly until the leaf cert expires. Freshness/replay protection is + /// the integrator's responsibility (documented in NatSpec). + function test_AttestationReplayHasNoOnChainProtection() public { + bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); + (bytes memory attestationTbs, bytes memory sig) = validator.decodeAttestationTbs(attestation); + ICertManager.VerifiedCert memory leaf = _cacheCertBundleWithHints(attestationTbs); + bytes memory hash = Sha2Ext.sha384(attestationTbs, 0, attestationTbs.length); + bytes memory hints = hintCollector.collectVerifyHints(hash, sig, leaf.pubKey); + + uint256 ts1 = validator.validateAttestationWithHints(attestationTbs, sig, hints).timestamp; + uint256 ts2 = validator.validateAttestationWithHints(attestationTbs, sig, hints).timestamp; + assertEq(ts1, ts2, "same attestation accepted twice (no replay protection)"); + } + + /// @dev Builds the malleable twin (r, n-s) from a 96-byte packed (r||s) signature. + function _computeTwinSig(bytes memory sig) internal pure returns (bytes memory twin) { + require(sig.length == 96, "sig must be 96 bytes"); + uint128 rhi; + uint256 rlo; + uint128 shi; + uint256 slo; + assembly { + let data := add(sig, 0x20) + rhi := shr(128, mload(data)) + rlo := mload(add(data, 0x10)) + shi := shr(128, mload(add(data, 0x30))) + slo := mload(add(data, 0x40)) + } + uint128 thi; + uint256 tlo; + if (N_LO >= slo) { + tlo = N_LO - slo; + thi = N_HI - shi; + } else { + unchecked { + tlo = N_LO - slo; + } + thi = N_HI - shi - 1; + } + twin = abi.encodePacked(rhi, rlo, thi, tlo); } function test_HintedAttestationRejectsSurplusHint() public {