Noise sv2 hardening

[bit-aloo]

closes: #2248
closes: https://github.com/project-loupe/audit-stratum/issues/21
closes: #2249
closes: https://github.com/project-loupe/audit-stratum/issues/41
closes: #2250
closes: https://github.com/project-loupe/audit-stratum/issues/22
closes: #2251
closes: https://github.com/project-loupe/audit-stratum/issues/80
closes: #2252
closes: https://github.com/project-loupe/audit-stratum/issues/61
closes: https://github.com/project-loupe/audit-stratum/issues/67
closes: https://github.com/project-loupe/audit-stratum/issues/98

companion stratum-mining/sv2-apps#706

[bit-aloo]

To be merged after: #2270

[bit-aloo]

Some adaptation needs to be made to sv2-apps.

[GitGab19]

Clanker review:

Findings

1. Remove the unsupported AES-GCM reference

   The updated Initiator documentation says transport supports either ChaCha20-Poly1305 or AES-GCM. However, following #2270, NoiseEngine contains only ChaCha20Poly1305 ciphers.

   Changed documentation · NoiseEngine implementation

   Suggested wording:

   After the handshake, it facilitates secure communication using [ChaCha20Poly1305]. Sensitive data is securely erased when no longer needed.

2. Narrow the stated CryptoRng guarantee

   The new comment says that CryptoRng rejects deterministic generators. It does not enforce that guarantee: deterministic CSPRNGs such as a seeded StdRng implement CryptoRng, as demonstrated by the tests in this crate.

   Comment · Seeded StdRng usage

   The bound itself is appropriate; only the explanation should be adjusted. For example:

   The CryptoRng bound requires generators that declare themselves suitable for cryptographic use.

   Seed quality and correct CryptoRng implementations remain the caller’s responsibility.

3. Add direct regression coverage for split transport state

   The new TransportEncryptState / TransportDecryptState path currently has no dedicated test in this PR. Please add a round-trip test covering both directions and sequential state reuse to protect the cipher direction and nonce-counter behavior.

[bit-aloo]

@GitGab19 added the test, should good now.

[GitGab19]

Second round of clanker review:

Review — Noise sv2 hardening

Concept ACK — the erase() fix is a genuine bug fix, and removing Clone from the cipher-bearing types is the right layer to fix nonce duplication at. Verified locally: cargo test -p codec_sv2 -p noise_sv2 --all-features passes (14 + 17), clippy adds no new warnings on the touched crates, and the semver-check baselines (noise_sv2 1.4.2 → 2.0.0, codec_sv2 6.0.0 → 7.0.0) already absorb the Clone removals and the added CryptoRng bounds.

Findings below are additional to the three already resolved in this thread (AES-GCM doc reference, CryptoRng wording, split-transport test).

#	Finding	Crate	Severity
1	Derived key material — including the transport session keys — is never wiped	noise_sv2	Medium
2	The Cipher thread-safety comment is factually wrong	noise_sv2	Low
3	erase() doc claim still inaccurate after the fix	noise_sv2	Low
4	A failed chunk encryption poisons the encoder for all later frames	codec_sv2	Medium
5	The split is opt-in; a shared-direction transport is still constructible	codec_sv2	Medium (design)
6	split_transport consumes the state on its error path	codec_sv2	Low
7	Example and test helper still model the pre-split API	codec_sv2	Low
8	New round-trip test misses the chunked path and decoder reuse	codec_sv2	Low
9	encode_transport / next_transport_frame naming mismatch	codec_sv2	Nit

---

noise_sv2

1. The derived key material is never wiped — including the transport session keys

Severity: Medium · initiator.rs:373-381, responder.rs:364-373, handshake.rs:171-202

This PR makes erase() actually wipe the ck and h fields, which is the right fix. But every derived key that transits the stack on the way into the cipher is left untouched:

let (temp_k1, temp_k2) = Self::hkdf_2(self.get_ck(), &[]);   // the two transport session keys
let c1 = ChaCha20Poly1305::new(&temp_k1.into());             // + a GenericArray copy of each
let c2 = ChaCha20Poly1305::new(&temp_k2.into());
let c1: Cipher<ChaCha20Poly1305> = Cipher::from_key_and_cipher(temp_k1, c1);  // + a copy in Cipher.k
// ...
encryptor.erase_k();   // only wipes Cipher.k
decryptor.erase_k();

erase_k() wipes the copy stored inside Cipher, but temp_k1 / temp_k2 — the live transport session keys — are dropped without a wipe, and their stack slots retain them. The same applies to hkdf_2's intermediate temp_key (the HKDF PRK, from which both session keys are recoverable) and to mix_key's temp_k on every handshake step.

This is squarely the theme of #2248/#2249, and arguably higher-value than ck/h: ck is handshake material, whereas temp_k1/temp_k2 decrypt the entire session. Wrapping these locals in zeroize::Zeroizing<[u8; 32]>, or applying the same write_volatile loop already used in erase(), would close it.

Worth stating explicitly in the PR: stack zeroization in Rust is best-effort — LLVM may already have spilled copies elsewhere, and hmac_hash's internal buffers are outside the crate's control. But the crate has already committed to that best-effort model, and volatile-wiping ck/h while leaving the session keys behind is inconsistent.

2. The Cipher thread-safety comment is factually wrong

Severity: Low (but it is load-bearing rationale) · cipher_state.rs:152-162

Ensures that the Cipher type is not Sync, which prevents multiple threads from simultaneously accessing the same instance […] The Cipher struct is neither Sync nor Copy due to its cipher field.

This is not true. Compiling an external crate that asserts the bounds against this branch:

NoiseEngine/NoiseEncryptor/NoiseDecryptor are Send + Sync

ChaChaPoly1305 is a GenericArray<u8, U32> plus two PhantomData, so it is Send + Sync, and Cipher<ChaCha20Poly1305> inherits both. Nothing "ensures" the type is not Sync — there is no PhantomData<*const ()> and no negative impl anywhere.

There is no exploitable consequence: encrypt/decrypt take &mut self, so Sync alone cannot produce nonce reuse. But this comment is the crate's stated rationale for its thread-safety story, it sits directly above the Clone derive this PR removes, and an auditor reading it draws a false conclusion. Since the block is already being touched, it is worth correcting to what actually holds: nonce-unique use is enforced by &mut exclusivity and by the type no longer being Clone, not by the absence of Sync.

3. erase()'s doc claim is still inaccurate after the fix

Severity: Low · initiator.rs:393-398, same in responder.rs

The comment says erase "overwrites the stored keys, chaining key, handshake hash, and session ciphers with zeros." It never touches handshake_cipher — that key is wiped only transitively, by ChaChaPoly1305's own unconditional zeroize-on-drop. Also, unlike Cipher::erase_k, erase() zeroes k in place but leaves it as Some([0u8; 32]).

---

codec_sv2

4. A failed chunk encryption poisons the encoder for every later frame

Severity: Medium · encoder.rs:209-253

encrypt_frame sets noise_buffer's start offset before each chunk and resets it only after the loop completes:

while start < sv2.len() {
    // ...
    self.noise_buffer.danger_set_start(encrypted_len);
    encrypt(&mut self.noise_buffer)?;   // early return skips the reset below
    // ...
}
self.noise_buffer.danger_set_start(0);

When encrypt returns Err mid-loop, the ? skips danger_set_start(0) and the caller never reaches get_data_owned() — so the buffer is neither reset nor drained. NoiseEncoder is explicitly reusable (prop_noise_encoder_reusable), so the next encode / encode_transport on that encoder writes at a stale offset and emits a corrupt frame. On a Noise transport that means a ciphertext the peer can never open — a dead connection rather than a recoverable error.

The trigger is an AeadBuffer capacity failure inside chacha20poly1305's in-place encrypt, reachable under buffer-pool exhaustion.

This is pre-existing in encode(), but the refactor now exposes it through a second public entry point, and this is the transport-state hardening PR. Cheapest fix is to reset on the error path:

let result = /* chunk loop */;
self.noise_buffer.danger_set_start(0);
result

5. The split is opt-in, so the invariant is documented rather than enforced

Severity: Medium (design) · lib.rs:96-121

State::Transport, encode() and next_frame() all survive unchanged. Removing Clone closes the accidental duplication path, but nothing stops a downstream from holding one State behind an Arc<Mutex<_>> and driving both directions from it — the exact shape #2250/#2251 are about.

codec_sv2 is already carrying an unreleased major bump, so this is the one cheap moment to make the split mandatory: return (TransportEncryptState, TransportDecryptState) from step_1/step_2 and drop the State::Transport variant. In-repo the only callers of the combined path are the encoder prop-test helper, benches/noise_roundtrip.rs, and examples/encrypted.rs — so the migration is small. If that is out of scope here, it is worth a tracked follow-up rather than left implicit.

6. split_transport destroys the state on its error path

Severity: Low · lib.rs:342-355

split_transport(self) consumes self and returns Err(UnexpectedNoiseState) without handing the State back. Before this PR a caller could state.clone() ahead of a fallible consuming call; that escape hatch is now gone by design. Either return Err((Self, Error)) or add an is_transport() predicate so callers can check before committing. Low impact at the current call sites, but it is new public API in a major release.

7. Example and test helper still model the pre-split API

Severity: Low · examples/encrypted.rs:164-172, make_transport_state_pair in the encoder tests

Both still do State::Transport(c) => State::with_transport_mode(c) followed by encode(.., &mut state). The example is the surface downstream integrators copy from, so it should demonstrate split_transport.

8. The new round-trip test leaves the two paths it is meant to protect uncovered

Severity: Low · lib.rs:385-470

• TestMsg is 2 bytes, so every frame is single-chunk. The chunked (> SV2_FRAME_CHUNK_SIZE) loop in encrypt_frame / decode_noise_frame is reached only via the old encode() in the prop tests, never through encode_transport / next_transport_frame.
• round_trip builds a fresh StandardNoiseDecoder per frame. Real consumers keep one decoder per direction for the life of the connection, and that is where leftover-buffer / missing_noise_b bugs live.

The nonce-progression and direction-keying halves of the test are sound — this is about widening it, not replacing it.

9. Naming mismatch on the new public API

Severity: Nit

encode_transport vs next_transport_frame — inconsistent shape for a matched pair. Free to change now, permanent after the release.

[bit-aloo]

@GitGab19 comments were pretty good, specially the zeroize key material on stack. Though I have not taken up 5, 6 and 9, considering we gonna be ripping apart the codec runtime panic infra soon (this week). Rest are incorporated, thanks.

[GitGab19]

Follow-up review — 1b689312

Two new findings, plus one note on the claim in your comment.

---

1. The hmac_hash wipe is incomplete — the buffer grows and can abandon the padded key

Severity: Medium · noise-sv2/src/handshake.rs:149

let mut to_hash = Vec::with_capacity(64 + data.len());
to_hash.extend_from_slice(&ipad);
to_hash.extend_from_slice(data);          // phase 1: exactly fills capacity
let mut temp = Sha256Hash::hash(&to_hash).to_byte_array();

// Wiped before `clear`, while the length still spans the padded key.
zeroize_bytes(&mut to_hash);
to_hash.clear();
to_hash.extend_from_slice(&opad);         // phase 2: needs 64 + 32 = 96 bytes
to_hash.extend_from_slice(&temp);
// ...
zeroize_bytes(&mut to_hash);              // only reaches the *new* allocation

Phase 1 fills the allocation exactly. Phase 2 needs 96 bytes, so whenever data.len() < 32 the Vec must grow, and if the allocator cannot extend in place it copies to a new block and frees the old one — with opad still in it. The final zeroize_bytes then wipes the new allocation, not the abandoned one.

Replicating the exact buffer lifecycle (block=true occupies the following heap space so in-place growth is impossible, which is the normal case on a fragmented heap):

data.len()=0   block=true  cap 64->128 moved=true  opad_bytes_left_at_old_addr=56
data.len()=1   block=true  cap 65->130 moved=true  opad_bytes_left_at_old_addr=56
data.len()=33  block=true  cap 97->97  moved=false opad_bytes_left_at_old_addr=0

This is not an edge case. hmac_hash(&temp_key, &[0x1]) has data.len() == 1, so it grows on every hkdf_2 call, and hkdf_2(ck, &[]) — the derivation that produces the two transport session keys — has data.len() == 0.

Since opad[0..32] is key ^ 0x5c, the abandoned block yields 24 of the 32 key bytes; the first 8 are clobbered by glibc's free-list bookkeeping. So this is a partial recovery of the pseudorandom key / chaining key rather than a full break, and it is allocator-dependent — but it is exactly the material the rest of this PR sets out to protect.

The behavior itself predates the PR. What is new is the comment asserting a completeness the allocation lifecycle does not deliver. One line closes the whole question:

let mut to_hash = Vec::with_capacity(96 + data.len());

After that neither phase reallocates, and the existing final zeroize_bytes(&mut to_hash) genuinely covers everything.

2. 286a09df stripped comments that were doing real work

Severity: Low, but it undoes a fix · codec-sv2/src/lib.rs, encoder.rs, decoder.rs

The commit dropped 33 lines, two of which were /// documentation on public API:

• on split_transport — "The state is consumed whatever the outcome: a state that is not in transport mode is dropped rather than handed back, so callers that cannot guarantee the mode should check Self::is_transport first."
• on is_transport — "Self::split_transport consumes the state on its error path as well, so this is the way to check before committing to the call."

Those two lines were the fix. Without them there is a predicate no caller knows to reach for and an undocumented destructive error path, which leaves the API where it started minus the documentation.

Also removed, and worth keeping:

• The rationale for the encoder and decoder error-path cleanups. That code looks like a no-op to a reader who does not know danger_set_start persists across calls, which makes it a prime candidate for a future "dead code" deletion — the comments are what stop that.
• The note explaining use core::result::Result; in the encoder tests. Without it, that import reads as redundant and someone tidies it away, breaking the build.

If the goal was reducing comment density, the test doc-comments are the fair thing to cut. The public-API warnings and the invariant rationale are not.

[bit-aloo]

Extended the hmac buffer capacity and restored the agent deleted comments