Clear the connection's key slots, erase the handshake keypair in place, and document what is now cleared

Option::take moves a value out but writes only the None marker, so the
old bytes stay in the slot. Completing a handshake left the whole
handshake state, the node's long-term private key and the ephemeral key
included, in the connection's handshake slot, which on a peer machine
is heap memory that outlives the handshake, and take_session left both
traffic keys of the session in the session slot.

Add clear_slot in the noise module: it drops whatever the slot holds,
overwrites every byte of the slot with zeros through zeroize's volatile
flat-type erase, and leaves None. complete_handshake takes and unwraps
the handshake first and then clears the slot, so a release build copies
the handshake once, as before, and then zeroes the slot; clearing inside
the take had cost a second stack copy that nothing cleared.
take_cleared, built on clear_slot, takes the value out and clears the
slot, and take_session uses it, since there the session goes straight
to the caller and no extra copy appears.

start_handshake and receive_handshake_init hold the node's long-term
keypair across early returns, so a guard erases it on every exit. The
guard owned a copy: building it copied the keypair into a temporary and
then moved it into the guard's binding, and only the binding was erased.
On the early-return paths nothing reused the temporary's stack slot, so
a copy of the long-term private key stayed in the frame. The guard now
borrows the caller's binding and erases it where it lies when dropped,
so making or moving it copies no key bytes.

Tests cover what safe code can observe: the value comes out intact and
is dropped exactly once, the slot is None and reusable, a type whose
all-zero bytes would read as Some still ends as None, a handshake and
session taken this way still interoperate, and a guard held across an
early and a normal return erases the caller's own keypair on both. The
clearing itself is shown by a release build's disassembly and a scan of
process memory, not by a unit test.

The security reference now says the SHA-256 and HMAC states clear
themselves on drop and that a completed handshake and a session taken for
a rekey leave their slot cleared, and names what is still left: the copies
stack moves leave, the session keys or unfinished handshake that stay in a
control machine's heap memory when the whole connection state is moved off
it on promotion or reaping, and the copies of the private key that loading
the identity from a secret string leaves in that constructor's frame, now a
known limit. It also no longer says each erase is a volatile write followed
by a compiler fence: only secp256k1's erase uses a fence, zeroize's follows
the write with an empty asm! optimisation barrier, and in both it is the
volatile write that keeps the store.

docs/reference/security.md

@@ -96,17 +96,23 @@ Noise handshake holds, the chaining key and handshake hash, the
 per-message Diffie-Hellman results, the key-derivation outputs and the
 two session keys derived from them, the retained key on each cipher
 state, the bech32 and hex encodings of a secret, and the configuration
-text that carries `node.identity.nsec`. A completed session keeps its
+text that carries `node.identity.nsec`. The SHA-256 state that hashes
+each Diffie-Hellman result and the HMAC states inside HKDF clear
+themselves on drop, through the opt-in `zeroize` features of `sha2`
+and `hmac`, which the daemon turns on. A completed session keeps its
 own copy of the handshake hash and does not clear it, on purpose:
 nothing derives a key from it, and the session hands it out to any
 caller.
 
-Each erase is a volatile write followed by a compiler fence, so the
-optimiser cannot remove it as a dead store. This was checked against
-generated code rather than assumed: in an x86_64 release build (Rust
-1.94.1, `secp256k1` 0.30.0, `zeroize` 1.9.0), every erase in the Noise
-handshake and identity code that is linked into the daemon is present
-as stores in the machine code.
+Each erase is a volatile write, and the optimiser does not remove a
+volatile write as a dead store. `secp256k1`'s erase follows the write
+with a compiler fence, and `zeroize`'s with an optimisation barrier (an
+empty `asm!` block on x86_64); those limit how the compiler may reorder
+code around the write, but it is the volatile write that keeps the
+store. This was checked against generated code rather than assumed: in
+an x86_64 release build (Rust 1.94.1, `secp256k1` 0.30.0, `zeroize`
+1.9.0), every erase in the Noise handshake and identity code that is
+linked into the daemon is present as stores in the machine code.
 
 An erase reaches only the place it is called on. What it does not
 reach:
@@ -117,22 +123,25 @@ reach:
   dropped. Each of those moves leaves behind, in a stack frame that is
   no longer in use, a copy of the node's long-term private key and,
   once the handshake has started, of its ephemeral key and chaining
-  key. When a completed handshake is taken out of the connection slot
-  that held it, the slot keeps the handshake's full contents in heap
-  memory until that memory is reused. The session that comes out of
-  the handshake is left the same way: it is moved out of the handshake
-  and into the connection's session slot, and taking it out of that
-  slot leaves both of its traffic keys behind in heap memory.
+  key. Two moves out of the slots on a connection's control machine
+  are cleared: when a completed handshake leaves the slot that held it,
+  and when a session is taken out of its slot for a rekey, the slot is
+  overwritten as the value leaves. Other moves are not. When a
+  connection is promoted to an active peer, or reaped as stale, its
+  whole handshake state is moved off its control machine, which leaves
+  the session's two traffic keys, or an unfinished handshake's private
+  keys, in the heap memory the machine occupies.
+- **Loading the identity from a secret string.** Building the node's
+  identity from its key file or from `node.identity.nsec` leaves
+  copies of the private key, among them a whole intermediate identity,
+  in that constructor's stack frame, and they are not cleared.
 - **Registers and spilled temporaries**, which no code in the daemon
   can name.
-- **Library state.** The SHA-256 state that hashes each
-  Diffie-Hellman result and the HMAC states inside HKDF are not
-  cleared: `sha2` and `hmac` offer an opt-in `zeroize` feature that
-  clears them on drop, and the daemon does not enable it. The cipher
-  keys cached inside `ring`'s `LessSafeKey` have no clearing route. The
-  daemon cannot clear the internal temporaries of the `libsecp256k1` C
-  library either; the library clears some of its own, such as the
-  nonce and secret scalar used in signing, on a best-effort basis.
+- **Library state.** The cipher keys cached inside `ring`'s
+  `LessSafeKey` have no clearing route. The daemon cannot clear the
+  internal temporaries of the `libsecp256k1` C library either; the
+  library clears some of its own, such as the nonce and secret scalar
+  used in signing, on a best-effort basis.
 
 Clearing therefore shortens how long secret material stays in memory
 and removes it from the places the daemon's own code keeps it; it does

src/identity/local.rs

@@ -20,7 +20,9 @@ use super::{FipsAddress, IdentityError, NodeAddr, sha256};
 /// write, so the optimiser keeps it, but it reaches only the place it is
 /// called on: a copy made before it runs is not cleared, and that includes
 /// the bytes a move leaves behind wherever the value used to be, such as
-/// the frame of the constructor that built it.
+/// the frame of the constructor that built it. `from_secret_str` is a known
+/// case: it leaves copies of the private key, a whole intermediate
+/// `Identity` among them, in its own frame.
 #[derive(Clone)]
 pub struct Identity {
     keypair: Keypair,
@@ -143,36 +145,36 @@ impl Drop for Identity {
     }
 }
 
-/// A `Keypair` copy that is erased when it goes out of scope.
+/// Erases a `Keypair` in place when it goes out of scope.
 ///
 /// `Keypair` is `Copy` and so cannot clear itself on drop. A frame that holds
 /// a copy of the node's long-term private key across several exit paths —
 /// early error returns, `?`, a normal return — would otherwise need an erase
-/// written at each one, and a missed path is invisible. Holding the copy here
-/// instead makes the clearing structural.
+/// written at each one, and a missed path is invisible. Guarding the copy
+/// makes the clearing structural.
 ///
-/// This clears the copy this guard owns, not every copy that ever existed.
-/// The erase is a volatile write, so the optimiser keeps it, but it reaches
-/// only the guard's final location: moving the guard, including out of
-/// [`ErasingKeypair::take`], leaves the bytes behind where it used to be.
-pub(crate) struct ErasingKeypair(Keypair);
+/// The guard borrows the keypair rather than holding one, so making it and
+/// moving it copy no key bytes: the erase lands on the caller's own binding,
+/// wherever that lies, and there is no second copy for an early return to
+/// leave behind. It clears that one binding, not every copy that ever
+/// existed. The erase is a volatile write, so the optimiser keeps it, but a
+/// copy made before the guard existed, or taken out through
+/// [`ErasingKeypair::get`], is not cleared by it.
+pub(crate) struct ErasingKeypair<'a>(&'a mut Keypair);
 
-impl ErasingKeypair {
-    /// Take a copy of `source` into the guard and erase `source` in place, so
-    /// the caller's own binding does not outlive the move.
-    pub(crate) fn take(source: &mut Keypair) -> Self {
-        let guarded = Self(*source);
-        source.non_secure_erase();
-        guarded
+impl<'a> ErasingKeypair<'a> {
+    /// Guard `source` so it is erased where it lies when the guard drops.
+    pub(crate) fn new(source: &'a mut Keypair) -> Self {
+        Self(source)
     }
 
     /// Borrow the guarded keypair.
     pub(crate) fn get(&self) -> &Keypair {
-        &self.0
+        self.0
     }
 }
 
-impl Drop for ErasingKeypair {
+impl Drop for ErasingKeypair<'_> {
     fn drop(&mut self) {
         self.0.non_secure_erase();
     }

src/identity/tests.rs

@@ -634,3 +634,31 @@ fn test_identity_debug() {
     assert!(!debug.contains("keypair"));
     assert!(debug.contains(".."));
 }
+
+/// Hold a guard over `keypair` and return early or normally, the way the
+/// handshake entry points do.
+fn guarded_secret(keypair: &mut Keypair, fail: bool) -> Result<[u8; 32], ()> {
+    let guard = ErasingKeypair::new(keypair);
+    if fail {
+        return Err(());
+    }
+    Ok(guard.get().secret_bytes())
+}
+
+#[test]
+fn test_erasing_keypair_erases_the_callers_binding_in_place_on_every_exit() {
+    let identity = Identity::generate();
+    let original = identity.keypair().secret_bytes();
+    // `non_secure_erase` overwrites a keypair with a fixed dummy whose
+    // secret is 32 bytes of 0x01.
+    let erased = [1u8; 32];
+    assert_ne!(original, erased);
+
+    let mut keypair = identity.keypair();
+    assert_eq!(guarded_secret(&mut keypair, false), Ok(original));
+    assert_eq!(keypair.secret_bytes(), erased);
+
+    let mut keypair = identity.keypair();
+    assert_eq!(guarded_secret(&mut keypair, true), Err(()));
+    assert_eq!(keypair.secret_bytes(), erased);
+}

src/noise/handshake.rs

@@ -344,10 +344,10 @@ impl HandshakeState {
             SecretKey::from_slice(&secret_bytes).expect("32 random bytes is valid secret key");
         self.ephemeral_keypair = Some(Keypair::from_secret_key(&self.secp, &secret_key));
         secret_bytes.zeroize();
-        // The erase is called non-secure because the private key may sit in
-        // further copies this function cannot name. It still clears the copy
-        // this frame owns; the keypair the key was just stored in is cleared
-        // by `Drop for HandshakeState`.
+        // This clears the copy this frame owns, with a volatile write the
+        // optimiser keeps; a copy made before it, such as one a call above
+        // left in a register or a stack slot, is not reached. The keypair the
+        // key was just stored in is cleared by `Drop for HandshakeState`.
         secret_key.non_secure_erase();
     }
 
@@ -364,9 +364,9 @@ impl HandshakeState {
     /// caller below binds the value `Keypair::secret_key` hands back and
     /// erases that binding once the DH is done, because the returned
     /// `SecretKey` is a whole private key rather than a handle to one. Those
-    /// erases clear the copies this crate owns; `secp256k1` names its erase
-    /// non-secure because the compiler may hold further copies that no code
-    /// here can name.
+    /// erases clear the copies this crate owns. Each is a volatile write the
+    /// optimiser keeps, but a copy the compiler made before it runs, in a
+    /// register or a stack slot, is not reached.
     fn ecdh(&self, our_secret: &SecretKey, their_public: &PublicKey) -> [u8; 32] {
         // Get raw (x, y) coordinates (64 bytes) without any hashing
         let mut point = shared_secret_point(their_public, our_secret);
@@ -1102,9 +1102,11 @@ impl Drop for HandshakeState {
     /// Each erase is a volatile write, so the optimiser keeps it, but a
     /// `HandshakeState` is built on the stack and moved several times before
     /// it is dropped, and every move leaves the old bytes, both private keys
-    /// included, where the value used to be. Taking it out of an `Option`
-    /// with `take()` leaves its full contents in the slot it came from, which
-    /// for a connection's handshake slot is heap memory.
+    /// included, where the value used to be. A plain `take()` out of an
+    /// `Option` leaves its full contents in the slot. When a connection's
+    /// handshake completes, the connection's heap slot is cleared with
+    /// `clear_slot` once the handshake has left it; a move of the whole
+    /// connection state out of its machine does not clear anything.
     fn drop(&mut self) {
         self.static_keypair.non_secure_erase();
         if let Some(ephemeral) = self.ephemeral_keypair.as_mut() {

src/noise/mod.rs

@@ -489,5 +489,49 @@ pub(crate) fn open(
     Ok(buf)
 }
 
+/// Move the value out of `slot`, leaving `None`, and clear the bytes the
+/// value occupied.
+///
+/// `Option::take` moves the value out but writes only the `None` marker, so
+/// the old value's bytes stay where they were. For a slot holding a
+/// handshake or a session that is a full copy of its keys, which nothing
+/// will clear because nothing owns it any more. Here, once the value has
+/// moved out, the slot is cleared with [`clear_slot`].
+///
+/// This clears the slot only. The moved value is the caller's to clear, and
+/// any copy made on the way out, such as in a register or a stack slot, is
+/// out of reach as it is for every other move. A caller that unwraps the
+/// result straight away should use `take().expect(..)` followed by
+/// [`clear_slot`] instead: unwrapping after the slot has been cleared makes
+/// the optimiser keep a second copy of the value on the stack, because it
+/// can no longer copy the value straight from the slot to where it ends up.
+pub(crate) fn take_cleared<T>(slot: &mut Option<T>) -> Option<T> {
+    let value = slot.take();
+    clear_slot(slot);
+    value
+}
+
+/// Drop whatever `slot` holds, overwrite every byte of it with zeros, and
+/// leave it `None`.
+///
+/// The zeros are volatile writes, which the optimiser keeps. Meant for a
+/// slot whose value has just been moved out, so the bytes the move left
+/// behind are cleared.
+pub(crate) fn clear_slot<T>(slot: &mut Option<T>) {
+    *slot = None;
+    let raw: *mut Option<T> = slot;
+    // SAFETY: `raw` comes from a live `&mut`, so it is valid and aligned for
+    // `size_of::<Option<T>>()` bytes. Seen as `MaybeUninit`, those bytes have
+    // no drop glue, own nothing and may be all zero, which is what
+    // `zeroize_flat_type` asks of its target. The slot holds `None` after the
+    // assignment above, so zeroing it discards nothing, and writing `None`
+    // (which does not drop the zeroed bytes) leaves the slot valid before
+    // anything reads it again.
+    unsafe {
+        zeroize::zeroize_flat_type(raw.cast::<std::mem::MaybeUninit<Option<T>>>());
+        raw.write(None);
+    }
+}
+
 #[cfg(test)]
 mod tests;

src/noise/session.rs

@@ -11,8 +11,10 @@ use std::fmt;
 /// The two traffic keys are cleared when their `CipherState`s are dropped,
 /// which clears only the copy being dropped. A session is moved out of
 /// `HandshakeState::into_session` and into a connection's session slot, and
-/// every move leaves both keys where the value used to be; taking it out of
-/// that slot with `take()` leaves them in the slot, which is heap memory.
+/// every move leaves both keys where the value used to be. Taking it out of
+/// that slot through `take_cleared`, as the machine's `take_session` does,
+/// clears the slot; a plain `take()`, or a move of the whole connection
+/// state out of its machine, leaves both keys there.
 pub struct NoiseSession {
     /// Our role in the original handshake.
     role: HandshakeRole,

src/noise/tests.rs

@@ -927,3 +927,122 @@ fn test_sha256_states_used_by_hashing_and_hkdf_are_cleared_on_drop() {
     clears_on_drop::<sha2::Sha256>();
     clears_on_drop::<<sha2::Sha256 as hmac::EagerHash>::Core>();
 }
+
+/// A value that counts its drops, so a test can tell a value dropped once
+/// from one dropped twice or leaked.
+struct DropCounter<'a> {
+    bytes: [u8; 48],
+    heap: Vec<u8>,
+    drops: &'a std::cell::Cell<usize>,
+}
+
+impl Drop for DropCounter<'_> {
+    fn drop(&mut self) {
+        self.drops.set(self.drops.get() + 1);
+    }
+}
+
+#[test]
+fn test_take_cleared_moves_the_value_out_intact_and_leaves_the_slot_none() {
+    let drops = std::cell::Cell::new(0);
+    let mut slot = Some(DropCounter {
+        bytes: [0xa5; 48],
+        heap: vec![7u8; 100],
+        drops: &drops,
+    });
+
+    let taken = take_cleared(&mut slot).expect("the slot held a value");
+
+    assert!(slot.is_none());
+    assert_eq!(taken.bytes, [0xa5; 48]);
+    assert_eq!(taken.heap, vec![7u8; 100]);
+    assert_eq!(drops.get(), 0, "clearing the slot must not drop the value");
+    drop(taken);
+    assert_eq!(drops.get(), 1);
+}
+
+#[test]
+fn test_take_cleared_slot_is_reusable_and_an_empty_slot_stays_empty() {
+    let drops = std::cell::Cell::new(0);
+    let mut slot: Option<DropCounter<'_>> = None;
+    assert!(take_cleared(&mut slot).is_none());
+    assert!(slot.is_none());
+
+    slot = Some(DropCounter {
+        bytes: [1; 48],
+        heap: vec![2; 3],
+        drops: &drops,
+    });
+    let first = take_cleared(&mut slot).expect("first value");
+    slot = Some(DropCounter {
+        bytes: [3; 48],
+        heap: vec![4; 5],
+        drops: &drops,
+    });
+    let second = take_cleared(&mut slot).expect("second value");
+    assert_eq!((first.bytes[0], second.bytes[0]), (1, 3));
+    assert!(slot.is_none());
+    drop((first, second));
+    assert_eq!(drops.get(), 2);
+}
+
+/// For `Option<bool>`, all-zero bytes read as `Some(false)`, not `None`.
+/// The slot must still come out as `None`, so zeroing alone is not enough.
+#[test]
+fn test_take_cleared_leaves_none_where_zero_bytes_would_read_as_some() {
+    let mut slot = Some(true);
+    assert_eq!(take_cleared(&mut slot), Some(true));
+    assert_eq!(slot, None);
+
+    let mut slot = Some((true, [9u8; 32]));
+    assert_eq!(take_cleared(&mut slot), Some((true, [9u8; 32])));
+    assert_eq!(slot, None);
+}
+
+#[test]
+fn test_take_cleared_session_still_decrypts_what_its_peer_encrypts() {
+    let initiator_keypair = generate_keypair();
+    let responder_keypair = generate_keypair();
+    let mut initiator =
+        HandshakeState::new_initiator(initiator_keypair, responder_keypair.public_key());
+    let mut responder = HandshakeState::new_responder(responder_keypair);
+    initiator.set_local_epoch(generate_epoch());
+    responder.set_local_epoch(generate_epoch());
+    let msg1 = initiator.write_message_1().unwrap();
+    responder.read_message_1(&msg1).unwrap();
+    let msg2 = responder.write_message_2().unwrap();
+
+    let mut handshake_slot = Some(initiator);
+    let mut initiator = take_cleared(&mut handshake_slot).expect("handshake in slot");
+    assert!(handshake_slot.is_none());
+    initiator.read_message_2(&msg2).unwrap();
+
+    let mut session_slot = Some(initiator.into_session().unwrap());
+    let mut sender = take_cleared(&mut session_slot).expect("session in slot");
+    assert!(session_slot.is_none());
+    let mut receiver = responder.into_session().unwrap();
+    let ciphertext = sender.encrypt(b"after the move").unwrap();
+    assert_eq!(receiver.decrypt(&ciphertext).unwrap(), b"after the move");
+}
+
+#[test]
+fn test_clear_slot_drops_a_present_value_once_and_leaves_none() {
+    let drops = std::cell::Cell::new(0);
+    let mut slot = Some(DropCounter {
+        bytes: [5; 48],
+        heap: vec![6; 7],
+        drops: &drops,
+    });
+    clear_slot(&mut slot);
+    assert!(slot.is_none());
+    assert_eq!(drops.get(), 1);
+
+    clear_slot(&mut slot);
+    assert!(slot.is_none());
+    assert_eq!(drops.get(), 1, "an empty slot has nothing to drop");
+
+    let mut flag = Some(true);
+    flag.take();
+    clear_slot(&mut flag);
+    assert_eq!(flag, None);
+}

src/peer/machine.rs

@@ -622,8 +622,9 @@ impl PeerMachine {
     ) -> Result<Vec<u8>, NoiseError> {
         // The parameter is this frame's own copy of the node's long-term
         // private key, and the state checks below return before it is used.
-        // The guard clears it on every exit path.
-        let our_keypair = ErasingKeypair::take(&mut our_keypair);
+        // The guard clears it in place on every exit path, and makes no copy
+        // of its own for an early return to leave behind.
+        let our_keypair = ErasingKeypair::new(&mut our_keypair);
 
         let msg1 = {
             let direction = self.conn.direction();
@@ -668,8 +669,9 @@ impl PeerMachine {
         current_time_ms: u64,
     ) -> Result<Vec<u8>, NoiseError> {
         // Same as `start_handshake`: the parameter copy outlives two early
-        // returns, so the guard owns it rather than an erase per exit path.
-        let our_keypair = ErasingKeypair::take(&mut our_keypair);
+        // returns, so the guard clears it in place rather than an erase per
+        // exit path.
+        let our_keypair = ErasingKeypair::new(&mut our_keypair);
 
         let (msg2, learned_identity, remote_epoch) = {
             let direction = self.conn.direction();
@@ -738,10 +740,15 @@ impl PeerMachine {
                 });
             }
 
+            // The slot is heap memory that outlives this call; clearing it once
+            // the handshake has left keeps both private keys from staying
+            // there. Unwrapping before clearing lets the handshake move
+            // straight from the slot to `hs`, with no second stack copy.
             let mut hs = leg
                 .noise_handshake
                 .take()
                 .expect("noise handshake must exist in SentMsg1 state");
+            noise::clear_slot(&mut leg.noise_handshake);
 
             hs.read_message_2(message)?;
 
@@ -766,7 +773,11 @@ impl PeerMachine {
     pub(crate) fn take_session(&mut self) -> Option<NoiseSession> {
         // The session exists iff the handshake reached `Complete`, so taking it
         // unconditionally is byte-equivalent to the old `== Complete` gate.
-        self.leg.as_mut().and_then(|leg| leg.noise_session.take())
+        // The slot is cleared as the session leaves, so its two traffic keys
+        // do not stay behind in the machine.
+        self.leg
+            .as_mut()
+            .and_then(|leg| noise::take_cleared(&mut leg.noise_session))
     }
 
     /// Check if we have a completed session ready to take.