[pablobaro]

Summary

Importing an untrusted PKCS#12 container currently allows two denial-of-service vectors, both reachable before password verification completes — so an attacker only needs to get a crafted .p12/.pfx file parsed, not decrypted. I hit both while integrating p12-keystore as the PKCS#12 backend of an offline PDF-signing application, where the PFX file is untrusted user input.

Two focused commits, each with a dedicated rejection test. The full existing suite still passes (42 tests, unchanged).

1. Issuer-cycle hang → memory exhaustion (src/keystore.rs)

KeyStore::from_pkcs12 builds each certificate chain by repeatedly looking up the certificate whose subject matches the current issuer, with no record of which certificates it has already visited:

while let Some(issuer) = parsed_certs
    .iter()
    .find(|c| c.cert.subject == entry.cert.issuer && !c.trusted)
{
    // ... push issuer, continue ...
}

If two certificates name each other as issuer (cert A issued by B, cert B issued by A — trivial to craft, and also producible by accident with cross-signed CAs), the lookup alternates between them forever. The certs vector grows on every iteration until the process is OOM-killed. This is reachable purely from parsing an untrusted container.

Fix: track visited certificates by their DER identity and stop when an issuer repeats. Acyclic chains keep exactly their previous shape — the change only adds a terminating condition to a loop that could previously run unbounded.

let mut visited = BTreeSet::from([entry.cert.as_der()]);
while let Some(issuer) = /* ... */ {
    if !visited.insert(issuer.cert.as_der()) {
        break;
    }
    // ...
}

2. Unbounded KDF work factors → CPU / memory exhaustion (src/codec.rs)

The MAC iteration count, the PBES1 iterations, the PBES2/PBKDF2 iterationCount, and all three scrypt parameters are read straight from the container DER with no upper bound. A crafted file can therefore request:

• up to i32::MAX / u32::MAX KDF rounds → the key-derivation runs for effectively forever (CPU exhaustion), or
• scrypt parameters whose memory cost 128 * N * r bytes is unbounded → immediate memory exhaustion / abort. Note this is not just N: the block size r and parallelism p are u16 values that also come straight from the DER, so bounding N alone leaves the cost unbounded via a large r (e.g. a modest N with r = 65535).

Worth flagging explicitly, because it's easy to assume pkcs5 already covers this: pkcs5 only enforces its MAX_ITERATION_COUNT inside the Pbkdf2Params::hmac_sha256 builder. The DER decode path (TryFrom<AnyRef>) fills the parameters without any check, so values parsed from an untrusted container are never bounded unless the consumer bounds them. That's why the guard has to live here.

Fix: before any derivation runs, reject with Error::InvalidParameters:

• iteration counts (MAC, PBES1, PBES2/PBKDF2) above 1,000,000;
• scrypt effective memory 128 * N * r above 1 GiB, computed with checked_mul so an overflowing product is rejected rather than wrapping;
• scrypt parallelism p above 16.

The limits are deliberately generous — well above what mainstream tooling produces (OpenSSL's PKCS#12 default is 2048 iterations) — so no interoperable, honestly-produced container is rejected.

Testing

• cargo test — 44 passing, including a rejection test per guard: cyclic-chain import terminates; over-limit MAC, PBES1, and PBES2/PBKDF2 iterations rejected; and scrypt rejected for an oversized N, for a large block size r with an otherwise-valid N, and for excessive parallelism p — each before the KDF runs.
• Both fixes have been running in production (vendored) against real and adversarial PFX inputs.

Happy to adjust the limits, split the commits differently, or add more test vectors if you'd prefer.

[pablobaro]

Good call — done, pushed as an update to the first commit. Certificate already derives PartialEq, so certs.contains(&issuer.cert) works directly and drops the extra set plus its import.

One thing I double-checked while switching: the loop skips pushing an issuer whose subject == leaf_cert.subject, so such a cert never lands in certs. That turns out to be safe — to keep the chain going past it, its issuer must differ from its subject (otherwise the existing self-signed break fires), which means the next hop is a non-self-signed cert that does get pushed and so is caught on its second appearance. The two-cert A↔B cycle test still passes.

[ancwrd1]

This AI-generated comment looks completely redundant because the function name already explains it.

[ancwrd1]

Rustdoc-style comments (///) are not needed for non-public members. Replace with normal comments (and below as well).

[ancwrd1]

The comment is too verbose, we don't want every change to be documented like this. It could be skipped completely or be just // prevent infinite loop in case of cycle certificate chains

[ancwrd1]

Replace with normal comments.

[pablobaro]

Done — addressed all four: /// on private items switched to plain //, the redundant scrypt_within_limits doc removed, the test helper's comment de-doc'd, and the cycle-guard comment cut down to // prevent infinite loop in case of cycle certificate chains. Pushed.