diff --git a/docs/reference/security.md b/docs/reference/security.md index 0d5d7f8d..71a2b13f 100644 --- a/docs/reference/security.md +++ b/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 diff --git a/src/identity/local.rs b/src/identity/local.rs index 5d1c3a31..e8dad691 100644 --- a/src/identity/local.rs +++ b/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(); } diff --git a/src/identity/tests.rs b/src/identity/tests.rs index 546c7ff0..df066380 100644 --- a/src/identity/tests.rs +++ b/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); +} diff --git a/src/noise/handshake.rs b/src/noise/handshake.rs index 64a1ae97..58ccc9da 100644 --- a/src/noise/handshake.rs +++ b/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() { diff --git a/src/noise/mod.rs b/src/noise/mod.rs index 85366e4f..653eb89b 100644 --- a/src/noise/mod.rs +++ b/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(slot: &mut Option) -> Option { + 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(slot: &mut Option) { + *slot = None; + let raw: *mut Option = slot; + // SAFETY: `raw` comes from a live `&mut`, so it is valid and aligned for + // `size_of::>()` 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::>>()); + raw.write(None); + } +} + #[cfg(test)] mod tests; diff --git a/src/noise/session.rs b/src/noise/session.rs index 49b9472e..c2436451 100644 --- a/src/noise/session.rs +++ b/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, diff --git a/src/noise/tests.rs b/src/noise/tests.rs index d8adec46..952772b2 100644 --- a/src/noise/tests.rs +++ b/src/noise/tests.rs @@ -927,3 +927,122 @@ fn test_sha256_states_used_by_hashing_and_hkdf_are_cleared_on_drop() { clears_on_drop::(); clears_on_drop::<::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, + drops: &'a std::cell::Cell, +} + +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> = 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`, 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); +} diff --git a/src/peer/machine.rs b/src/peer/machine.rs index a9b28ec9..90ca055d 100644 --- a/src/peer/machine.rs +++ b/src/peer/machine.rs @@ -622,8 +622,9 @@ impl PeerMachine { ) -> Result, 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, 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 { // 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.