feat: bypass SSL AEAD wrapper in RecordProtection

[larseggert]

Replace SSLExp_AeadEncrypt/Decrypt with direct PK11_AEADOp calls, removing the ssl_AeadInner overhead (sslBuffer allocation, tls13_WriteNonce) that fired on every QUIC packet.

Key derivation is replicated using SSL_HkdfExpandLabelWithMech (already bound in hp.rs).

Also extract xor_nonce as a shared free function, eliminating the duplicate nonce-XOR logic between RecordProtection and Aead.

Benchmarks: mozilla/neqo#3600 (comment)

[Copilot]

Pull request overview

This PR replaces the SSLExp_AeadEncrypt/SSLExp_AeadDecrypt wrapper-based QUIC RecordProtection implementation with direct PK11_AEADOp usage, aiming to reduce per-packet overhead while preserving NSS-compatible key/IV derivation.

Changes:

• Re-implements RecordProtection to derive key/iv via SSL_HkdfExpandLabelWithMech and perform AEAD directly with PK11_AEADOp.
• Splits encryption/decryption into separate PKCS#11 contexts (ctx_encrypt / ctx_decrypt) and stores nonce_base locally.
• Extracts duplicated nonce XOR logic into a shared xor_nonce helper.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

Summary

Clean refactoring that replaces the SSLExp_AeadEncrypt/Decrypt wrappers with direct PK11_AEADOp calls and manual HKDF key/IV derivation. The approach is sound: it removes the per-packet sslBuffer allocation and tls13_WriteNonce overhead from the hot path, and the extracted xor_nonce helper eliminates the duplicate nonce-XOR logic.

The label derivation via SSL_HkdfExpandLabelWithMech correctly replicates what SSL_MakeAead did internally (concatenating the prefix with "key"/"iv" and using HKDF-Expand-Label per RFC 8446 §7.3). Splitting into separate encrypt/decrypt PK11Contexts is also an improvement — it removes shared mutable state if callers ever need directional parallelism.

Two issues worth addressing before merge:

• The cast_mut() on the tag pointer in decrypt creates a soundness hazard (details in inline comment).
• An arithmetic overflow in the encrypt output-length check (details in inline comment).

[github-actions]

Clean refactoring that removes the ssl_AeadInner per-packet overhead (sslBuffer allocation, tls13_WriteNonce) from the QUIC hot path by calling PK11_AEADOp directly. The key/IV derivation via SSL_HkdfExpandLabelWithMech correctly replicates what SSL_MakeAead did internally per RFC 8446 §7.3, and the split into separate encrypt/decrypt PK11Contexts is a structural improvement. The extracted xor_nonce helper is a nice DRY win.

All issues raised in prior reviews (nonce fixed-portion parameter, checked_add overflow guard, cast_mut() soundness fix via tag copy, safety doc overlap clause, output_max sizing, and cipher-suite test coverage) have been addressed in 0646049.

Two minor items remain (see inline comments):

• decrypt_in_place tag/output overlap vs. the aead_op safety doc — a 16-byte stack copy fixes it consistently with decrypt.
• In-place roundtrip test coverage for AES-256-GCM and ChaCha20-Poly1305.

Note: the pre-existing cast_mut() on a shared-reference tag pointer in Aead::decrypt (the lower-level AEAD, not RecordProtection) has the same soundness hazard that this PR fixed for RecordProtection::decrypt. Worth a follow-up.

[github-actions]

Sound refactoring that replaces RecordProtection's dependency on NSS's experimental SSL AEAD API (SSL_MakeAead/SSL_AeadEncrypt/SSL_AeadDecrypt) with the stable PKCS#11 PK11_AEADOp interface. The key/IV derivation via SSL_HkdfExpandLabelWithMech correctly replicates TLS 1.3 HKDF-Expand-Label per RFC 8446 §7.1, and the nonce construction follows §5.3.

The constructor trades one SSL_MakeAead call for two PK11_CreateContextBySymKey calls plus an HKDF extraction, but removes per-packet sslBuffer allocation and tls13_WriteNonce overhead — a clear win for the QUIC data path. Splitting into separate encrypt/decrypt PK11Contexts is a structural improvement over the old single-context design. The public API surface is unchanged.

Most issues from earlier review rounds have been addressed in 1d44782 (nonce fixed-portion parameter, checked_add guard, cast_mut() soundness fix in decrypt, safety-doc overlap clause, output_max sizing, and AES-256/ChaCha20 roundtrip tests). Two items remain unresolved — see inline comments for details and suggested fixes.

[github-actions]

Sound refactoring that replaces RecordProtection's dependency on NSS's experimental SSL AEAD API (SSL_MakeAead/SSL_AeadEncrypt/SSL_AeadDecrypt) with the stable PKCS#11 PK11_AEADOp interface. The key/IV derivation via SSL_HkdfExpandLabelWithMech correctly replicates TLS 1.3 HKDF-Expand-Label per RFC 8446 §7.1, and the nonce construction follows §5.3. The empty HKDF context (null(), 0) is correct for key/IV derivation.

All issues from prior review rounds have been addressed in the fixup commits:

• Nonce fixed-portion parameter (NONCE_LEN - COUNTER_LEN) ✓
• checked_add overflow guard in encrypt ✓
• cast_mut() soundness fix in decrypt (tag copied to stack buffer) ✓
• decrypt_in_place tag/output overlap fix (tag copied to stack buffer, matching decrypt) ✓
• Safety doc overlap/aliasing clause ✓
• output_max sizing for decrypt methods ✓
• AES-256-GCM and ChaCha20-Poly1305 roundtrip test coverage (including _in_place paths) ✓

One minor inline comment on a consistency nit (double as_mut_ptr() in encrypt).

Follow-up note: The pre-existing cast_mut() on a shared-reference tag pointer in Aead::decrypt (flagged in the prior review round) still has the same soundness hazard that this PR fixed for RecordProtection::decrypt. Worth a separate follow-up.

[github-actions]

Clean, well-motivated refactoring. Replacing the experimental SSL_MakeAead/SSL_AeadEncrypt/SSL_AeadDecrypt wrappers with direct PK11_AEADOp calls removes per-packet sslBuffer allocation and tls13_WriteNonce overhead from the QUIC data path. The key/IV derivation via SSL_HkdfExpandLabelWithMech correctly replicates TLS 1.3 HKDF-Expand-Label (RFC 8446 §7.1), and nonce construction follows §5.3. The public API surface is unchanged.

All issues raised in prior review rounds have been addressed in the fixup commits: nonce fixed-portion parameter, checked_add overflow guard, cast_mut() soundness fix via tag copy, safety doc overlap clause, output_max sizing, and full cipher-suite roundtrip coverage.

Splitting into separate encrypt/decrypt PK11Contexts is a structural improvement. The xor_nonce extraction is a good DRY win. The #[cfg_attr(..., path = "...")] module selection is idiomatic.

Follow-up note: The pre-existing cast_mut() on a shared-reference tag pointer in Aead::decrypt (the lower-level AEAD, not RecordProtection) has the same soundness hazard that this PR correctly fixed for RecordProtection::decrypt by copying the tag to a stack buffer. Worth a separate follow-up.

[github-actions]

Sound refactoring that replaces RecordProtection's dependency on NSS's experimental SSL AEAD API (SSL_MakeAead/SSL_AeadEncrypt/SSL_AeadDecrypt) with the stable PKCS#11 PK11_AEADOp interface, removing per-packet sslBuffer allocation and tls13_WriteNonce overhead from the QUIC data path.

The key/IV derivation via SSL_HkdfExpandLabelWithMech correctly replicates TLS 1.3 HKDF-Expand-Label per RFC 8446 §7.1, and nonce construction follows §5.3. The empty HKDF context (null(), 0) is correct for key/IV derivation. Using CKM_HKDF_DATA for the IV expansion is the right choice — it makes the key data extractable for the nonce_base read.

All issues from prior review rounds have been addressed in the fixup commits:

• Nonce fixed-portion parameter (NONCE_LEN - COUNTER_LEN) ✓
• checked_add overflow guard in encrypt ✓
• cast_mut() soundness fix in decrypt/decrypt_in_place (tag copied to stack buffer) ✓
• Safety doc overlap/aliasing clause on aead_op ✓
• output_max sizing for decrypt methods ✓
• Null-impl encrypt_in_place underflow guard ✓
• xor_nonce extraction replacing make_nonce (DRY win) ✓
• #[cfg_attr(..., path = "...")] module selection ✓
• AES-256-GCM and ChaCha20-Poly1305 roundtrip coverage (including _in_place paths) ✓

Splitting into separate encrypt/decrypt PK11Contexts is a structural improvement over the old single-context design. The public API surface is unchanged.

One minor inline comment on the null implementation's encrypt missing a bounds check (consistency with the real impl and the already-fixed encrypt_in_place).

Follow-up note: The pre-existing cast_mut() on a shared-reference tag pointer in Aead::decrypt has the same soundness hazard that this PR correctly fixed for RecordProtection::decrypt by copying the tag to a stack buffer. Worth a separate follow-up.

[github-actions]

Clean, well-motivated refactoring. Replacing the experimental SSL_MakeAead/SSL_AeadEncrypt/SSL_AeadDecrypt wrappers with direct PK11_AEADOp calls is a clear win for the QUIC data path — removes per-packet sslBuffer allocation and tls13_WriteNonce overhead. The key/IV derivation via SSL_HkdfExpandLabelWithMech correctly replicates TLS 1.3 HKDF-Expand-Label (RFC 8446 §7.1), nonce construction follows §5.3. Public API surface is unchanged.

The xor_nonce extraction is a good DRY win, the #[cfg_attr(..., path = "...")] module selection is idiomatic, and splitting into separate encrypt/decrypt PK11Contexts is a structural improvement. Test coverage now spans all three cipher suites with both separate-buffer and in-place roundtrip paths.

Requesting changes for one blocking issue: the src/p11.rs change reverts RandomCache::SIZE from 4096 back to 256, undoing PR #51 — this appears to be an accidental rebase artifact. The Aead::decrypt cast_mut() soundness issue in mod.rs:242 is pre-existing but worth fixing in this PR since the identical pattern was already correctly fixed for RecordProtection::decrypt.

Stale