Coinbase: nitro-validator [05294ec0]

Summary

_parsePcrs bounds every PCR index against count — the number of PCR entries present in the document — instead of against the fixed size of the PCR bank. This is only correct if the PCR keys present form a contiguous 0..count-1 set. AWS Nitro PCR banks are sparse by design, so a standard, non-adversarial attestation document can revert the parser.

Description

require(count <= MAX_PCRS, "too many pcrs");pcrs = new CborElement[](count);for (uint256 i = 0; i < count; i++) {    current = tbs.nextPositiveInt(current);    uint256 key = current.value();    require(key < count, "invalid pcr key value");    require(CborElement.unwrap(pcrs[key]) == 0, "duplicate pcr key");    current = tbs.nextByteString(current);    pcrs[key] = current;}

The loop allocates pcrs as a count-length array and rejects any key >= count. That's a sound bound only when the key set is exactly {0, 1, ..., count-1}. It is not sound for AWS Nitro's actual PCR population rules:

• PCR 0–2 are populated unconditionally (enclave image hash, kernel/bootstrap hash, application hash).
• PCR 3/4 are populated conditionally, when the parent instance's IAM role ARN / instance ID are relevant.
• PCR 5–7 are never populated.
• PCR 8 is populated specifically when the enclave image is cryptographically signed.

This means a completely standard, genuine attestation from a signed enclave image reports the key set {0, 1, 2, 3, 4, 8} — six entries, but a maximum key of 8. _parsePcrs sets count = 6, and when it reaches key 8 the check require(8 < 6, ...) fails, reverting with "invalid pcr key value" even though the document is well-formed and correctly signed by AWS.

The key < count pattern predates the forward-compatibility refactor in PR #32. It is not exercised by any existing test: IndefiniteLengthCbor.t.sol and HintedNitroAttestation.t.sol only use contiguous PCR key sets (none include PCR8). Triggering this does not require a malicious or malformed input — a routine signed-image deployment is sufficient.

Illustrative Example: Consider an attestation document whose pcrs CBOR map contains these six key/value pairs (byte strings truncated for illustration):

{  0: <hash>,   // enclave image  1: <hash>,   // bootstrap  2: <hash>,   // application  3: <hash>,   // IAM role ARN  4: <hash>,   // instance ID  8: <hash>  // present because the image is signed}

_parsePcrs computes count = 6 (six map entries) and allocates pcrs = new CborElement[](6). Parsing proceeds key-by-key; the moment it hits key = 8:

require(key < count, "invalid pcr key value"); // require(8 < 6) -> reverts

the whole call reverts, even though every byte of the document is valid CBOR and the attestation would otherwise verify correctly against AWS's signing chain. Any caller of validateAttestationWithHints targeting a signed enclave image is unconditionally blocked from verifying, regardless of the actual measurement values.

Impact: This is a liveness/correctness bug, not a false-accept — the contract fails closed (reverts) rather than mis-parsing or accepting bad data, so it does not weaken the trust model. However, if the analysis above holds against a real document, it makes validateAttestationWithHints unusable for any enclave image that is signed (or otherwise reports a non-contiguous PCR set), which is a standard Nitro configuration — effectively a denial of service against a large fraction of legitimate deployments.

Recommendation

Consider replacing the dense-array indexing with a fixed-size PCR bank array sized to the maximum valid index (32) rather than to the number of entries present, and bounding the key check by the actual PCR bank size, not by count.

Summary

AWS Nitro Enclaves launched with --debug-mode or --attach-console produce attestation documents with every PCR value set to all zeroes, per AWS's documentation. However, validateAttestationWithHints only checks each PCR's length and never its content, which allows a debug-mode attestation — cryptographically genuine, correctly signed by AWS, and structurally identical to a production one except for its PCR content — to pass verification exactly like a production attestation with the caller's expected measurements. This is risky depending on the caller's assumptions.

Description

The function validateAttestationWithHints's NatSpec already places PCR/moduleID value policy on the caller: "the caller must check ptrs.pcrs / ptrs.moduleID against the enclave image(s) they trust." That's a reasonable general design, but the all-zero debug-mode case is a specific, well-documented AWS behavior that's easy for an integrator to miss: a caller who checks "PCRs are present and the right length" can still be fooled if they don't also independently special-case all-zero values, because zero is otherwise a structurally-valid PCR value that satisfies every check validateAttestationWithHints performs.

Neither the NatSpec block nor docs/hinted-p384-nitro-attestation.md mention debug-mode enclaves or all-zero PCRs anywhere. So a caller relying solely on this documentation has no signal that they need to add that specific check. For example, a similar gap — trusting a debug-mode attestation — was part of the attack chain in the Taiko/SecondFi incident.

Impact: This is not a false-accept, but the specific debug-mode failure has real-world precedent as an exploited caller pitfall.

Recommendation

Consider either:

1. Implementing a helper/require that rejects an attestation whose PCR0–2 (the unconditionally populated bank) are all-zero, so callers who want production-only attestations get a guard instead of having to reimplement the AWS-documented check themselves. Or
2. Documenting an explicit NatSpec callout naming the debug-mode all-zero-PCR case specifically (not just "check PCR values" generically), since integrators are more likely to defend against a named, documented AWS behavior than a generic reminder.

Coinbase

PR #56

Cantina

PR #56 documents this AWS behavior.

Summary

An oversized pathLenConstraint INTEGER is silently truncated into an arbitrary int64 instead of being rejected.

Description

In _verifyBasicConstraintsExtension(), uintAt delegates to positiveIntegerContent(der, ptr, 32), which accepts any canonically-encoded DER INTEGER up to 32 bytes without reverting. The result (a uint256) is then narrowed via uint64(...) and reinterpreted via int64(...). An INTEGER encoded with more than 8 significant bytes passes ASN.1 validation cleanly and is then silently wrapped into an arbitrary int64 value (including a value that reads as negative) instead of causing a revert.

if (certificate[basicConstraintsPtr.header()] == 0x02) {    if (basicConstraintsPtr.length() == 0) revert InvalidBasicConstraints();    maxPathLen = int64(uint64(certificate.uintAt(basicConstraintsPtr)));} else {    revert InvalidBasicConstraints();}

This is inconsistent with the parser's stated philosophy elsewhere in the codebase, which is to reject rather than silently accept a malformed-but-signed encoding (see docs/hinted-p384-nitro-attestation.md, "Certificate parser hardening model": "the contract should also avoid trusting certs that are signed yet malformed or interpreted differently by strict DER/X.509 tooling"). The truncated value flows into maxPathLen constraint checks for the rest of the chain.

Impact: Exploitability is limited in practice: it requires a certificate that is already validly signed up the pinned AWS root chain to carry an anomalous (>8-byte) pathLenConstraint encoding. Real X.509 tooling / AWS Nitro should never produce this, so it is not reachable by an unprivileged attacker today. However, this is a robustness gap.

Recommendation

Consider validating the decoded pathLenConstraint value against type(int64).max before casting, rejecting encodings that would overflow int64.

Summary

Four fields across CertManager.sol are hashed or compared purely by their raw content bytes via certificate.keccak(ptr.content(), ptr.length()) or an equivalent content-only comparison without validating that the field carries its expected ASN.1 tag. A node whose tag byte is substituted for a different one, but whose content bytes are left unchanged, is indistinguishable from a correctly-tagged field at every one of these sites.

Description

certificate.keccak(ptr.content(), ptr.length()) hashes only the field's content, never its tag byte. So tag substitution with identical content is invisible to the hash. The four sites below don't validate their tags:

1. serialNumber: hashed into serialHash, half of the onchain keccak256(issuerHash, serialHash) revocation identity, without checking der[serialPtr.header()] == 0x02. _parseTbs separately walks past the same node without even binding it to a variable.
2. issuer: hashed into issuerHash, the other half of the same revocation identity, without checking der[issuerPtr.header()] == 0x30. Notably, _certIdentity's own doc comment states it "mirrors the issuer-hash derivation in _parseTbsInner", but it mirrors the traversal, but not the guard: _parseTbsInner calls _requireAsn1Tag(certificate, issuerPtr, 0x30) immediately before hashing the same field, while _certIdentity does not.
3. Extension OID: classifies an extension as basicConstraints/keyUsage by hashing oidPtr's content against BASIC_CONSTRAINTS_OID/ KEY_USAGE_OID (themselves keccak256 of raw OID bytes only), without checking der[oidPtr.header()] == 0x06. firstChildOf only asserts the parent extensionPtr is constructed; it never validates oidPtr's own tag.
4. SubjectPublicKeyInfo algorithm/curve OIDs: pubKeyAlgoIdPtr and algoParamsPtr are each hashed and compared against EC_PUB_KEY_OID/SECP_384_R1_OID (the same vulnerable content-only-hash shape as #3 above) without checking either carries tag 0x06. The enclosing pubKeyAlgoPtr SEQUENCE is tag-checked, but its two OID children are not.

All four are the same "different tag, same content bytes" substitution gap that commit 338bfd2 ("reject substituted ASN.1 certificate tags") introduced _requireAsn1Tag to close, applied to every other sibling field in the same functions (version, signatureAlgorithm, validity, subject, subjectPublicKeyInfo, the extension's own SEQUENCE wrapper, the pubkey AlgorithmIdentifier SEQUENCE) but missed on these four.

Impact: Not a reachable false-accept however all are flagged as a strict-DER-conformance / defense-in-depth gap, consistent with the project's own stated philosophy of rejecting malformed-but-signed encodings rather than silently tolerating them.

Recommendation

Consider adding the following, matching the _requireAsn1Tag pattern already used for every other structural field in the file:

• _requireAsn1Tag(certificate, serialPtr, 0x02) in both _parseTbs (after binding the intermediate node instead of discarding it) and _certIdentity.
• _requireAsn1Tag(certificate, issuerPtr, 0x30) in _certIdentity, right after issuerPtr is bound.
• _requireAsn1Tag(certificate, oidPtr, 0x06) in _verifyExtensions, immediately after computing oidPtr.
• _requireAsn1Tag(certificate, pubKeyAlgoIdPtr, 0x06) and _requireAsn1Tag(certificate, algoParamsPtr, 0x06) in _parsePubKey, right after algoParamsPtr is bound.

Description

The decoder reads the payload from the position returned for the unprotected header map:

CborElement protectedPtr = attestation.byteStringAt(offset);CborElement unprotectedPtr = attestation.nextMap(protectedPtr);CborElement payloadPtr = attestation.nextByteString(unprotectedPtr);CborElement signaturePtr = attestation.nextByteString(payloadPtr);

However, unprotectedPtr.end() does not point after the complete map. Map elements have a reported content length of zero, so the pointer stops immediately after the map header:

function end(CborElement self) internal pure returns (uint256) {    return start(self) + length(self);}
function length(CborElement self) internal pure returns (uint256) {    uint8 _type = cborType(self);    if (_type == 0x40 || _type == 0x60) {        return value(self);    }    return 0;}

This works when the unprotected header is empty, as required by the current AWS Nitro format. If the map contains an entry, the decoder treats that entry as the payload and either reads the wrong fields or rejects the message. The issue therefore does not affect normal AWS attestations today, but it limits compatibility with non-standard wrappers or a future format change.

Recommendation

Skip the complete unprotected header map before reading the payload and signature. Also confirm that the map is well-formed and that the outer message contains no unexpected trailing data.

Description

AWS defines public_key as optional. When it is omitted, the parser never assigns ptrs.publicKey, leaving it as a zero-value pointer. The following validation accepts an explicit CBOR null or a key between 1 and 1024 bytes, but it does not accept that unset pointer:

require(    ptrs.publicKey.isNull()        || (1 <= ptrs.publicKey.length() && ptrs.publicKey.length() <= 1024),    "invalid pub key");

The null check recognizes only explicit CBOR null or undefined values:

function isNull(CborElement self) internal pure returns (bool) {    uint8 _type = cborType(self);    return _type == 0xf6 || _type == 0xf7;}

For an omitted field, isNull() returns false and length() returns zero, so validation reverts with invalid pub key.

AWS's current reference representation normally keeps this field and represents an absent key as an explicit null value, which the validator accepts. The incompatibility is therefore confirmed against the documented format, but there is not yet evidence that current AWS production attestations completely omit the field. If AWS produces or later adopts that representation, an otherwise valid signed attestation will be rejected.

Recommendation

Treat a zero-value pointer as an omitted public_key. If the field is present, validate its type and permitted length separately.

require(-    ptrs.publicKey.isNull() || (1 <= ptrs.publicKey.length() && ptrs.publicKey.length() <= 1024),+    CborElement.unwrap(ptrs.publicKey) == 0+        || ptrs.publicKey.isNull()+        || (1 <= ptrs.publicKey.length() && ptrs.publicKey.length() <= 1024),     "invalid pub key" );

Description

The date parser accepts any day from 1 to 31 without checking whether that day exists in the selected month:

require(year >= 1970);require(1 <= month && month <= 12);require(1 <= day && day <= 31);require(hour <= 23);require(minute <= 59);require(second <= 59);
int256 _year = int256(year);int256 _month = int256(month);int256 _day = int256(day);
int256 _days = _day - 32075    + 1461 * (_year + 4800 + (_month - 14) / 12) / 4    + 367 * (_month - 2 - (_month - 14) / 12 * 12) / 12    - 3 * ((_year + 4900 + (_month - 14) / 12) / 100) / 4    - 2440588;

As a result, an impossible date such as February 30 is accepted and converted into a date in March. A malformed certificate may therefore receive a different effective validity period instead of being rejected.

Exploitation would require a malformed certificate signed by a trusted AWS certificate authority, so an external attacker cannot trigger the issue independently.

Recommendation

Validate the correct number of days for each month before converting the date. February must also account for leap years.

Summary

critical / cA BOOLEAN fields are not checked against DER's canonical encoding (content byte must be exactly 0x00 or 0xFF). Both silently resolve any other byte value to one branch instead of rejecting the encoding as malformed.

Description

// _verifyExtensionsif (certificate[valuePtr.header()] == 0x01) {    if (valuePtr.length() != 1) revert InvalidExtension();    if (!recognized && certificate[valuePtr.content()] != 0x00) revert UnsupportedCriticalExtension();

// _verifyBasicConstraintsExtensionif (certificate[basicConstraintsPtr.header()] == 0x01) {    if (basicConstraintsPtr.length() != 1) revert InvalidBasicConstraints();    isCA = certificate[basicConstraintsPtr.content()] == 0xff;

In _verifyExtensions, critical is treated as true for any nonzero content byte (0x01, 0x7F, …), not just canonical 0xFF. In _verifyBasicConstraintsExtension, isCA is true only for exact 0xFF, so any other nonzero byte (e.g. 0x01) is silently treated as false rather than flagged as an invalid encoding.

Commit ac0c243 ("reject non-canonical ASN.1 integers") hardened canonical INTEGER encoding via a centralized positiveIntegerContent helper in Asn1Decode.sol, but no equivalent shared helper or check exists for BOOLEAN. Both sites above implement their own ad hoc, mutually inconsistent interpretation of non-canonical values inline.

Impact: Both cases resolve fail-closed or neutral rather than fail-open: an out-of-spec critical byte is treated as more restrictive (critical=true, more likely to revert via UnsupportedCriticalExtension), and an out-of-spec cA byte is treated as false, which still gets caught by the subsequent if (ca != isCA) revert InvalidBasicConstraints() comparison when it disagrees with the caller-supplied ca flag. This is unreachable against genuine AWS-signed certs and is flagged as a consistency gap with the parser's own stated non-canonical-rejection philosophy. Untested: test/CertManager.t.sol's extension tests only cover 0xFF/absent critical bytes, not a non-canonical nonzero value.

Recommendation

Consider adding a canonical-boolean check (content byte == 0x00 || == 0xFF, reverting otherwise) similar to positiveIntegerContent.

Summary

The cached ancestor chain is walked in full, up to the pinned root, to check revocation, but expiry is only ever checked on the certificate being verified and its immediate parent. A cached grandparent-or-higher certificate authority that has since expired is invisible to this check.

Description

function _requireCachedChainNotRevoked(bytes32 certHash) internal view {    while (certHash != bytes32(0)) {        _requireNotRevoked(_revocationKey(certHash));        if (certHash == ROOT_CA_CERT_HASH) {            return;        }        certHash = verifiedParent[certHash];    }    revert IncompleteCertChain();}

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");}

The revocation walk recurses all the way to the pinned root, because revocation of any ancestor must invalidate the whole chain beneath it. Expiry, by contrast, is only asserted for the two certificates directly in hand during a single verification call. Nothing walks further up the chain to check an earlier ancestor's expiry.

This is not a reachable false-accept against a genuine attestation bundle today. The only shipped caller of the hinted verification entrypoints always resubmits the full certificate bundle on every call, so every ancestor's expiry gets checked directly as that certificate is re-verified in its own right. The gap only opens up for a hypothetical caller that verifies a certificate directly against an already-cached parent without resubmitting the full chain above it — e.g. a future integration that caches a CA once and repeatedly verifies new leaves under it by hash, trusting the cache to still reflect a valid chain. Such a caller would have no way to learn that a grandparent has quietly expired.

Impact: No live exploit against the current codebase because the sole caller always re-supplies the full chain, so every ancestor's expiry is independently re-checked as a side effect of re-verifying it. The risk is latent — a future direct caller that relies on cache warmth for anything above the immediate parent would silently inherit a stale-chain acceptance bug that the public verification API gives no indication of.

Recommendation

Consider extending the revocation walk to also check expiry on every ancestor in the same pass, rather than only on the certificate being verified and its immediate parent. The explicit parent-expiry check performed separately today would become redundant once the walk covers the parent as its first iteration, and could be removed. This makes the invariant that every ancestor in a cached chain is unexpired enforced by the contract itself, independent of caller behavior.

Summary

versionPtr (the explicit [0] wrapper around the version INTEGER) is navigated past via its own self-declared outer length when computing sigAlgoPtr, but the inner INTEGER is only ever validated in isolation, via firstChildOf(versionPtr) + uintAt, without ever checking that the inner node's own length actually accounts for all of versionPtr's declared content. Any bytes left over inside the [0] wrapper, after the version INTEGER ends but before the wrapper's declared length is exhausted, are silently ignored.

Detailed Description

Asn1Ptr versionPtr = certificate.firstChildOf(ptr);_requireAsn1Tag(certificate, versionPtr, 0xa0);Asn1Ptr sigAlgoPtr = certificate.nextSiblingOf(certificate.nextSiblingOf(versionPtr));_requireAsn1Tag(certificate, sigAlgoPtr, 0x30);
if (certificate.keccak(sigAlgoPtr.content(), sigAlgoPtr.length()) != CERT_ALGO_OID) {    revert InvalidCertAlgorithm();}// as extensions are used in cert, version should be 3 (value 2) as per https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.1if (certificate.uintAt(certificate.firstChildOf(versionPtr)) != 2) revert InvalidCertVersion();

readNodeLength (via firstChildOf) parses the version INTEGER starting at versionPtr.content() using the INTEGER's own embedded length field. It has no notion of, and never checks against, versionPtr.length(). So versionPtr's declared outer length can validly exceed the number of bytes the INTEGER itself actually consumes; the excess is just unparsed content, never bound to any field and never checked against.

For example, a wrapper encoded as: A0 06 02 01 02 00 00 00 is a completely valid, tag-correct [0] wrapper of declared content-length 6, containing a canonical INTEGER 2 (02 01 02, i.e. X.509 version 3) followed by three arbitrary trailing bytes (00 00 00) that are still inside the wrapper's declared 6-byte span. _requireAsn1Tag(..., 0xa0) passes (correct outer tag), uintAt(firstChildOf(versionPtr)) returns 2 (the INTEGER parses fine and passes the version check). The three trailing bytes are never read, never hashed, and never rejected.

The documented certificate-parser hardening model states as an invariant that hardening changes should preserve: "signed TBS and extension structures do not contain trailing ignored fields." The version field lives inside the signed TBS, so this gap violates that stated invariant.

Impact: Not a reachable false-accept against a genuine AWS-signed certificate — reaching this gap requires a certificate whose TBS was already validly signed, so forging the trailing bytes without breaking the signature isn't possible. This is flagged as the same strict-DER-conformance / defense-in-depth gap as the parser's other known tag-substitution/trailing-byte gaps: the parser's own stated philosophy (and the doc's explicit "no trailing ignored fields" invariant) is to reject malformed-but-signed encodings rather than silently accept them, and this specific field is the one place that check was never added. Untested by the existing certificate test suite — no test appends trailing bytes inside the version wrapper's declared length.

Recommendation

Consider adding a boundary check that the version INTEGER fully consumes versionPtr's declared content, matching the child-end-equals-parent-end pattern already used for the signature wrapper and extensions.

Summary

Two nested wrappers in the SubjectPublicKeyInfo parsing path — the outer subjectPublicKeyInfoPtr and the inner pubKeyAlgoPtr — are both navigated into via firstChildOf/nextSiblingOf, but neither one ever verifies that its last child's end (content() + length()) actually reaches its own declared end. Any bytes placed inside either wrapper's declared content, after the last field the code reads but before the wrapper's declared length is exhausted, are silently skipped over and never hashed, parsed, or rejected.

Description

function _parsePubKey(bytes memory certificate, Asn1Ptr subjectPublicKeyInfoPtr)    internal    pure    returns (bytes memory subjectPubKey){    Asn1Ptr pubKeyAlgoPtr = certificate.firstChildOf(subjectPublicKeyInfoPtr);    _requireAsn1Tag(certificate, pubKeyAlgoPtr, 0x30);    Asn1Ptr pubKeyAlgoIdPtr = certificate.firstChildOf(pubKeyAlgoPtr);    Asn1Ptr algoParamsPtr = certificate.nextSiblingOf(pubKeyAlgoIdPtr);    Asn1Ptr subjectPublicKeyPtr = certificate.nextSiblingOf(pubKeyAlgoPtr);    Asn1Ptr subjectPubKeyPtr = certificate.bitstring(subjectPublicKeyPtr);
    if (certificate.keccak(pubKeyAlgoIdPtr.content(), pubKeyAlgoIdPtr.length()) != EC_PUB_KEY_OID) {        revert InvalidSubjectPublicKey();    }    if (certificate.keccak(algoParamsPtr.content(), algoParamsPtr.length()) != SECP_384_R1_OID) {        revert InvalidSubjectPublicKey();    }    ...}

nextSiblingOf/firstChildOf locate the next node purely from the current node's own header/length fields — they have no notion of, and never check against, an enclosing parent's declared length. So a parent wrapper's declared content length can validly exceed the sum of the lengths of the children the code actually walks; the excess is unparsed content that is never bound to any field and never checked.

This affects two wrappers here:

1. pubKeyAlgoPtr (AlgorithmIdentifier SEQUENCE): its two children, pubKeyAlgoIdPtr (the OID) and algoParamsPtr (nextSiblingOf(pubKeyAlgoIdPtr), the curve-parameters OID), are both hashed and checked — but nothing checks that algoParamsPtr.content() + algoParamsPtr.length() equals pubKeyAlgoPtr.content() + pubKeyAlgoPtr.length(). subjectPublicKeyPtr is then derived as nextSiblingOf(pubKeyAlgoPtr), jumping straight to pubKeyAlgoPtr's declared end — so any trailing bytes stuffed between the real end of algoParamsPtr and pubKeyAlgoPtr's declared end are stepped over and never examined.
2. subjectPublicKeyInfoPtr (SubjectPublicKeyInfo SEQUENCE, received as a parameter, already validated by the caller only for its own tag): its two children are pubKeyAlgoPtr (checked above) and subjectPublicKeyPtr (nextSiblingOf(pubKeyAlgoPtr), the BIT STRING containing the actual EC point). Nothing checks that subjectPublicKeyPtr.content() + subjectPublicKeyPtr.length() equals subjectPublicKeyInfoPtr.content() + subjectPublicKeyInfoPtr.length(). The caller, _parseTbsInner, computes extensionsPtr as nextSiblingOf(subjectPublicKeyInfoPtr) and does check extensionsPtr's end against tbsEnd — but that check only proves subjectPublicKeyInfoPtr's declared outer span is correct relative to the TBS; it says nothing about whether subjectPublicKeyInfoPtr's inner content (down to subjectPublicKeyPtr) is fully consumed by its two children with no gap.

For example, a SubjectPublicKeyInfo encoded as:

30 <len>   30 13                                  -- AlgorithmIdentifier, declared content-length 0x13 (19)      06 07 2A 86 48 CE 3D 02 01          -- id-ecPublicKey OID (9 bytes)      06 05 2B 81 04 00 22                -- secp384r1 OID (7 bytes)      00 00 00                           -- 3 arbitrary trailing bytes, still inside the 0x13 span   03 62 00 04 ...(96 bytes EC point)...   -- BIT STRING, subjectPublicKey

is a completely valid, tag-correct SubjectPublicKeyInfo: pubKeyAlgoPtr's declared content-length (0x13) exceeds the 16 bytes actually consumed by the two OIDs; subjectPublicKeyPtr is still found correctly via nextSiblingOf(pubKeyAlgoPtr) because that call trusts pubKeyAlgoPtr's declared length rather than where its children actually ended. Both OID checks pass, the EC point parses and is accepted — the three trailing bytes inside pubKeyAlgoPtr are never read, hashed, or rejected. The same construction applies one level up: padding subjectPublicKeyInfoPtr's own declared length past where subjectPublicKeyPtr (the BIT STRING) actually ends hides bytes that are likewise never checked, since extensionsPtr is located via subjectPublicKeyInfoPtr's declared length, not via where subjectPublicKeyPtr actually ends.

The project's documented certificate-parser hardening model states as an invariant that hardening changes should preserve: "signed TBS and extension structures do not contain trailing ignored fields." SubjectPublicKeyInfo lives inside the signed TBS, so this gap violates that stated invariant, even though — like the parser's other tag-substitution/trailing-byte gaps — it doesn't currently open a false-accept path, since these wrappers' bytes are covered by the certificate's own signature.

Impact: Not a reachable false-accept against a genuine AWS-signed certificate — reaching this gap requires a certificate whose TBS was already validly signed, so forging the trailing bytes without breaking the signature isn't possible. This is flagged as the same strict-DER-conformance / defense-in-depth gap as the parser's other known tag-substitution/trailing-byte gaps: the parser's own stated philosophy (and the doc's explicit "no trailing ignored fields" invariant) is to reject malformed-but-signed encodings rather than silently accept them, and SubjectPublicKeyInfo's two wrappers are two more places that check was never added. Untested by the existing certificate test suite — no test appends trailing bytes inside either the AlgorithmIdentifier or SubjectPublicKeyInfo wrapper's declared length.

Recommendation

Consider adding boundary checks that the last child of each wrapper fully consumes the wrapper's declared content, matching the child-end-equals-parent-end pattern already used for the signature wrapper, extensions, and the recommended version-wrapper fix: after computing algoParamsPtr, verify its end (content() + length()) equals pubKeyAlgoPtr's declared end, reverting with InvalidSubjectPublicKey otherwise; and after computing subjectPublicKeyPtr, verify its end equals subjectPublicKeyInfoPtr's declared end, reverting the same way if it doesn't.

Summary

The leaf/client certificate has no length bound, unlike every cabundle entry.

Description

bytes memory cert = attestationTbs.slice(ptrs.cert);bytes[] memory cabundle = new bytes[](ptrs.cabundle.length);for (uint256 i = 0; i < ptrs.cabundle.length; i++) {    require(1 <= ptrs.cabundle[i].length() && ptrs.cabundle[i].length() <= 1024, "invalid cabundle cert");    cabundle[i] = attestationTbs.slice(ptrs.cabundle[i]);}

Every cabundle[i] entry is bounded to [1, 1024] bytes, but ptrs.cert (the leaf/client certificate) has no equivalent bound anywhere in NitroValidator or in CertManager's direct verifyClientCertWithHints/verifyCACertWithHints entrypoints.

Impact: Self-funded gas only, i.e. the caller pays for parsing an oversized blob. It is not exploitable against other users or the shared cache. However, this is an inconsistency gap.

Recommendation

Consider applying the same [1, 1024]-style bound to ptrs.cert.length() for consistency with the cabundle entries.

Summary

Privileged roles owner and revoker are addresses with the power to instantly revoke or unrevoke any cached certificate identity, including a global halt. Their operational security risk should be mitigated appropriately.

Description

Privileged revoker role can instantly revoke any cached (issuer, serial) identity. Because the verified-cert cache is global and shared across all integrators, a compromised or malicious revoker key can immediately break validation for every consumer relying on a given cached cert, i.e. a live DoS vector against the shared cache, not just the revoker's own usage.

Privileged owner role can revoke ROOT_CA_CERT_HASH as an "emergency global halt" (by design), but can also call unrevokeCert on any revoked identity including one that was correctly revoked because AWS itself compromised or revoked the underlying cert via CRL with no timelock or secondary approval.

Both roles implement a role rotation (transferOwnership, setRevoker) that is a single-step operation. transferOwnership sets owner = newOwner immediately in a single call, which allows a mistyped address or a newOwner whose key is lost/inaccessible to permanently lock the contract out of its owner role, since nothing else can call transferOwnership again.

Recommendation

Consider:

1. OpenZeppelin-Ownable2Step-style two-step ownership transfer pattern for ownership transfers. Or
2. Documenting expectations for the production deployment:
   • owner and revoker held by a reasonable multisig (not an EOA)
   • timelock on owner actions
   • monitoring/alerting on every revoker/owner transaction
   • key-rotation/incident-response playbook for different compromise scenarios.

Coinbase

PR #78

Cantina

PR #78 documents expectations for the production deployment.

Description

The revocation functions write the requested value and emit an event without checking whether the certificate is already in that state:

function unrevokeCert(bytes32 certId) external onlyOwner {    revoked[certId] = false;    emit CertUnrevoked(certId, msg.sender);}
function _revokeCert(bytes32 certId) internal {    revoked[certId] = true;    emit CertRevoked(certId, msg.sender);}

Revoking an already revoked certificate therefore emits another revocation event even though no state changes. Restoring an already active certificate similarly emits another restoration event.

The stored status remains correct, but monitoring systems may interpret duplicate events as real state changes, creating unnecessary noise or a misleading activity history.

Recommendation

Emit an event only when the certificate status changes. A repeated call can return without an event or be rejected.

Description

verifyCachedCertBundle() returns the verified leaf certificate whose public key is used to verify the attestation signature. However, the returned value is named parent:

ICertManager.VerifiedCert memory parent = verifyCachedCertBundle(cert, cabundle);bytes memory hash = Sha2Ext.sha384(attestationTbs, 0, attestationTbs.length);require(    p384Verifier.verifyP384SignatureWithHints(        hash,        signature,        parent.pubKey,        attestationSigHints    ),    "invalid sig");

The variable does not contain the parent certificate. The name does not currently change validation behavior, but it can confuse maintainers and increase the chance of using the wrong certificate or public key in future changes.

Recommendation

Rename the variable to leafCert, signingCert, or another name that clearly identifies the certificate it contains.

Summary

ECDSA384.verifyWithHintsConsumed is a local addition to the vendored library that returns the verification result and a hint-consumption count, without enforcing that the caller-supplied hint stream was fully consumed unlike its sibling verifyWithHints, which enforces exact consumption itself. Nothing in this repository calls verifyWithHintsConsumed: the only shipped consumer of the hinted verification family, P384Verifier, calls verifyWithHints.

Detailed Description

Both functions share the same private verification core, which reports the pass/fail result plus how many hint bytes were actually read. verifyWithHints closes the loop itself, reverting unless every hint byte was consumed, enforcing the general practice of rejecting ambiguous/partially-consumed encodings rather than tolerating them. verifyWithHintsConsumed skips that check and hands the consumption count back to the caller instead. And since the only real entry point into hinted verification calls verifyWithHints, that responsibility is never actually exercised anywhere in the repository, including in tests.

Unconsumed hint bytes can't cause a false accept today: the pass/fail result depends only on a deterministic scalar comparison, never on the consumption count, and every hint is independently checked on-chain before use. So the worst case of calling this function and ignoring the count is accepting a hint blob with unread trailing bytes, not a forged signature. But that safety only holds because nothing currently relies on the count. It isn't enforced by the function itself, so a future caller (in this repo or a downstream fork of the vendored library) that reaches for this variant inherits that gap silently, with no warning that they now own an invariant the sibling function otherwise provides automatically.

Recommendation

Consider removing verifyWithHintsConsumed and its unused test mirror; neither has a caller, and verifyWithHints already covers the only verification path this codebase needs. If a future use case genuinely needs the raw consumption count, reintroduce it then with explicit documentation that the caller owns validating it.

Summary

bitstring panics with a raw array out-of-bounds access on a zero-length BIT STRING unlike its sibling bitstringUintAt that has a zero-length guard.

Description

Commit bc53b17 (#39) added a zero-length guard to bitstringUintAt so that a BIT STRING missing its mandatory unused-bits octet reverts cleanly with InvalidAsn1Length. The same guard was never added to bitstring, its sibling helper. On an identical zero-length input, bitstring reads past the node's bounds and panics with a raw Panic(0x32) instead of a typed revert.

Both outcomes fail closed, so this isn't a false-accept. It's a validation-consistency gap: two sibling helpers decoding the same field handle the identical malformed input differently, one matching this parser's stated philosophy of typed reverts on malformed encodings and one not.

Recommendation

Consider adding the same if (ptr.length() == 0) revert InvalidAsn1Length(); guard to bitstring that bc53b17 added to bitstringUintAt.

Summary

Both owner and revoker default to the deploying address at deployment and rely on post-deployment role rotation.

Description

owner controls ownership transfer, revoker updates, and the emergency halt; revoker can revoke individual certs. Correctness of this privileged setup depends entirely on post-deployment transferring both roles to a hardened wallet/multisig. Until that transfer happens, a compromised or merely careless deployer key can revoke certs or trigger the emergency halt, unilaterally denying service. Stale deployer-held admin roles left untransferred after deployment are a recurring root cause of real-world exploits, not a hypothetical concern.

Recommendation

Consider:

1. Accepting the intended owner/revoker as constructor arguments so they're set atomically at deployment instead of via a follow-up transaction
2. Documenting/enforcing transferring owner and revoker immediately post-deployment
3. Adding monitoring that flags if either role still equals the deployer after an expected window

Summary

Public/external functions in CertManager.sol (e.g. verifyCACertWithHints, verifyClientCertWithHints, loadVerified, computeCertId, transferOwnership, setRevoker, revokeCert, revokeCerts, unrevokeCert) have @notice/@dev tags but no @param or @return tags documenting their arguments and return values.

Recommendation

Consider adding @param and @return tags to the external/public interface so generated docs and IDE tools fully describe each function's inputs and outputs.

Summary

verifyCachedCertBundle is internal but is not prefixed with an underscore, deviating from the codebase's naming convention for internal/private functions.

Recommendation

Consider renaming to _verifyCachedCertBundle (and update its call sites) for consistency.