diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bc2119..29b1b31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,25 @@ All notable changes to this project are documented here. The format is based on ## [Unreleased] +### Added +- Operational certificate revocation in `CertManager`: an owner-managed `revoker` can mark one or + many certificates revoked, and the owner can rotate the revoker or undo accidental revocations. +- `CertManager.computeCertId(certDER)`: returns a certificate's `(issuer, serial)` revocation + identity key. + +### Changed +- Certificate verification and cached reuse now reject revoked certificates and revoked cached + parent-chain ancestors independently of `notAfter`. +- Revocation is keyed by the `(issuer, serial)` identity `keccak256(issuerHash, serialHash)` (what + AWS CRLs use), not by `keccak256(certBytes)`. Byte-keying was bypassable, because ECDSA signature + malleability and DER re-encoding let a revoked certificate be re-presented with different bytes + that still verify; the signature-protected identity closes that gap and lets operators revoke + directly from CRL issuer/serial entries. +- Root certificate revocation is owner-only (keyed by the pinned `ROOT_CA_CERT_HASH`, since the root + is never parsed on-chain), while non-root revocation remains delegated to the revoker role. +- Cold certificate verification rejects submitted cert bytes with trailing data or fields after the + signature. + ### Fixed - Reject non-canonical P-384 public key coordinates greater than or equal to the field prime `p`. @@ -49,4 +68,5 @@ yet a general-availability release. in NatSpec, the README, and the design doc. - Moved the demo `CertManagerDemo` out of `src/` into `test/helpers/`. +[Unreleased]: https://github.com/base/nitro-validator/compare/v2.0.0-rc.1...HEAD [2.0.0-rc.1]: https://github.com/base/nitro-validator/releases/tag/v2.0.0-rc.1 diff --git a/README.md b/README.md index 69d72b0..1c6b346 100644 --- a/README.md +++ b/README.md @@ -3,8 +3,10 @@ This repo provides solidity contracts for the verification of attestations generated by AWS Nitro Enclaves, as outlined in [this doc](https://github.com/aws/aws-nitro-enclaves-nsm-api/blob/4b851f3006c6fa98f23dcffb2cba03b39de9b8af/docs/attestation_process.md#3-attestation-document-validation). -This library does not currently support certificate revocation, which is disabled in AWS's attestation verification documentation +AWS's attestation verification documentation disables CRL checks in its sample flow [here](https://github.com/aws/aws-nitro-enclaves-nsm-api/blob/4b851f3006c6fa98f23dcffb2cba03b39de9b8af/docs/attestation_process.md#32-syntactical-validation). +This library supports operational revocation with an authorized revoker: operators monitor AWS CRLs +off-chain and call `CertManager.revokeCert` / `revokeCerts` for affected certificate identity keys. ## Hinted P-384 verification @@ -33,9 +35,12 @@ For the full design, security argument, and measured gas, see Deploy in this order (the verifier references are immutable): 1. `P384Verifier` -2. `CertManager(p384Verifier)` — pins the AWS Nitro root CA in its constructor. +2. `CertManager(p384Verifier)` — pins the AWS Nitro root CA and sets the deployer as owner/revoker. 3. `NitroValidator(certManager, p384Verifier)` +After deployment, move ownership to the production admin and set the operational revoker with +`transferOwnership` / `setRevoker`. + ### Verification flow Verification has two phases. Certificate chains are reused across many attestations from the same @@ -69,6 +74,28 @@ node tools/p384_hints.js attestation --attestation <0x COSE> --pubkey <0x leaf p Production callers should reimplement this in their backend language; the contract re-verifies every hint, so the generator is trusted only for liveness, never for correctness. +### Revocation operations + +`CertManager` does not fetch or parse AWS CRLs on-chain. Instead, an authorized `revoker` address +marks certificates revoked after checking AWS CRLs off-chain. Revocation is keyed by the +certificate's **(issuer, serial) identity** — `keccak256(issuerHash, serialHash)`, the same identity +AWS CRLs use to list revoked certs — not by `keccak256(certBytes)`. Raw cert bytes are **not** a +stable identity: ECDSA signatures are malleable (the `(r, n-s)` twin also verifies) and DER is +re-encodable, so a byte-keyed revocation could be bypassed by a re-encoded twin of the revoked cert. +Keying on the signature-protected (issuer, serial) pair closes that gap and lets operators revoke +straight from CRL data. Compute the key with `CertManager.computeCertId(certDER)` (or replicate it +off-chain). Revoked certificates are rejected on both cold verification and cached reuse, +independently of `notAfter`. Cached descendants are also rejected when their cached parent chain +contains a revoked certificate. + +- The deployer starts as both `owner` and `revoker`. +- The owner can call `transferOwnership`, `setRevoker`, `unrevokeCert`, and revoke + `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. + ### Example consumer ```solidity @@ -141,8 +168,10 @@ integrator (see [docs](docs/hinted-p384-nitro-attestation.md#integrator-responsi attestation fields instead. - **Enclave policy** — checking `pcrs` / `moduleID` against the enclave image(s) you trust is your responsibility. -- **Revocation** — not supported (consistent with AWS's documented validation process, linked - above). +- **Revocation operations** — the contract enforces the on-chain revoked set, but an off-chain + operator must monitor AWS CRLs and submit the affected certificate identity keys + (`keccak256(issuerHash, serialHash)`, computed via `computeCertId` or directly from the CRL's + issuer/serial entries). ## Build diff --git a/docs/hinted-p384-nitro-attestation.md b/docs/hinted-p384-nitro-attestation.md index 8d745a5..69177f0 100644 --- a/docs/hinted-p384-nitro-attestation.md +++ b/docs/hinted-p384-nitro-attestation.md @@ -218,8 +218,9 @@ Three deployable contracts: limit. It uses the hint-aware `ECDSA384` library vendored at `src/vendor/ECDSA384.sol` (see `src/vendor/README.md`). `CertManager` and `NitroValidator` hold **immutable** references to it. -- **`CertManager`** — parses/validates certificates, caches verified ones, and - pins the AWS Nitro root. Implements `ICertManager`. +- **`CertManager`** — parses/validates certificates, caches verified ones, pins the + AWS Nitro root, and enforces an owner-managed revocation set. Implements + `ICertManager`. - **`NitroValidator`** — parses the CBOR/COSE attestation and drives the certificate chain through `CertManager`. @@ -372,9 +373,46 @@ Practical reuse cases: **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. The cache is global -on-chain state — once any caller verifies a cert, others reuse it until expiry, but -only under the same parent binding. +`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 +flow. This implementation keeps CRL parsing off-chain and exposes an operational +revocation hook on-chain: + +- the `CertManager` deployer starts as both `owner` and `revoker`; +- the owner can transfer ownership, rotate the revoker, undo accidental revocations + with `unrevokeCert`, and revoke `ROOT_CA_CERT_HASH` as an emergency global halt; +- the revoker can call `revokeCert` / `revokeCerts` for non-root AWS certificate + identity keys after checking AWS CRLs off-chain. + +Revocation keys are **(issuer, serial) identities**: `keccak256(issuerHash, serialHash)`, +the same pair AWS CRLs use to list revoked certificates. `CertManager.computeCertId(certDER)` +returns this key (operators can also replicate it off-chain directly from a CRL entry). The +key is deliberately **not** `keccak256(certBytes)`: raw cert bytes are not a stable identity, +because ECDSA signatures are malleable (for a valid `(r, s)` the twin `(r, n-s)` also verifies) +and DER can be re-encoded — so a byte-keyed revocation could be bypassed by submitting a +re-encoded twin of the revoked certificate, which hashes differently but still verifies. The +(issuer, serial) pair lives inside the CA-signed TBS, so it is fixed for every byte-encoding of +a given certificate, and the identity recorded when a cert is first verified matches the key an +operator computes from the CRL. The root is the one exception: it is never parsed on-chain (it +is pinned by `ROOT_CA_CERT_HASH`), so its emergency-halt key is that pinned hash. Cold +verification additionally rejects certificate byte strings whose outer ASN.1 certificate object +does not consume all submitted bytes, or whose certificate sequence contains fields after the +signature. + +Revoked certs are rejected during cold verification, cached reuse, and warm attestation +bundle re-walks. Parent-chain revocation is also enforced for cached intermediates, so a +cached descendant cannot keep verifying through an ancestor that was later revoked. +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. **Warm-only guard.** `validateAttestationWithHints` re-runs the cabundle checks with an *empty* hint stream. Cached certs return before signature verification; a missing cert @@ -439,7 +477,7 @@ Runtime sizes (`forge build --sizes`); EIP-170 limit is 24,576 bytes: | contract | runtime size | margin | |----------|-------------:|-------:| | `P384Verifier` | 7,805 | 16,771 | -| `CertManager` | 19,620 | 4,956 | +| `CertManager` | 22,297 | 2,279 | | `NitroValidator` | 14,062 | 10,514 | (Test-only helper contracts are not part of the deployable contract set.) @@ -448,13 +486,13 @@ Runtime sizes (`forge build --sizes`); EIP-170 limit is 24,576 bytes: The hinted contracts are exercised against the real fixture and adversarial inputs. Covered failure modes: mutated hint, truncated hint, surplus hint, wrong parent hash, -expired cached cert, expired cert on first (cold) verification, the `notAfter` validity -boundary, CA/client role mismatch, missing warm cache, invalid final signature, -out-of-range ECDSA scalars (`r=0`, `r≥n`, `s=0`, `s>lowSmax`), disabled unhinted -entrypoints, EIP-170 fit, and off-chain↔on-chain hint equivalence. The DER, CBOR, and -byte-slicing parsers additionally have direct unit and fuzz tests for malformed and -out-of-bounds input (`test/Asn1Decode.t.sol`, `test/CborDecode.t.sol`, -`test/LibBytes.t.sol`). +revoked certs, revoked parents, expired cached cert, expired cert on first (cold) +verification, the `notAfter` validity boundary, CA/client role mismatch, missing warm +cache, invalid final signature, out-of-range ECDSA scalars (`r=0`, `r≥n`, `s=0`, +`s>lowSmax`), disabled unhinted entrypoints, EIP-170 fit, and off-chain↔on-chain hint +equivalence. The DER, CBOR, and byte-slicing parsers additionally have direct unit and +fuzz tests for malformed and out-of-bounds input (`test/Asn1Decode.t.sol`, +`test/CborDecode.t.sol`, `test/LibBytes.t.sol`). | invariant | component | how it is tested | |-----------|-----------|------------------| @@ -464,6 +502,7 @@ out-of-bounds input (`test/Asn1Decode.t.sol`, `test/CborDecode.t.sol`, | hinted verifier matches the original accept/reject set | `P384Verifier` | accepts a valid signature; rejects mutated hash / signature / public key | | no unhinted fallback via hinted entrypoints | `CertManager` | the unhinted entrypoints revert | | warm validation requires cached certs | `NitroValidator` | empty-hint final validation reverts when a cert is uncached | +| revoked certs are never trusted | `CertManager` | revoked cold/cached certs, revoked parents/ancestors, and revoked root/leaf warm paths revert | | out-of-range scalars are rejected | `P384Verifier` | `r=0` / `r≥n` / `s=0` / `s>lowSmax` signatures return false | | certificate validity is enforced at the boundary | `CertManager` | cold-path expiry reverts; valid at `notAfter`, expired at `notAfter+1` | | parsers reject malformed / out-of-bounds input | `Asn1Decode`, `CborDecode`, `LibBytes` | direct unit + fuzz tests for bad tags, lengths, types, and slices | @@ -516,10 +555,9 @@ deliberately left to the caller and must be handled in the consuming contract: - **Freshness / anti-replay.** `validateAttestationWithHints` only checks that `timestamp` is non-zero and that `nonce` is within a size bound; it never compares `timestamp` (milliseconds) to `block.timestamp` (seconds) nor matches `nonce` to a - challenge. A valid - attestation can be replayed until its short-lived leaf certificate expires. If you - need freshness, compare `ptrs.timestamp / 1000` to `block.timestamp` and/or verify - `ptrs.nonce` against a value you issued. + challenge. A valid attestation can be replayed until its short-lived leaf certificate + expires or is revoked. If you need freshness, compare `ptrs.timestamp / 1000` to + `block.timestamp` and/or verify `ptrs.nonce` against a value you issued. - **Signature malleability.** Low-S is intentionally not enforced (AWS does not guarantee low-S; see `CURVE_LOW_S_MAX` in `ECDSA384Curve.sol`), so for a valid signature `(r, s)` the twin `(r, n−s)` also verifies. This cannot forge an @@ -528,6 +566,12 @@ deliberately left to the caller and must be handled in the consuming contract: attestation fields (e.g. `moduleID + timestamp + nonce`). - **Enclave-image / PCR policy.** The contract returns the parsed `pcrs` and `moduleID`; deciding which enclave images you trust is application policy. +- **CRL monitoring.** `CertManager` enforces the certificate identity keys that have been + marked revoked on-chain, but it does not fetch or parse AWS CRLs. A trusted off-chain + operator must monitor AWS CRLs and submit `revokeCert` / `revokeCerts` transactions + promptly, passing each affected cert's `(issuer, serial)` identity key + (`keccak256(issuerHash, serialHash)`, via `computeCertId` or computed directly from the + CRL entry). ## 11. On-chain demo diff --git a/src/CertManager.sol b/src/CertManager.sol index 658d1bf..305d528 100644 --- a/src/CertManager.sol +++ b/src/CertManager.sol @@ -11,13 +11,17 @@ import {IP384Verifier} from "./IP384Verifier.sol"; // Manages a mapping of verified certificates and their metadata. // The root of trust is the AWS Nitro root cert. -// Certificate revocation is not currently supported. +// Certificate revocation is applied by an authorized revoker that tracks AWS CRLs off-chain. contract CertManager is ICertManager { using Asn1Decode for bytes; using LibAsn1Ptr for Asn1Ptr; using LibBytes for bytes; event CertVerified(bytes32 indexed certHash); + event CertRevoked(bytes32 indexed certHash); + event CertUnrevoked(bytes32 indexed certHash); + event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); + event RevokerUpdated(address indexed previousRevoker, address indexed newRevoker); // root CA certificate constants (don't store it to reduce contract size) bytes32 public constant ROOT_CA_CERT_HASH = 0x311d96fcd5c5e0ccf72ef548e2ea7d4c0cd53ad7c4cc49e67471aed41d61f185; @@ -48,12 +52,37 @@ contract CertManager is ICertManager { mapping(bytes32 => bytes) public verified; // certHash -> parent cert hash used during cold verification mapping(bytes32 => bytes32) internal verifiedParent; + // certHash -> revocation identity key (keccak256(issuerHash, serialHash)), recorded at cold + // verification so the warm path and parent-chain walk can check revocation without re-parsing. + mapping(bytes32 => bytes32) internal certIdentity; + // revocation set, keyed by the (issuer, serial) identity for non-root certs (see {computeCertId}) + // and by the pinned ROOT_CA_CERT_HASH for the root emergency halt. NOT keyed by keccak256(cert): + // raw cert bytes are not a stable identity (ECDSA signatures are malleable and DER is re-encodable), + // so byte-keying would let a re-encoded twin of a revoked cert slip through. + mapping(bytes32 => bool) public revoked; IP384Verifier public immutable p384Verifier; + address public owner; + address public revoker; + + modifier onlyOwner() { + _onlyOwner(); + _; + } + + function _onlyOwner() internal view { + require(msg.sender == owner, "not owner"); + } + + function _onlyRevoker() internal view { + require(msg.sender == revoker, "not revoker"); + } constructor(IP384Verifier p384Verifier_) { require(address(p384Verifier_) != address(0), "missing P384 verifier"); p384Verifier = p384Verifier_; + owner = msg.sender; + revoker = msg.sender; _saveVerified( ROOT_CA_CERT_HASH, VerifiedCert({ @@ -64,6 +93,8 @@ contract CertManager is ICertManager { pubKey: ROOT_CA_CERT_PUB_KEY }) ); + emit OwnershipTransferred(address(0), msg.sender); + emit RevokerUpdated(address(0), msg.sender); } /// @notice DEPRECATED — always reverts. The fully on-chain (non-hinted) path is too expensive @@ -104,10 +135,96 @@ contract CertManager is ICertManager { return _verifyCert(cert, keccak256(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. function loadVerified(bytes32 certHash) external view returns (VerifiedCert memory) { return _loadVerified(certHash); } + function isRevoked(bytes32 certId) external view returns (bool) { + return revoked[certId]; + } + + /// @notice Compute the revocation identity key for a certificate. + /// @dev Returns `keccak256(issuerHash, serialHash)` — the (issuer DN, serial number) pair that + /// uniquely identifies an X.509 certificate and that AWS CRLs use to list revoked certs. + /// This key is invariant to ECDSA signature malleability (the `(r, n-s)` twin) and to DER + /// re-encoding, unlike `keccak256(cert)`. Operators pass the result to {revokeCert} / + /// {revokeCerts}; the same value is recorded on-chain when the cert is first verified, so a + /// revocation applies to every byte-encoding of that certificate. Reverts on malformed DER. + function computeCertId(bytes memory cert) external pure returns (bytes32) { + Asn1Ptr tbsCertPtr = cert.firstChildOf(cert.root()); + return _certIdentity(cert, tbsCertPtr); + } + + function transferOwnership(address newOwner) external onlyOwner { + require(newOwner != address(0), "invalid owner"); + emit OwnershipTransferred(owner, newOwner); + owner = newOwner; + } + + function setRevoker(address newRevoker) external onlyOwner { + require(newRevoker != address(0), "invalid revoker"); + emit RevokerUpdated(revoker, newRevoker); + revoker = newRevoker; + } + + /// @notice Revoke a certificate by its identity key (see {computeCertId}); use ROOT_CA_CERT_HASH + /// to trigger the owner-only emergency global halt. + function revokeCert(bytes32 certId) external { + _requireCanRevoke(certId); + _revokeCert(certId); + } + + function revokeCerts(bytes32[] calldata certIds) external { + for (uint256 i = 0; i < certIds.length; ++i) { + _requireCanRevoke(certIds[i]); + _revokeCert(certIds[i]); + } + } + + function unrevokeCert(bytes32 certId) external onlyOwner { + revoked[certId] = false; + emit CertUnrevoked(certId); + } + + function _revokeCert(bytes32 certId) internal { + revoked[certId] = true; + emit CertRevoked(certId); + } + + function _requireCanRevoke(bytes32 certId) internal view { + // The root is identified by its pinned cert hash (it is never parsed on-chain); revoking it + // halts all validation, so it is owner-only. Non-root revocation by (issuer, serial) identity + // is delegated to the revoker role. + if (certId == ROOT_CA_CERT_HASH) { + _onlyOwner(); + } else { + _onlyRevoker(); + } + } + + function _requireNotRevoked(bytes32 certId) internal view { + require(!revoked[certId], "cert revoked"); + } + + /// @dev The revocation key for a cached cert: the pinned hash for the root, otherwise the + /// (issuer, serial) identity recorded at cold verification. + function _revocationKey(bytes32 certHash) internal view returns (bytes32) { + return certHash == ROOT_CA_CERT_HASH ? ROOT_CA_CERT_HASH : certIdentity[certHash]; + } + + function _requireCachedChainNotRevoked(bytes32 certHash) internal view { + while (certHash != bytes32(0)) { + _requireNotRevoked(_revocationKey(certHash)); + if (certHash == ROOT_CA_CERT_HASH) { + return; + } + certHash = verifiedParent[certHash]; + } + } + function _verifyCert( bytes memory certificate, bytes32 certHash, @@ -115,9 +232,11 @@ contract CertManager is ICertManager { bytes32 parentCertHash, bytes memory signatureHints ) internal returns (VerifiedCert memory) { - VerifiedCert memory parent = _loadVerified(parentCertHash); + VerifiedCert memory parent; if (certHash != ROOT_CA_CERT_HASH) { + parent = _loadVerified(parentCertHash); require(parent.pubKey.length > 0, "parent cert unverified"); + _requireCachedChainNotRevoked(parentCertHash); require(!_certificateExpired(parent.notAfter), "parent cert expired"); require(parent.ca, "parent cert is not a CA"); require(!ca || parent.maxPathLen != 0, "maxPathLen exceeded"); @@ -126,6 +245,7 @@ contract CertManager is ICertManager { // skip verification if already verified VerifiedCert memory cert = _loadVerified(certHash); if (cert.pubKey.length != 0) { + _requireNotRevoked(_revocationKey(certHash)); require(!_certificateExpired(cert.notAfter), "cert expired"); require(cert.ca == ca, "cert is not a CA"); if (certHash != ROOT_CA_CERT_HASH) { @@ -134,9 +254,13 @@ contract CertManager is ICertManager { return cert; } - cert = _verifyUncachedCert(certificate, ca, parent, signatureHints); + bytes32 identity; + (cert, identity) = _verifyUncachedCert(certificate, ca, parent, signatureHints); + // Reject by (issuer, serial) identity so a re-encoded twin of a revoked cert cannot pass. + _requireNotRevoked(identity); _saveVerified(certHash, cert); verifiedParent[certHash] = parentCertHash; + certIdentity[certHash] = identity; emit CertVerified(certHash); @@ -148,14 +272,17 @@ contract CertManager is ICertManager { bool ca, VerifiedCert memory parent, bytes memory signatureHints - ) internal view returns (VerifiedCert memory cert) { + ) internal view returns (VerifiedCert memory cert, bytes32 identity) { Asn1Ptr root = certificate.root(); + require(root.totalLength() == certificate.length, "invalid cert length"); Asn1Ptr tbsCertPtr = certificate.firstChildOf(root); (uint64 notAfter, int64 maxPathLen, bytes32 issuerHash, bytes32 subjectHash, bytes memory pubKey) = _parseTbs(certificate, tbsCertPtr, ca); require(parent.subjectHash == issuerHash, "issuer / subject mismatch"); + identity = _certIdentity(certificate, tbsCertPtr); + // constrain maxPathLen to parent's maxPathLen-1 if (parent.maxPathLen > 0 && (maxPathLen < 0 || maxPathLen >= parent.maxPathLen)) { maxPathLen = parent.maxPathLen - 1; @@ -168,6 +295,21 @@ contract CertManager is ICertManager { }); } + /// @dev Derives the (issuer, serial) revocation identity from a parsed certificate. The serial is + /// the second TBS field (after the explicit [0] version) and the issuer DN is the fourth + /// (after the inner signature algorithm); both lie inside the CA-signed TBS, so the identity + /// is fixed for any byte-encoding of the certificate that verifies. Mirrors the issuer-hash + /// derivation in `_parseTbsInner`. + function _certIdentity(bytes memory certificate, Asn1Ptr tbsCertPtr) internal pure returns (bytes32) { + Asn1Ptr versionPtr = certificate.firstChildOf(tbsCertPtr); + Asn1Ptr serialPtr = certificate.nextSiblingOf(versionPtr); + Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(serialPtr); + Asn1Ptr issuerPtr = certificate.nextSiblingOf(sigAlgoPtr); + bytes32 serialHash = certificate.keccak(serialPtr.content(), serialPtr.length()); + bytes32 issuerHash = certificate.keccak(issuerPtr.content(), issuerPtr.length()); + return keccak256(abi.encodePacked(issuerHash, serialHash)); + } + function _parseTbs(bytes memory certificate, Asn1Ptr ptr, bool ca) internal view @@ -337,19 +479,16 @@ contract CertManager is ICertManager { ) internal view { Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(ptr); require(certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) == CERT_ALGO_OID, "invalid cert sig algo"); + Asn1Ptr sigPtr = certificate.nextSiblingOf(sigAlgoPtr); + require(sigPtr.header() + sigPtr.totalLength() == certificate.length, "trailing cert fields"); bytes memory hash = Sha2Ext.sha384(certificate, ptr.header(), ptr.totalLength()); - bytes memory sigPacked = _certSignature(certificate, sigAlgoPtr); + bytes memory sigPacked = _certSignature(certificate, sigPtr); require(p384Verifier.verifyP384SignatureWithHints(hash, sigPacked, pubKey, signatureHints), "invalid sig"); } - function _certSignature(bytes memory certificate, Asn1Ptr sigAlgoPtr) - internal - pure - returns (bytes memory sigPacked) - { - Asn1Ptr sigPtr = certificate.nextSiblingOf(sigAlgoPtr); + function _certSignature(bytes memory certificate, Asn1Ptr sigPtr) internal pure returns (bytes memory sigPacked) { Asn1Ptr sigBPtr = certificate.bitstring(sigPtr); Asn1Ptr sigRoot = certificate.rootOf(sigBPtr); Asn1Ptr sigRPtr = certificate.firstChildOf(sigRoot); diff --git a/src/ICertManager.sol b/src/ICertManager.sol index e072abe..69a4a5c 100644 --- a/src/ICertManager.sol +++ b/src/ICertManager.sol @@ -12,6 +12,12 @@ interface ICertManager { // --- Active (hinted) entrypoints --- + function owner() external view returns (address); + + function revoker() external view returns (address); + + function revoked(bytes32 certHash) external view returns (bool); + function verifyCACertWithHints(bytes memory cert, bytes32 parentCertHash, bytes memory signatureHints) external returns (bytes32); @@ -20,8 +26,21 @@ interface ICertManager { external returns (VerifiedCert memory); + /// @notice Raw cache read. 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); + + function transferOwnership(address newOwner) external; + + function setRevoker(address newRevoker) external; + + function revokeCert(bytes32 certHash) external; + + function revokeCerts(bytes32[] calldata certHashes) external; + + function unrevokeCert(bytes32 certHash) external; + // --- DEPRECATED: these always revert; use the *WithHints variants above. --- /// @dev DEPRECATED — always reverts ("use hinted cert verification"). diff --git a/src/NitroValidator.sol b/src/NitroValidator.sol index a760895..e87eae1 100644 --- a/src/NitroValidator.sol +++ b/src/NitroValidator.sol @@ -91,8 +91,8 @@ contract NitroValidator { /// well-formed, but deliberately does NOT enforce: /// - Freshness / anti-replay: `ptrs.timestamp` is only checked to be non-zero and `nonce` /// is only length-bounded. A valid attestation can be replayed until its leaf cert - /// expires. Callers that need freshness must compare `ptrs.timestamp / 1000` to - /// `block.timestamp` and/or match `ptrs.nonce` against a challenge they issued. + /// expires or is revoked. Callers that need freshness must compare `ptrs.timestamp / 1000` + /// to `block.timestamp` and/or match `ptrs.nonce` against a challenge they issued. /// - Signature non-malleability: low-S is not enforced (see {ECDSA384Curve.CURVE_LOW_S_MAX}), /// so do not use `signature` (or its hash) as a uniqueness key — dedupe on attestation /// fields instead. diff --git a/test/IndefiniteLengthCbor.t.sol b/test/IndefiniteLengthCbor.t.sol index 90b0059..2f31395 100644 --- a/test/IndefiniteLengthCbor.t.sol +++ b/test/IndefiniteLengthCbor.t.sol @@ -87,6 +87,18 @@ contract NitroValidatorHarness is NitroValidator { /// @notice Minimal ICertManager stub; _parseAttestation is pure so this is never called. contract StubCertManager is ICertManager { + function owner() external pure returns (address) { + return address(0); + } + + function revoker() external pure returns (address) { + return address(0); + } + + function revoked(bytes32) external pure returns (bool) { + return false; + } + function verifyCACert(bytes memory, bytes32) external pure returns (bytes32) { return bytes32(0); } @@ -110,6 +122,20 @@ contract StubCertManager is ICertManager { function loadVerified(bytes32) external pure returns (VerifiedCert memory v) { return v; } + + function isRevoked(bytes32) external pure returns (bool) { + return false; + } + + function transferOwnership(address) external pure {} + + function setRevoker(address) external pure {} + + function revokeCert(bytes32) external pure {} + + function revokeCerts(bytes32[] calldata) external pure {} + + function unrevokeCert(bytes32) external pure {} } // ────────────────────────────────────────────────────────────── diff --git a/test/hinted/HintedNitroAttestation.t.sol b/test/hinted/HintedNitroAttestation.t.sol index 0e7c495..f68f60a 100644 --- a/test/hinted/HintedNitroAttestation.t.sol +++ b/test/hinted/HintedNitroAttestation.t.sol @@ -135,6 +135,270 @@ contract HintedNitroAttestationTest is Test { certManager.verifyClientCertWithHints(clientCert, keccak256(rootCert), ""); } + function test_CertManagerRevocationRoles() public { + assertEq(certManager.owner(), address(this)); + assertEq(certManager.revoker(), address(this)); + + bytes32 certHash = keccak256("cert"); + certManager.revokeCert(certHash); + assertTrue(certManager.revoked(certHash)); + assertTrue(certManager.isRevoked(certHash)); + + address newRevoker = address(0xBEEF); + vm.prank(address(0xCAFE)); + vm.expectRevert("not owner"); + certManager.setRevoker(newRevoker); + + vm.expectRevert("invalid revoker"); + certManager.setRevoker(address(0)); + + certManager.setRevoker(newRevoker); + assertEq(certManager.revoker(), newRevoker); + + bytes32 otherCertHash = keccak256("other cert"); + vm.prank(address(0xCAFE)); + vm.expectRevert("not revoker"); + certManager.revokeCert(otherCertHash); + + vm.prank(newRevoker); + certManager.revokeCert(otherCertHash); + assertTrue(certManager.revoked(otherCertHash)); + + vm.prank(newRevoker); + vm.expectRevert("not owner"); + certManager.unrevokeCert(otherCertHash); + + certManager.unrevokeCert(otherCertHash); + assertFalse(certManager.revoked(otherCertHash)); + + vm.expectRevert("invalid owner"); + certManager.transferOwnership(address(0)); + + address newOwner = address(0xA11CE); + vm.prank(address(0xCAFE)); + vm.expectRevert("not owner"); + certManager.transferOwnership(newOwner); + + certManager.transferOwnership(newOwner); + assertEq(certManager.owner(), newOwner); + + vm.expectRevert("not owner"); + certManager.setRevoker(address(0x1234)); + + vm.prank(newOwner); + certManager.setRevoker(address(0x1234)); + assertEq(certManager.revoker(), address(0x1234)); + } + + function test_CertManagerRevokerCanBatchRevoke() public { + address newRevoker = address(0xBEEF); + certManager.setRevoker(newRevoker); + + bytes32[] memory certHashes = new bytes32[](2); + certHashes[0] = keccak256("cert 1"); + certHashes[1] = keccak256("cert 2"); + + vm.prank(newRevoker); + certManager.revokeCerts(certHashes); + + assertTrue(certManager.revoked(certHashes[0])); + assertTrue(certManager.revoked(certHashes[1])); + } + + function test_CertManagerRootRevocationRequiresOwner() public { + address newRevoker = address(0xBEEF); + certManager.setRevoker(newRevoker); + bytes32 rootHash = certManager.ROOT_CA_CERT_HASH(); + + vm.prank(newRevoker); + vm.expectRevert("not owner"); + certManager.revokeCert(rootHash); + assertFalse(certManager.revoked(rootHash)); + + bytes32[] memory certHashes = new bytes32[](2); + certHashes[0] = keccak256("non-root cert"); + certHashes[1] = rootHash; + + vm.prank(newRevoker); + vm.expectRevert("not owner"); + certManager.revokeCerts(certHashes); + assertFalse(certManager.revoked(certHashes[0])); + assertFalse(certManager.revoked(rootHash)); + + certManager.revokeCert(rootHash); + assertTrue(certManager.revoked(rootHash)); + } + + function test_HintedCACertRejectsTrailingBytes() public { + bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); + (bytes memory attestationTbs,) = validator.decodeAttestationTbs(attestation); + NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs); + (bytes memory caCert, bytes32 parentHash,) = _firstNonRootCA(attestationTbs, ptrs); + + vm.expectRevert("invalid cert length"); + certManager.verifyCACertWithHints(abi.encodePacked(caCert, bytes1(0x00)), parentHash, ""); + } + + function test_HintedCACertRejectsTrailingFieldInsideCertificateSequence() public { + bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); + (bytes memory attestationTbs,) = validator.decodeAttestationTbs(attestation); + NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs); + (bytes memory caCert, bytes32 parentHash,) = _firstNonRootCA(attestationTbs, ptrs); + + vm.expectRevert("trailing cert fields"); + certManager.verifyCACertWithHints(_appendInsideOuterSequence(caCert, bytes1(0x00)), parentHash, ""); + } + + function test_HintedCACertRejectsRevokedColdCert() public { + bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); + (bytes memory attestationTbs,) = validator.decodeAttestationTbs(attestation); + NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs); + (bytes memory caCert, bytes32 parentHash, bytes memory parentPubKey) = _firstNonRootCA(attestationTbs, ptrs); + bytes memory hints = hintCollector.collectCertSignatureHints(caCert, parentPubKey); + + certManager.revokeCert(certManager.computeCertId(caCert)); + + vm.expectRevert("cert revoked"); + certManager.verifyCACertWithHints(caCert, parentHash, hints); + } + + function test_HintedCACertRejectsRevokedCachedCert() public { + bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); + (bytes memory attestationTbs,) = validator.decodeAttestationTbs(attestation); + NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs); + (bytes memory caCert, bytes32 parentHash, bytes memory parentPubKey) = _firstNonRootCA(attestationTbs, ptrs); + bytes memory hints = hintCollector.collectCertSignatureHints(caCert, parentPubKey); + certManager.verifyCACertWithHints(caCert, parentHash, hints); + + certManager.revokeCert(certManager.computeCertId(caCert)); + + vm.expectRevert("cert revoked"); + certManager.verifyCACertWithHints(caCert, parentHash, ""); + } + + function test_HintedCAChildRejectsRevokedParent() public { + bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); + (bytes memory attestationTbs,) = validator.decodeAttestationTbs(attestation); + NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs); + + bytes memory rootCert = attestationTbs.slice(ptrs.cabundle[0]); + bytes32 rootHash = keccak256(rootCert); + bytes memory ca1 = attestationTbs.slice(ptrs.cabundle[1]); + bytes memory ca1Hints = hintCollector.collectCertSignatureHints(ca1, certManager.loadVerified(rootHash).pubKey); + bytes32 ca1Hash = certManager.verifyCACertWithHints(ca1, rootHash, ca1Hints); + + bytes memory ca2 = attestationTbs.slice(ptrs.cabundle[2]); + certManager.revokeCert(certManager.computeCertId(ca1)); + + vm.expectRevert("cert revoked"); + certManager.verifyCACertWithHints(ca2, ca1Hash, ""); + } + + function test_HintedCAChildRejectsRevokedAncestor() public { + bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); + (bytes memory attestationTbs,) = validator.decodeAttestationTbs(attestation); + NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs); + + bytes memory rootCert = attestationTbs.slice(ptrs.cabundle[0]); + bytes32 rootHash = keccak256(rootCert); + bytes memory ca1 = attestationTbs.slice(ptrs.cabundle[1]); + bytes memory ca1Hints = hintCollector.collectCertSignatureHints(ca1, certManager.loadVerified(rootHash).pubKey); + bytes32 ca1Hash = certManager.verifyCACertWithHints(ca1, rootHash, ca1Hints); + bytes memory ca2 = attestationTbs.slice(ptrs.cabundle[2]); + bytes memory ca2Hints = hintCollector.collectCertSignatureHints(ca2, certManager.loadVerified(ca1Hash).pubKey); + + certManager.revokeCert(rootHash); + + vm.expectRevert("cert revoked"); + certManager.verifyCACertWithHints(ca2, ca1Hash, ca2Hints); + } + + function test_HintedCachedCertRejectsRevokedAncestor() public { + bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); + (bytes memory attestationTbs,) = validator.decodeAttestationTbs(attestation); + NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs); + + bytes memory rootCert = attestationTbs.slice(ptrs.cabundle[0]); + bytes32 rootHash = keccak256(rootCert); + bytes memory ca1 = attestationTbs.slice(ptrs.cabundle[1]); + bytes memory ca1Hints = hintCollector.collectCertSignatureHints(ca1, certManager.loadVerified(rootHash).pubKey); + bytes32 ca1Hash = certManager.verifyCACertWithHints(ca1, rootHash, ca1Hints); + bytes memory ca2 = attestationTbs.slice(ptrs.cabundle[2]); + bytes memory ca2Hints = hintCollector.collectCertSignatureHints(ca2, certManager.loadVerified(ca1Hash).pubKey); + certManager.verifyCACertWithHints(ca2, ca1Hash, ca2Hints); + + certManager.revokeCert(rootHash); + + vm.expectRevert("cert revoked"); + certManager.verifyCACertWithHints(ca2, ca1Hash, ""); + } + + function test_HintedValidationRejectsRevokedLeaf() public { + bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); + (bytes memory attestationTbs, bytes memory signature) = validator.decodeAttestationTbs(attestation); + NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs); + ICertManager.VerifiedCert memory leaf = _cacheCertBundleWithHints(attestationTbs); + bytes memory hash = Sha2Ext.sha384(attestationTbs, 0, attestationTbs.length); + bytes memory attestationHints = hintCollector.collectVerifyHints(hash, signature, leaf.pubKey); + + certManager.revokeCert(certManager.computeCertId(attestationTbs.slice(ptrs.cert))); + + vm.expectRevert("cert revoked"); + validator.validateAttestationWithHints(attestationTbs, signature, attestationHints); + } + + function test_HintedValidationRejectsRevokedRoot() 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 attestationHints = hintCollector.collectVerifyHints(hash, signature, leaf.pubKey); + + certManager.revokeCert(certManager.ROOT_CA_CERT_HASH()); + + vm.expectRevert("cert revoked"); + validator.validateAttestationWithHints(attestationTbs, signature, attestationHints); + } + + function test_HintedCachedCertCanVerifyAfterOwnerUnrevokes() public { + bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); + (bytes memory attestationTbs,) = validator.decodeAttestationTbs(attestation); + NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs); + (bytes memory caCert, bytes32 parentHash, bytes memory parentPubKey) = _firstNonRootCA(attestationTbs, ptrs); + bytes memory hints = hintCollector.collectCertSignatureHints(caCert, parentPubKey); + bytes32 caHash = certManager.verifyCACertWithHints(caCert, parentHash, hints); + bytes32 caId = certManager.computeCertId(caCert); + + certManager.revokeCert(caId); + vm.expectRevert("cert revoked"); + certManager.verifyCACertWithHints(caCert, parentHash, ""); + + certManager.unrevokeCert(caId); + assertEq(certManager.verifyCACertWithHints(caCert, parentHash, ""), caHash); + } + + /// @dev Revocation is keyed by the (issuer, serial) identity, not by keccak256(cert), so a cert + /// whose bytes differ only outside the CA-signed TBS — e.g. an ECDSA-malleable `(r, n-s)` + /// signature twin or a DER re-encoding of the signature — maps to the SAME revocation key + /// and cannot slip past a revocation. Here we mutate a signature byte to model that variant. + function test_RevocationIdentityIsInvariantToSignatureBytes() public view { + bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); + (bytes memory attestationTbs,) = validator.decodeAttestationTbs(attestation); + NitroValidator.Ptrs memory ptrs = parser.parseAttestation(attestationTbs); + (bytes memory caCert,,) = _firstNonRootCA(attestationTbs, ptrs); + + bytes memory variant = bytes.concat(caCert); + // Flip the last signature byte (inside the trailing s INTEGER, outside the TBS). + variant[variant.length - 1] = bytes1(uint8(variant[variant.length - 1]) ^ 0x01); + + assertTrue(keccak256(caCert) != keccak256(variant), "byte hashes differ"); + assertEq( + certManager.computeCertId(caCert), + certManager.computeCertId(variant), + "identity invariant to signature bytes" + ); + } + function test_HintedCACertRejectsExpiredCachedCert() public { bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); (bytes memory attestationTbs,) = validator.decodeAttestationTbs(attestation); @@ -718,6 +982,17 @@ contract HintedNitroAttestationTest is Test { output[index] = bytes1(uint8(output[index]) ^ 1); } + function _appendInsideOuterSequence(bytes memory der, bytes1 value) internal pure returns (bytes memory output) { + require(der.length >= 4 && der[0] == 0x30 && der[1] == 0x82, "test: expected long sequence"); + uint256 length = uint256(uint8(der[2])) << 8 | uint8(der[3]); + require(length + 4 == der.length, "test: unexpected sequence length"); + length += 1; + + output = abi.encodePacked(der, value); + output[2] = bytes1(uint8(length >> 8)); + output[3] = bytes1(uint8(length)); + } + function _repairMissingPublicKeyBytes(bytes memory attestation) internal pure returns (bytes memory repaired) { // The pasted Base64 sample is missing "ic_" in the CBOR key // "public_key", but the key length and outer COSE payload length still