Security-focused walk through decrypt_legacy_encrypted_pem. The implementation faithfully follows the legacy OpenSSL PEM format (RFC 1421-style with Proc-Type: / DEK-Info:), and the header-parsing path is reasonably tight. CI 133/136 success.

No non-constant-time comparisons on secret data

Went through every comparison in the new code. All string/byte comparisons (proc_type != Some("4,ENCRYPTED"), cipher_name match arms, dek_info.split_once(',')) operate on the public PEM header — not on derived keys, plaintext, or padding bytes. No timing channel introduced at the parsing layer. ✓

Real concern: PKCS#7 unpadding is not constant-time (potential padding oracle)

cbc::Decryptor::<…>::new_from_slices(...).decrypt_padded_vec_mut::<Pkcs7>(...) delegates to block-padding 0.3.3's Pkcs7::unpad, which short-circuits on the first padding-byte mismatch. CBC + early-exit padding check is the classic Vaudenay-2002 padding oracle surface. Mitigations already in place help but don't fully close the channel:

• All decryption failures map to one error variant (InvalidEncryptedPemPrivateKey) — good; the user can't distinguish "wrong cipher" / "wrong passphrase" / "wrong padding" / "valid padding but malformed inner DER" through the error class. ✓
• But timing alone can still leak: a malformed-padding ciphertext returns before the inner-DER parse, while a valid-padding-but-malformed-DER ciphertext does the full PKCS#7 unpad + DER parse before erroring. That's the timing differential a padding oracle exploits.

For the one-shot local-PEM use case (a developer calling crypto.createPrivateKey({ key: pemString, passphrase }) with their own key on their own machine) this isn't reachable. But for server-side use cases — e.g., a service that imports user-supplied encrypted PEMs and returns any observable signal (timing, error class, retry behaviour) per attempt — this is exploitable. Worth either:

1. Switching to a constant-time PKCS#7 unpad if available (the subtle crate has variants), or
2. Adding an explicit doc note that crypto.createPrivateKey with legacy Proc-Type: PEM should not be used in network-exposed adversarial contexts.

Inherent legacy-format weaknesses worth documenting

These aren't bugs in this PR — they're properties of the format being adopted — but they're now Deno-supported attack surface:

1. evp_bytes_to_key is MD5-based, single-pass with no iteration count (keys.rs:3293–3317). Brute-force / dictionary attacks on passphrases are trivial by modern standards. PBKDF1/MD5 has been deprecated since the early 2000s.
2. Salt = first 8 bytes of the IV (keys.rs:3431–3433). This isn't a bug — it's the spec — but it means salt and IV aren't independently random, so the KDF and the cipher share entropy. For DES-EDE3-CBC where the IV is exactly 8 bytes, salt = entire IV.
3. No MAC, CBC-only. Ciphertext is malleable; without authentication, the padding oracle is the only "is this decryption valid?" check.

A // SECURITY: comment block at the top of decrypt_legacy_encrypted_pem flagging these as known-weak (and pointing users at PKCS#8/PBES2 for new keys) would help future readers and document the threat model deliberately.

Other concerns I checked and discarded

• IV length check ordering (keys.rs:3434–3436): iv.len() != expected_iv_len runs before salt.copy_from_slice(&iv[..8]), and min expected_iv_len is 8 across all supported ciphers, so the slice is always in-bounds. ✓
• Hex-decode and base64-decode: both errors mapped to a single error variant; no panic path on malformed input. ✓
• pem.contains("Proc-Type:") as the routing predicate (keys.rs:1378): substring match against the full PEM, including base64 body. Base64 alphabet has no :, so a body-level false positive isn't constructible from valid base64. ✓
• End label not verified against begin label: for line in lines { if line.starts_with("-----END ") { break; } accepts a mismatched END label (e.g. BEGIN RSA PRIVATE KEY paired with END EC PRIVATE KEY). The downstream DER parse for the assumed label will reject mismatched content with InvalidPkcs1PrivateKey/InvalidSec1PrivateKey/etc., so it's not a security issue, just lenient. Robustness nit.
• Header injection / parser confusion: only Proc-Type: and DEK-Info: are accepted; any other header line is rejected with InvalidPemPrivateKey. ✓

Summary

Not blocking from a substance standpoint — the code is correct for the spec. The one finding worth a follow-up before merge is the padding-oracle-resistant unpadding question (constant-time PKCS#7 vs current block-padding 0.3.3 short-circuit behaviour). Whether that ships in this PR or as a // TODO(security) comment + tracking issue is a maintainer call. The other items (weak KDF, salt=IV[..8], no MAC) are format-level constraints worth documenting as known-weak in the function header.

Re-reviewed at 43b6488. The new // SECURITY: block on decrypt_legacy_encrypted_pem (keys.rs:3438–3457 per the hunk header) is exactly the disclosure I was asking for in my prior security review — it covers all four format-level weaknesses (MD5 EVP_BytesToKey with no iteration count, salt = first 8 bytes of IV, CBC + no MAC, PKCS#7 padding non-constant-time in block-padding 0.3.x), correctly notes that the single-error-variant mapping is a partial mitigation but doesn't close the timing channel, and gives clear usage guidance: "Callers should treat this path as 'import a local key file', not 'decrypt attacker-supplied PEMs as a service'."

That's the right level of resolution given the constraints — implementing a constant-time PKCS#7 unpad would require either a subtle-based reimplementation or upgrading block-padding, neither of which belongs in this PR. The documentation makes the threat model explicit so future readers (and downstream auditors) can reason about which call sites are safe.

No other substance changes from my prior pass — decrypt_legacy_encrypted_pem shape is the same (header parse, PBES1 KDF via evp_bytes_to_key, AES-CBC / 3DES decrypt with PKCS#7 unpad), and the public-key path at keys.rs:1382 still routes through the private-key handle and derives via to_public_key(). All my non-blocking nits from before still apply only as future-cleanup ideas (END label not verified against BEGIN, etc.).

CI

Fresh CI run after the force-push. Release shards across all 4 platforms are green for both test node_compat and test unit_node; lint debug linux/macos green, lint debug windows in flight. Debug shards on macos-aarch64 / linux-aarch64 / linux-x86_64 still running. Holding COMMENT one more cycle until those land — but barring a surprise red, this is APPROVE-ready from my side.

The coordination point with #33762 (which has the parallel parse_legacy_encrypted_pem) still stands: pick one canonical implementation across the two PRs before both merge.

Single new commit since my prior security walk: 23f1f35 — "fix(ext/node_crypto): route Proc-Type/DEK-Info PEMs through private key path". Substance is sound: createPublicKey({ key: encryptedPem, passphrase }) was failing because decode_pem_lenient base64-decodes the entire PEM body including the Proc-Type: 4,ENCRYPTED / DEK-Info: AES-128-CBC,... header lines (those aren't valid base64), so the parse failed at the public-key path before the encrypted-PEM decryption ever ran. The fix detects pem.contains("Proc-Type: 4,ENCRYPTED") early in the public-key path, routes through new_asymmetric_private_key_from_js (which knows how to handle the legacy format), then converts back via to_public_key(). Right shape — the alternative would have been to teach decode_pem_lenient to skip non-base64 header lines, which is broader surface than this targeted route.

The earlier security disclosure on decrypt_legacy_encrypted_pem is unchanged and still adequate.

CI failure is from main, not from this PR

The 9 build debug * shard failures across linux/macos/windows are all the same panic:

thread 'main' panicked at libs/core/runtime/jsruntime.rs:2564:9:
Failed to initialize JsRuntime for snapshotting: CoreError(JsBox(JsErrorBox { class: "Error", message: "Specifier \"ext:deno_web/08_text_encoding.js\" was not passed as an extension module and was not included in the snapshot." }))

This PR's diff only touches ext/node_crypto/keys.rs (+19/-0) and tests/node_compat/config.jsonc (+1) — neither of which references 08_text_encoding.js. So this is not a regression introduced by 33769.

Verified that main itself is currently broken: the most recent main CI run (f14cb96, in_progress) and the previous two completed runs (349f015 from PR #33795, 09c6782 from PR #33708) all panic with the same 08_text_encoding.js was not passed as an extension module snapshot error. Main is in a broken-build state right now, and every PR that merges main inherits the breakage.

The likely culprit is one of the recent lazy_loaded_js conversions that converted 08_text_encoding.js from ESM to script form without updating all consumers — same shape as the OffscreenCanvas regression that hit #33760. Probably worth a separate fix-main PR before any of the in-flight feature PRs can land cleanly.

Not blocking on substance — happy to come back once main is unbroken and 33769's CI re-runs cleanly. PR is APPROVE-shape from my side.