Fix certificate cache key malleability (#35)

* fix: canonicalize certificate cache keys

Key non-root certificate verification cache entries by the signed tbsCertificate instead of the full DER bytes so ECDSA signature malleability cannot create shadow cache identities. Keep the pinned root under ROOT_CA_CERT_HASH and reject malleable aliases of that trust anchor.

Add regression coverage for malleated certificate signatures and update docs/scripts to use the returned cache key.

* fix: use canonical cert ids in hint plans

Author: leanthebean

README.md

@@ -53,6 +53,10 @@ own document signature.
      `ROOT_CA_CERT_HASH` / `keccak256(rootCert)` for the first non-root CA; `0` is only for the
      pinned root itself)
    - `certManager.verifyClientCertWithHints(leafCert, parentCertHash, hints)`
+   Use each CA verification's returned hash as the `parentCertHash` for the next child. For non-root
+   certs this cache key is `keccak256(tbsCertificate)`, not `keccak256(certBytes)`, so malleable
+   signature encodings share one cache entry. Leaf verification returns metadata, not a child
+   `parentCertHash`.
 2. **Validation — verify the document signature.** Once the chain is cached:
    - `validator.validateAttestationWithHints(attestationTbs, signature, attestationHints)`
    - **Precondition:** the whole `cabundle` + leaf must already be cached from phase 1. This call
@@ -93,8 +97,8 @@ contains a revoked certificate.
   `ROOT_CA_CERT_HASH` as an emergency global halt (the root is identified by its pinned hash, since
   it is never parsed on-chain).
 - The revoker can call `revokeCert` or `revokeCerts` for non-root certificate identity keys.
-- `loadVerified` is a raw cache read; returned metadata does not imply the certificate is
-  currently trusted.
+- `loadVerified` is a raw cache read by verification cache key; returned metadata does not imply
+  the certificate is currently trusted.
 
 ### Example consumer
 
@@ -164,7 +168,8 @@ integrator (see [docs](docs/hinted-p384-nitro-attestation.md#integrator-responsi
   to `block.timestamp` (seconds) or match the `nonce` to a challenge. Enforce freshness yourself if
   you need it.
 - **Signature malleability** — low-S is not enforced (AWS does not emit low-S), so the malleable
-  twin `(r, n-s)` also verifies. Never use the signature as a uniqueness key; dedupe on canonical
+  twin `(r, n-s)` also verifies. Certificate caching excludes the outer signature bytes, but
+  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.

docs/hinted-p384-nitro-attestation.md

@@ -361,8 +361,9 @@ document signature.
 Once the leaf and its CA chain are cached and unexpired, a later attestation signed by
 the same leaf is a **single transaction** (`validateAttestationWithHints`) carrying
 only the COSE signature hints. The cabundle certs are not re-verified — they are
-reloaded by `keccak256(cert)` identity, checked against their original cached parent,
-and their cached metadata is re-checked.
+reloaded by verification cache key (`ROOT_CA_CERT_HASH` for the pinned root,
+`keccak256(tbsCertificate)` for non-root certs), checked against their original cached
+parent, and their cached metadata is re-checked.
 
 Practical reuse cases:
 
@@ -371,12 +372,12 @@ Practical reuse cases:
   document → 2 transactions;
 - **CA chain and leaf cached:** validate the document only → 1 transaction.
 
-**Cache reuse** is allowed when: the submitted DER hashes to a cached cert; the cert
-is unexpired (`notAfter ≥ block.timestamp`); the cached CA/client role matches; and
-`parentCertHash` matches the parent used during cold verification; and neither the cert
-nor its cached parent chain is revoked. The cache is global on-chain state — once any
-caller verifies a cert, others reuse it until expiry or revocation, but only under the
-same parent binding.
+**Cache reuse** is allowed when: the submitted DER contains a TBS that hashes to a
+cached cert; the cert is unexpired (`notAfter ≥ block.timestamp`); the cached CA/client
+role matches; `parentCertHash` matches the parent used during cold verification; and
+neither the cert nor its cached parent chain is revoked. The cache is global on-chain
+state — once any caller verifies a cert, others reuse it until expiry or revocation,
+but only under the same parent binding.
 
 ### Revocation model
 AWS's Nitro attestation documentation disables CRL checking in its sample validation
@@ -410,23 +411,24 @@ cached descendant cannot keep verifying through an ancestor that was later revok
 Revocation is checked independently of `notAfter`, so a revoked cert is untrusted even if
 its X.509 validity period has not expired.
 
-`loadVerified` is intentionally a raw cache read. A non-empty return value means the cert
-metadata was cached previously; it does not imply the cert is currently trusted, unexpired,
-or unrevoked.
+`loadVerified` is intentionally a raw cache read by verification cache key. A non-empty
+return value means the cert metadata was cached previously; it does not imply the cert is
+currently trusted, unexpired, or unrevoked.
 
 **First-verified parent pinning.** A cached cert is pinned to the parent it was first
-verified under: cold verification records `verifiedParent[certHash]` once, and every
-later warm reuse requires the caller to present that exact `parentCertHash` (a mismatch
-reverts with `parent cert mismatch`). This is a deliberate, conservative binding — warm
-reuse skips signature verification, so it must reflect the precise chain that was
-cryptographically checked, not merely *a* valid same-subject issuer. The liveness
-consequence is that if the same certificate is genuinely issued under two different CA
-objects (for example a same-key CA renewal that produces new DER bytes, hence a new
-parent hash), the cached leaf keeps verifying only through its first parent; a second
-caller chaining it through the renewed parent must wait for the cached entry to expire.
-For AWS Nitro this is effectively a non-issue because leaf certificates are short-lived
-(~3h) and expire long before their issuing CA is rotated, so the binding self-heals; it
-is documented here as a known edge rather than a fixed bug.
+verified under: cold verification records `verifiedParent[certHash]` once, where
+`certHash` is the canonical cache key, and every later warm reuse requires the caller to
+present that exact `parentCertHash` (a mismatch reverts with `parent cert mismatch`).
+This is a deliberate, conservative binding — warm reuse skips signature verification,
+so it must reflect the precise chain that was cryptographically checked, not merely *a*
+valid same-subject issuer. The liveness consequence is that if the same certificate is
+genuinely issued under two different CA objects (for example a same-key CA renewal that
+produces a new TBS, hence a new parent hash), the cached leaf keeps verifying only
+through its first parent; a second caller chaining it through the renewed parent must
+wait for the cached entry to expire. For AWS Nitro this is effectively a non-issue
+because leaf certificates are short-lived (~3h) and expire long before their issuing CA
+is rotated, so the binding self-heals; it is documented here as a known edge rather than
+a fixed bug.
 
 **Warm-only guard.** `validateAttestationWithHints` re-runs the cabundle checks with an
 *empty* hint stream. Cached certs return before signature verification; a missing cert

script/BaseSepoliaDemo.s.sol

@@ -2,6 +2,7 @@
 pragma solidity ^0.8.26;
 
 import {Script, console2} from "forge-std/Script.sol";
+import {Asn1Decode, LibAsn1Ptr, Asn1Ptr} from "../src/Asn1Decode.sol";
 import {CborDecode, CborElement, LibCborElement} from "../src/CborDecode.sol";
 import {CertManagerDemo} from "../test/helpers/CertManagerDemo.sol";
 import {ICertManager} from "../src/ICertManager.sol";
@@ -20,7 +21,9 @@ contract NitroValidatorScriptParser is NitroValidator {
 
 /// @dev Uses vm.ffi to run the off-chain hint tools; invoke the script with Foundry's `--ffi` flag.
 contract BaseSepoliaDemo is Script {
+    using Asn1Decode for bytes;
     using CborDecode for bytes;
+    using LibAsn1Ptr for Asn1Ptr;
     using LibBytes for bytes;
     using LibCborElement for CborElement;
 
@@ -97,7 +100,13 @@ contract BaseSepoliaDemo is Script {
         leaf = certManager.verifyClientCertWithHints(clientCert, parentHash, clientHints);
         require(leaf.pubKey.length > 0, "leaf not cached");
         console2.log("cached client cert");
-        console2.logBytes32(keccak256(clientCert));
+        console2.logBytes32(_certCacheKey(clientCert));
+    }
+
+    function _certCacheKey(bytes memory certificate) internal pure returns (bytes32) {
+        Asn1Ptr root = certificate.root();
+        Asn1Ptr tbsCertPtr = certificate.firstChildOf(root);
+        return certificate.keccak(tbsCertPtr.header(), tbsCertPtr.totalLength());
     }
 
     function _loadAttestation() internal returns (bytes memory) {

src/CertManager.sol

@@ -48,7 +48,8 @@ contract CertManager is ICertManager {
     bytes32 public constant BASIC_CONSTRAINTS_OID = 0x6351d72a43cb42fb9a2531a28608c278c89629f8f025b5f5dc705f3fe45e950a; // keccak256(hex"551d13")
     bytes32 public constant KEY_USAGE_OID = 0x45529d8772b07ebd6d507a1680da791f4a2192882bf89d518801579f7a5167d2; // keccak256(hex"551d0f")
 
-    // certHash -> VerifiedCert
+    // certHash -> VerifiedCert. The root is keyed by ROOT_CA_CERT_HASH; every non-root cert is keyed
+    // by keccak256(tbsCertificate), excluding the outer malleable ECDSA signature bytes.
     mapping(bytes32 => bytes) public verified;
     // certHash -> parent cert hash used during cold verification.
     // A cached cert is pinned to the parent it was FIRST verified under: warm reuse requires the
@@ -120,12 +121,13 @@ contract CertManager is ICertManager {
     ///      match the parent used during cold verification. On a cold cert, `signatureHints` must
     ///      contain the real off-chain inverse hints for the cert signature; they are re-verified
     ///      on-chain, so a wrong hint only reverts. Pass 0 only when submitting the pinned root;
-    ///      otherwise pass the cached parent cert hash.
+    ///      otherwise pass the cached parent cert hash. The returned hash is ROOT_CA_CERT_HASH for
+    ///      the pinned root and keccak256(tbsCertificate) for every non-root cert.
     function verifyCACertWithHints(bytes memory cert, bytes32 parentCertHash, bytes memory signatureHints)
         external
         returns (bytes32)
     {
-        bytes32 certHash = keccak256(cert);
+        bytes32 certHash = _certCacheKey(cert);
         _verifyCert(cert, certHash, true, parentCertHash, signatureHints);
         return certHash;
     }
@@ -138,12 +140,13 @@ contract CertManager is ICertManager {
         external
         returns (VerifiedCert memory)
     {
-        return _verifyCert(cert, keccak256(cert), false, parentCertHash, signatureHints);
+        return _verifyCert(cert, _certCacheKey(cert), false, parentCertHash, signatureHints);
     }
 
     /// @notice Return raw cached certificate metadata without current trust checks.
     /// @dev A non-empty return value only means the cert was cached previously. It may now be
-    ///      expired or revoked; use the verification entrypoints for trust-aware reuse.
+    ///      expired or revoked; use the verification entrypoints for trust-aware reuse. Pass the
+    ///      cache key returned by the verification entrypoints.
     function loadVerified(bytes32 certHash) external view returns (VerifiedCert memory) {
         return _loadVerified(certHash);
     }
@@ -262,6 +265,9 @@ contract CertManager is ICertManager {
 
         bytes32 identity;
         (cert, identity) = _verifyUncachedCert(certificate, ca, parent, signatureHints);
+        // The pinned root is already present under ROOT_CA_CERT_HASH. Do not allow a signature
+        // malleability twin of that same trust anchor to become a second cached parent key.
+        require(!_isPinnedRootAlias(certHash, cert), "root cert alias");
         // Reject by (issuer, serial) identity so a re-encoded twin of a revoked cert cannot pass.
         _requireNotRevoked(identity);
         _saveVerified(certHash, cert);
@@ -273,6 +279,24 @@ contract CertManager is ICertManager {
         return cert;
     }
 
+    function _certCacheKey(bytes memory certificate) internal pure returns (bytes32) {
+        bytes32 rawCertHash = keccak256(certificate);
+        if (rawCertHash == ROOT_CA_CERT_HASH) {
+            return ROOT_CA_CERT_HASH;
+        }
+
+        Asn1Ptr root = certificate.root();
+        require(root.totalLength() == certificate.length, "invalid cert length");
+        Asn1Ptr tbsCertPtr = certificate.firstChildOf(root);
+        return certificate.keccak(tbsCertPtr.header(), tbsCertPtr.totalLength());
+    }
+
+    function _isPinnedRootAlias(bytes32 certHash, VerifiedCert memory cert) internal pure returns (bool) {
+        return certHash != ROOT_CA_CERT_HASH && cert.ca && cert.subjectHash == ROOT_CA_CERT_SUBJECT_HASH
+            && cert.pubKey.length == ROOT_CA_CERT_PUB_KEY.length
+            && keccak256(cert.pubKey) == keccak256(ROOT_CA_CERT_PUB_KEY);
+    }
+
     function _verifyUncachedCert(
         bytes memory certificate,
         bool ca,

src/ICertManager.sol

@@ -18,6 +18,8 @@ interface ICertManager {
 
     function revoked(bytes32 certHash) external view returns (bool);
 
+    /// @return The cache key to use as the parent for child certs: ROOT_CA_CERT_HASH for the pinned
+    ///         root, otherwise keccak256(tbsCertificate).
     function verifyCACertWithHints(bytes memory cert, bytes32 parentCertHash, bytes memory signatureHints)
         external
         returns (bytes32);
@@ -26,7 +28,7 @@ interface ICertManager {
         external
         returns (VerifiedCert memory);
 
-    /// @notice Raw cache read. A returned cert may be expired or revoked.
+    /// @notice Raw cache read by verification cache key. A returned cert may be expired or revoked.
     function loadVerified(bytes32 certHash) external view returns (VerifiedCert memory);
 
     function isRevoked(bytes32 certHash) external view returns (bool);