Clear the symmetric key material we can reach when it goes out of scope

Adds the zeroize dependency and clears the retained ChaCha20-Poly1305 key on
CipherState, the chaining key and handshake hash on SymmetricState, and the
64-byte HKDF outputs in mix_key and split along with the two session keys
split derives. The key mix_key derives for the handshake cipher is cleared
too: it is passed by value into a Copy parameter, so the caller's copy
survives the call and is the same class of residue as the ones beside it.

Three fields are deliberately left out. The cached LessSafeKey holds its
material behind ring's API and cannot be cleared from here, and the nonce and
the has-key flag are not secret.

Also corrects the comments around this code, which were wrong in ways that
would have made the change harder to reason about later. ring's LessSafeKey
does implement Clone; UnboundKey is the one that does not, and two comments
said otherwise while a third said the opposite. The key bytes are retained for
Clone and cipher_clone, not for initialize_key, which builds from its own
argument. Caching the keyed cipher does not skip Poly1305 key derivation,
which is nonce-dependent and happens per message. And the construction it does
skip is a length check and a word conversion, not a constant-time check.

The HKDF state itself, the ephemeral private key, and the per-handshake DH
outputs are still uncleared; those are separate changes.

Cargo.lock

@@ -1126,6 +1126,7 @@ dependencies = [
  "tun",
  "windows-service",
  "wintun",
+ "zeroize",
 ]
 
 [[package]]
@@ -4288,6 +4289,20 @@ name = "zeroize"
 version = "1.9.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "e13c156562582aa81c60cb29407084cdb54c4164760106ab78e6c5b0858cf64e"
+dependencies = [
+ "zeroize_derive",
+]
+
+[[package]]
+name = "zeroize_derive"
+version = "1.5.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3c50655cbb0fe3fc43170059e702f1ce5e19b84cec58dc87b037a09935c2f328"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "syn 2.0.119",
+]
 
 [[package]]
 name = "zerotrie"

Cargo.toml

@@ -17,6 +17,7 @@ secp256k1 = { version = "0.30", features = ["rand", "global-context"] }
 sha2 = "0.10"
 hkdf = "0.12"
 ring = "0.17"
+zeroize = { version = "1.9", features = ["zeroize_derive"] }
 rand = "0.10.1"
 crossbeam-channel = "0.5"
 thiserror = "2.0"

src/node/encrypt_worker.rs

@@ -93,8 +93,10 @@ use tracing::{debug, trace, warn};
 /// inside the worker. That second alloc + ~1.5 KB memcpy per packet at
 /// line rate cost ~150 MB/sec of memory bandwidth on the hot worker.)
 pub(crate) struct FmpSendJob {
-    /// Cloned FMP send cipher. `LessSafeKey` is `Clone` (`ring::aead`)
+    /// Cloned FMP send cipher. `LessSafeKey` is `Clone` (`ring::aead`), but
-    /// — the clone is just a refcount bump on the inner key material.
+    /// the clone is not a refcount bump: `ring` stores the ChaCha20 key
+    /// inline as `[u32; 8]`, so cloning copies the key material outright and
+    /// leaves a second copy that nothing outside `ring` can clear.
     pub cipher: LessSafeKey,
     /// Pre-reserved monotonic counter (via `take_send_counter`).
     pub counter: u64,

src/noise/handshake.rs

@@ -9,6 +9,7 @@ use rand::Rng;
 use secp256k1::{Keypair, PublicKey, Secp256k1, SecretKey, ecdh::shared_secret_point};
 use sha2::{Digest, Sha256};
 use std::fmt;
+use zeroize::{Zeroize, ZeroizeOnDrop};
 
 /// Symmetric state during handshake.
 ///
@@ -17,7 +18,11 @@ use std::fmt;
 /// `Clone` exists for [`HandshakeState::try_read_xk_message_2`], which has to
 /// put the pre-read state back after a message that mixed material in before
 /// failing to authenticate.
-#[derive(Clone)]
+///
+/// `ck` and `h` are cleared on drop, including on the clone above once it
+/// goes out of scope. `cipher` is skipped because [`CipherState`] clears its
+/// own retained key.
+#[derive(Clone, Zeroize, ZeroizeOnDrop)]
 struct SymmetricState {
     /// Chaining key for key derivation.
     ck: [u8; 32],
@@ -30,6 +35,7 @@ struct SymmetricState {
     /// cookie binding) will silently not work until the AAD carries `h`.
     h: [u8; 32],
     /// Current cipher state for encrypting handshake payloads.
+    #[zeroize(skip)]
     cipher: CipherState,
 }
 
@@ -76,6 +82,9 @@ impl SymmetricState {
         let mut key = [0u8; 32];
         key.copy_from_slice(&output[32..64]);
         self.cipher.initialize_key(key);
+        key.zeroize();
+
+        output.zeroize();
     }
 
     /// Encrypt and mix into hash.
@@ -104,7 +113,13 @@ impl SymmetricState {
         k1.copy_from_slice(&output[..32]);
         k2.copy_from_slice(&output[32..64]);
 
-        (CipherState::new(k1), CipherState::new(k2))
+        let ciphers = (CipherState::new(k1), CipherState::new(k2));
+
+        output.zeroize();
+        k1.zeroize();
+        k2.zeroize();
+
+        ciphers
     }
 
     /// Get the handshake hash.

src/noise/mod.rs

@@ -42,6 +42,7 @@ mod session;
 use ring::aead::{Aad, CHACHA20_POLY1305, LessSafeKey, Nonce, UnboundKey};
 use std::fmt;
 use thiserror::Error;
+use zeroize::{Zeroize, ZeroizeOnDrop};
 
 pub use handshake::HandshakeState;
 pub use replay::ReplayWindow;
@@ -181,21 +182,32 @@ impl fmt::Display for HandshakeProgress {
 /// AEAD is `ring`'s ChaCha20-Poly1305 (BoringSSL backend), which dispatches
 /// to NEON on aarch64 and AVX2/AVX-512 on x86_64. The 32-byte key is
 /// retained alongside a cached `LessSafeKey` so the per-packet AEAD skips
-/// the keyed-cipher construction (key copy + Poly1305 key derivation).
+/// rebuilding the keyed cipher. (It does not skip Poly1305 key derivation,
-/// `LessSafeKey` itself doesn't implement `Clone` (deliberate, for safety),
+/// which is nonce-dependent and happens per message inside seal and open.)
-/// so `CipherState`'s manual `Clone` impl rebuilds the keyed AEAD from the
+/// `CipherState`'s manual `Clone` impl rebuilds the keyed AEAD from those
-/// retained key bytes — cheap for ChaCha20-Poly1305 since the construction
+/// retained bytes, and so does `cipher_clone` for each off-task AEAD worker;
-/// is essentially a key copy plus a constant-time check.
+/// those two are why the bytes are kept. The rebuild is cheap for
+/// ChaCha20-Poly1305: a length check and a conversion of the 32 key bytes
+/// into little-endian words.
+///
+/// Because the key bytes are retained, every clone leaves a second copy in
+/// memory; each copy is cleared when it goes out of scope. Only `key` is
+/// zeroized. `cipher` is skipped because `LessSafeKey`
+/// holds its material behind `ring`'s own API and cannot be cleared from
+/// here; `nonce` and `has_key` are skipped because they are not secret.
+#[derive(Zeroize, ZeroizeOnDrop)]
 pub struct CipherState {
-    /// Encryption key (32 bytes). Retained so we can rebuild the keyed
+    /// Encryption key (32 bytes). Retained because `Clone` and
-    /// AEAD on `Clone` and on `initialize_key` (ring's `UnboundKey` /
+    /// `cipher_clone` rebuild the keyed AEAD from these bytes.
-    /// `LessSafeKey` do not implement `Clone`).
     key: [u8; 32],
     /// Cached keyed AEAD, valid iff `has_key`. None for an un-keyed state.
+    #[zeroize(skip)]
     cipher: Option<LessSafeKey>,
     /// Nonce counter (8 bytes used, 4 bytes zero prefix).
+    #[zeroize(skip)]
     pub(super) nonce: u64,
     /// Whether this cipher has a valid key.
+    #[zeroize(skip)]
     has_key: bool,
 }
 
@@ -395,6 +407,13 @@ impl CipherState {
         self.has_key
     }
 
+    /// Copy out the retained key bytes, so a test can observe zeroization
+    /// without reading freed memory.
+    #[cfg(test)]
+    fn key_bytes(&self) -> [u8; 32] {
+        self.key
+    }
+
     /// Clone the underlying keyed AEAD, for off-task AEAD workers.
     ///
     /// Returns `None` if no key. The cloned `LessSafeKey` pairs with

src/noise/tests.rs

@@ -201,6 +201,26 @@ fn test_cipher_state_nonce_sequence() {
     assert_eq!(cipher.nonce(), 2);
 }
 
+#[test]
+fn cipher_state_drop_impl_clears_the_retained_key() {
+    // Reading the bytes back out of a dropped `CipherState` would mean
+    // reading freed memory, so the drop behaviour is asserted through two
+    // observable proxies instead: that `CipherState` implements
+    // `ZeroizeOnDrop`, and that an explicit `zeroize()` leaves `key`
+    // all-zero while `has_key` is untouched.
+    fn assert_zeroize_on_drop<T: zeroize::ZeroizeOnDrop>() {}
+    assert_zeroize_on_drop::<CipherState>();
+
+    let mut cipher = CipherState::new([7u8; 32]);
+    assert_eq!(cipher.key_bytes(), [7u8; 32]);
+    assert!(cipher.has_key());
+
+    cipher.zeroize();
+
+    assert_eq!(cipher.key_bytes(), [0u8; 32]);
+    assert!(cipher.has_key());
+}
+
 #[test]
 fn test_session_remote_static() {
     let keypair1 = generate_keypair();