diff --git a/src/bin/fips.rs b/src/bin/fips.rs index b43f5282..afc09ca2 100644 --- a/src/bin/fips.rs +++ b/src/bin/fips.rs @@ -10,6 +10,7 @@ use fips::{Config, Node}; use std::path::PathBuf; use tracing::{debug, error, info, warn}; use tracing_subscriber::{EnvFilter, fmt}; +use zeroize::Zeroize; /// FIPS mesh network daemon #[derive(Parser, Debug)] @@ -126,7 +127,7 @@ async fn run_daemon( fips::node::warn_on_legacy_config_paths(); // Identity provisioning: config nsec > key file > generate ephemeral - let resolved = match resolve_identity(&config, &loaded_paths) { + let mut resolved = match resolve_identity(&config, &loaded_paths) { Ok(r) => r, Err(e) => { error!("Failed to resolve identity: {}", e); @@ -146,7 +147,15 @@ async fn run_daemon( // Create node with resolved identity let mut config = config; - config.node.identity.nsec = Some(resolved.nsec); + // Take the nsec rather than move it: `ResolvedIdentity` clears its copy + // on drop, so it cannot be left partially moved. Clear whatever the field + // already held first — assigning over it drops the old `String` in place, + // which does not run `Drop for IdentityConfig`, so a key that came from + // the config file would be freed uncleared. + if let Some(mut old) = config.node.identity.nsec.take() { + old.zeroize(); + } + config.node.identity.nsec = Some(std::mem::take(&mut resolved.nsec)); debug!("Creating node"); let mut node = match Node::new(config) { Ok(node) => node, diff --git a/src/bin/fipsctl.rs b/src/bin/fipsctl.rs index 15543044..5cf7701e 100644 --- a/src/bin/fipsctl.rs +++ b/src/bin/fipsctl.rs @@ -15,6 +15,7 @@ use std::io::{BufRead, BufReader, Write}; use std::net::{Ipv6Addr, SocketAddrV6}; use std::path::{Path, PathBuf}; use std::time::Duration; +use zeroize::Zeroizing; /// FIPS control client #[derive(Parser, Debug)] @@ -382,11 +383,18 @@ fn main() { // Commands that don't require a running daemon if let Commands::Keygen { dir, force, stdout } = &cli.command { let identity = Identity::generate(); - let nsec = encode_nsec(&identity.keypair().secret_key()); + // `keypair()` and `secret_key()` each hand back a whole private key + // rather than a handle, so both temporaries are bound and erased. The + // nsec is that same key in another encoding, so it is guarded too. + let mut our_keypair = identity.keypair(); + let mut secret_key = our_keypair.secret_key(); + let nsec = Zeroizing::new(encode_nsec(&secret_key)); + secret_key.non_secure_erase(); + our_keypair.non_secure_erase(); let npub = identity.npub(); if *stdout { - println!("{nsec}"); + println!("{}", nsec.as_str()); println!("{npub}"); return; } diff --git a/src/config/mod.rs b/src/config/mod.rs index 10cfb0c4..bab1ec9a 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -32,6 +32,7 @@ use crate::{Identity, IdentityError}; use serde::{Deserialize, Serialize}; use std::path::{Path, PathBuf}; use thiserror::Error; +use zeroize::{Zeroize, Zeroizing}; #[cfg(target_os = "linux")] pub use gateway::{ConntrackConfig, GatewayConfig, GatewayDnsConfig, PortForward, Proto}; @@ -217,11 +218,16 @@ pub fn default_gateway_path() -> PathBuf { } /// Read a bare bech32 nsec from a key file. +/// +/// The file contents are the private key, and trimming copies it into a +/// second string, so the read buffer is cleared on every exit path rather +/// than dropped as it stands. The returned nsec is the caller's. pub fn read_key_file(path: &Path) -> Result { let contents = std::fs::read_to_string(path).map_err(|e| ConfigError::ReadFile { path: path.to_path_buf(), source: e, })?; + let contents = Zeroizing::new(contents); let nsec = contents.trim().to_string(); if nsec.is_empty() { return Err(ConfigError::EmptyKeyFile { @@ -442,7 +448,9 @@ pub fn resolve_identity( if config.node.identity.persistent { // Persistent mode: load existing key file or generate-and-persist if key_path.exists() { - let nsec = read_key_file(&key_path)?; + // Held in a guard, not a bare `String`: if the parse below fails, + // the `?` returns and a bare local would be freed uncleared. + let nsec = Zeroizing::new(read_key_file(&key_path)?); let identity = Identity::from_secret_str(&nsec)?; warn_unmanaged_key_file(&key_path); if let Err(e) = write_pub_file(&pub_path, &identity.npub()) { @@ -453,7 +461,7 @@ pub fn resolve_identity( ); } return Ok(ResolvedIdentity { - nsec, + nsec: nsec.to_string(), source: IdentitySource::KeyFile(key_path), }); } @@ -467,7 +475,8 @@ pub fn resolve_identity( Path::new(SYSTEM_CONFIG_DIR), Path::new(LEGACY_SYSTEM_CONFIG_DIR), ) { - let nsec = read_key_file(&legacy)?; + // Guarded for the same reason as the current-path read above. + let nsec = Zeroizing::new(read_key_file(&legacy)?); let identity = Identity::from_secret_str(&nsec)?; tracing::warn!( legacy = %legacy.display(), @@ -484,14 +493,20 @@ pub fn resolve_identity( ); } return Ok(ResolvedIdentity { - nsec, + nsec: nsec.to_string(), source: IdentitySource::KeyFile(legacy), }); } // No key file anywhere — generate and persist let identity = Identity::generate(); - let nsec = encode_nsec(&identity.keypair().secret_key()); + // `keypair()` and `secret_key()` each hand back a whole private key + // rather than a handle, so both temporaries are bound and erased. + let mut our_keypair = identity.keypair(); + let mut secret_key = our_keypair.secret_key(); + let nsec = encode_nsec(&secret_key); + secret_key.non_secure_erase(); + our_keypair.non_secure_erase(); let npub = identity.npub(); if let Some(parent) = key_path.parent() { @@ -529,7 +544,13 @@ pub fn resolve_identity( // Ephemeral mode (default): fresh keypair every start, write key files // for operator visibility let identity = Identity::generate(); - let nsec = encode_nsec(&identity.keypair().secret_key()); + // `keypair()` and `secret_key()` each hand back a whole private key + // rather than a handle, so both temporaries are bound and erased. + let mut our_keypair = identity.keypair(); + let mut secret_key = our_keypair.secret_key(); + let nsec = encode_nsec(&secret_key); + secret_key.non_secure_erase(); + our_keypair.non_secure_erase(); let npub = identity.npub(); if let Some(parent) = key_path.parent() { @@ -571,6 +592,14 @@ pub fn resolve_identity( } /// Result of identity resolution. +/// +/// `nsec` is the node's private key in plaintext. Every local that carries it +/// through [`resolve_identity`] either moves into this struct or is held in a +/// guard that clears it, so this is where the surviving string lives and where +/// clearing it belongs. +/// A caller that wants the value out should take it with [`Option::take`] or +/// `std::mem::take` rather than moving the field, which the `Drop` below +/// forbids. pub struct ResolvedIdentity { /// The nsec string (bech32 or hex) for creating an Identity. pub nsec: String, @@ -578,6 +607,14 @@ pub struct ResolvedIdentity { pub source: IdentitySource, } +impl Drop for ResolvedIdentity { + /// Clear the plaintext private key rather than dropping the allocation + /// with the key still in it. + fn drop(&mut self) { + self.nsec.zeroize(); + } +} + /// Where a resolved identity originated. pub enum IdentitySource { /// From explicit nsec in config file. @@ -639,6 +676,20 @@ pub struct IdentityConfig { pub persistent: bool, } +impl Drop for IdentityConfig { + /// Clear the plaintext private key. + /// + /// This field holds the node's private key for the whole process + /// lifetime, which is the longest any secret lives in this crate, so + /// leaving the allocation to be freed with the key still in it is the + /// largest residue the crate can reach. A caller that needs the value out + /// should take it with [`Option::take`]; moving the field is what the + /// `Drop` forbids. + fn drop(&mut self) { + self.nsec.zeroize(); + } +} + /// Root configuration structure. #[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct Config { @@ -709,10 +760,16 @@ impl Config { /// Load configuration from a single file. pub fn load_file(path: &Path) -> Result { - let contents = std::fs::read_to_string(path).map_err(|e| ConfigError::ReadFile { - path: path.to_path_buf(), - source: e, - })?; + // The config file is the highest-priority home of a plaintext key: + // `node.identity.nsec` is read straight out of it, so the whole file + // text is treated as secret for as long as it is held. + let contents = + Zeroizing::new( + std::fs::read_to_string(path).map_err(|e| ConfigError::ReadFile { + path: path.to_path_buf(), + source: e, + })?, + ); serde_yaml::from_str(&contents).map_err(|e| ConfigError::ParseYaml { path: path.to_path_buf(), @@ -755,10 +812,19 @@ impl Config { /// Merge another configuration into this one. /// /// Values from `other` override values in `self` when present. - pub fn merge(&mut self, other: Config) { - // Merge node.identity section + pub fn merge(&mut self, mut other: Config) { + // Merge node.identity section. The nsec is taken rather than moved + // out of `other.node.identity`, which clears its private key on drop + // and so cannot be left partially moved. if other.node.identity.nsec.is_some() { - self.node.identity.nsec = other.node.identity.nsec; + // Clear whatever this field already held before overwriting it. + // Assigning over the field drops the old `String` in place, which + // does not run `Drop for IdentityConfig` and would free a + // plaintext key uncleared when two config files both carry one. + if let Some(mut old) = self.node.identity.nsec.take() { + old.zeroize(); + } + self.node.identity.nsec = other.node.identity.nsec.take(); } if other.node.identity.persistent { self.node.identity.persistent = true; diff --git a/src/discovery/nostr/runtime.rs b/src/discovery/nostr/runtime.rs index af8c68f8..41704467 100644 --- a/src/discovery/nostr/runtime.rs +++ b/src/discovery/nostr/runtime.rs @@ -15,6 +15,7 @@ use serde::Serialize; use tokio::sync::{Mutex, Notify, RwLock, broadcast, mpsc, oneshot}; use tokio::task::JoinHandle; use tracing::{debug, info, trace, warn}; +use zeroize::{Zeroize, Zeroizing}; use super::failure_state::FailureState; use super::offer_admission::{AdmissionReject, OfferAdmission}; @@ -273,7 +274,15 @@ impl NostrDiscovery { return Err(BootstrapError::Disabled); } - let keys = nostr::Keys::parse(&hex::encode(identity.keypair().secret_bytes())) + // Three copies of the private key are made to reach `Keys::parse`: + // the keypair, its raw bytes, and the hex string. Each is bound and + // cleared here; `nostr::Keys` clears its own on drop. + let mut our_keypair = identity.keypair(); + let mut secret_bytes = our_keypair.secret_bytes(); + let secret_hex = Zeroizing::new(hex::encode(secret_bytes)); + secret_bytes.zeroize(); + our_keypair.non_secure_erase(); + let keys = nostr::Keys::parse(secret_hex.as_str()) .map_err(|e| BootstrapError::Nostr(e.to_string()))?; let client = Client::builder() .signer(keys.clone()) diff --git a/src/identity/encoding.rs b/src/identity/encoding.rs index d81f9aa6..772a635f 100644 --- a/src/identity/encoding.rs +++ b/src/identity/encoding.rs @@ -2,6 +2,7 @@ use bech32::{Bech32, Hrp}; use secp256k1::{SecretKey, XOnlyPublicKey}; +use zeroize::{Zeroize, Zeroizing}; use super::IdentityError; @@ -33,14 +34,25 @@ pub fn decode_npub(npub: &str) -> Result { } /// Encode a secret key as a bech32 nsec string (NIP-19). +/// +/// The returned string is the private key in another encoding, so it is the +/// caller's to clear. What this function clears is the raw byte copy +/// `secret_bytes` hands back, which would otherwise sit in an unnamed +/// temporary until the end of the statement. pub fn encode_nsec(secret_key: &SecretKey) -> String { - bech32::encode::(NSEC_HRP, &secret_key.secret_bytes()) - .expect("nsec encoding cannot fail") + let mut secret_bytes = secret_key.secret_bytes(); + let nsec = + bech32::encode::(NSEC_HRP, &secret_bytes).expect("nsec encoding cannot fail"); + secret_bytes.zeroize(); + nsec } /// Decode an nsec string to a secret key. pub fn decode_nsec(nsec: &str) -> Result { let (hrp, data) = bech32::decode(nsec)?; + // `data` is the raw private key. The guard clears it on every exit path, + // including the two length and prefix rejections below. + let data = Zeroizing::new(data); if hrp != NSEC_HRP { return Err(IdentityError::InvalidNsecPrefix(hrp.to_string())); @@ -59,7 +71,9 @@ pub fn decode_secret(s: &str) -> Result { if s.starts_with("nsec1") { decode_nsec(s) } else { - let bytes = hex::decode(s)?; + // `bytes` is the raw private key; the guard clears it on both the + // length rejection and the normal return. + let bytes = Zeroizing::new(hex::decode(s)?); if bytes.len() != 32 { return Err(IdentityError::InvalidNsecLength(bytes.len())); } diff --git a/src/identity/local.rs b/src/identity/local.rs index 26db8e17..7a494369 100644 --- a/src/identity/local.rs +++ b/src/identity/local.rs @@ -2,6 +2,7 @@ use secp256k1::{Keypair, PublicKey, SecretKey, XOnlyPublicKey}; use std::fmt; +use zeroize::Zeroize; use super::auth::{AuthResponse, auth_challenge_digest}; use super::encoding::{decode_secret, encode_npub}; @@ -11,6 +12,13 @@ use super::{FipsAddress, IdentityError, NodeAddr, sha256}; /// /// The identity holds the secp256k1 keypair and provides methods for signing /// and verifying protocol messages. +/// +/// The keypair is the node's long-term private key. It is erased when the +/// identity is dropped, and every constructor below erases the intermediate +/// secret it built the identity from. All of that clears the copies this +/// crate owns, not every copy that ever existed: `secp256k1` names its erase +/// non-secure because the compiler may duplicate or move the bytes to places +/// no code here can name. #[derive(Clone)] pub struct Identity { keypair: Keypair, @@ -23,39 +31,51 @@ impl Identity { pub fn generate() -> Self { let mut secret_bytes = [0u8; 32]; rand::Rng::fill_bytes(&mut rand::rng(), &mut secret_bytes); - let secret_key = + let mut secret_key = SecretKey::from_slice(&secret_bytes).expect("32 random bytes is a valid secret key"); - Self::from_secret_key(secret_key) + let identity = Self::from_secret_key(secret_key); + secret_bytes.zeroize(); + secret_key.non_secure_erase(); + identity } /// Create an identity from an existing keypair. - pub fn from_keypair(keypair: Keypair) -> Self { + pub fn from_keypair(mut keypair: Keypair) -> Self { let (pubkey, _parity) = keypair.x_only_public_key(); let node_addr = NodeAddr::from_pubkey(&pubkey); let address = FipsAddress::from_node_addr(&node_addr); - Self { + let identity = Self { keypair, node_addr, address, - } + }; + keypair.non_secure_erase(); + identity } /// Create an identity from a secret key. - pub fn from_secret_key(secret_key: SecretKey) -> Self { - let keypair = Keypair::from_secret_key(&super::SECP, &secret_key); - Self::from_keypair(keypair) + pub fn from_secret_key(mut secret_key: SecretKey) -> Self { + let mut keypair = Keypair::from_secret_key(&super::SECP, &secret_key); + let identity = Self::from_keypair(keypair); + keypair.non_secure_erase(); + secret_key.non_secure_erase(); + identity } /// Create an identity from secret key bytes. pub fn from_secret_bytes(bytes: &[u8; 32]) -> Result { - let secret_key = SecretKey::from_slice(bytes)?; - Ok(Self::from_secret_key(secret_key)) + let mut secret_key = SecretKey::from_slice(bytes)?; + let identity = Self::from_secret_key(secret_key); + secret_key.non_secure_erase(); + Ok(identity) } /// Create an identity from an nsec string (bech32) or hex-encoded secret. pub fn from_secret_str(s: &str) -> Result { - let secret_key = decode_secret(s)?; - Ok(Self::from_secret_key(secret_key)) + let mut secret_key = decode_secret(s)?; + let identity = Self::from_secret_key(secret_key); + secret_key.non_secure_erase(); + Ok(identity) } /// Return the underlying keypair. @@ -110,6 +130,51 @@ impl Identity { } } +impl Drop for Identity { + /// Erase the long-term private key this identity owns. + /// + /// `Keypair` is `Copy` and so cannot clear itself on drop; `Identity` is + /// not, so it does it for the copy it holds. See the type's own + /// documentation for what that does and does not reach. + fn drop(&mut self) { + self.keypair.non_secure_erase(); + } +} + +/// A `Keypair` copy that is erased 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. +/// +/// This clears the copy this guard owns, not every copy that ever existed: +/// `secp256k1` names its erase non-secure because the compiler may duplicate +/// or move the bytes to places no code here can name. +pub(crate) struct ErasingKeypair(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 + } + + /// Borrow the guarded keypair. + pub(crate) fn get(&self) -> &Keypair { + &self.0 + } +} + +impl Drop for ErasingKeypair { + fn drop(&mut self) { + self.0.non_secure_erase(); + } +} + impl fmt::Debug for Identity { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("Identity") diff --git a/src/identity/mod.rs b/src/identity/mod.rs index 6093427d..b260f8ce 100644 --- a/src/identity/mod.rs +++ b/src/identity/mod.rs @@ -20,6 +20,7 @@ use thiserror::Error; pub use address::FipsAddress; pub use auth::{AuthChallenge, AuthResponse}; pub use encoding::{decode_npub, decode_nsec, decode_secret, encode_npub, encode_nsec}; +pub(crate) use local::ErasingKeypair; pub use local::Identity; pub use node_addr::NodeAddr; pub use peer::PeerIdentity; diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index 4d5d9384..cc73a1ea 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -226,14 +226,18 @@ impl Node { packet.timestamp_ms, ); - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let noise_msg1 = &packet.data[header.noise_msg1_offset..]; - let msg2_response = match conn.receive_handshake_init( + let init_result = conn.receive_handshake_init( our_keypair, self.startup_epoch(), noise_msg1, packet.timestamp_ms, - ) { + ); + our_keypair.non_secure_erase(); + let msg2_response = match init_result { Ok(m) => m, Err(e) => { debug!( diff --git a/src/node/handlers/rekey.rs b/src/node/handlers/rekey.rs index fc2c22fb..bac07b8a 100644 --- a/src/node/handlers/rekey.rs +++ b/src/node/handlers/rekey.rs @@ -194,8 +194,11 @@ impl Node { }; // Create IK initiator handshake directly (no PeerConnection) - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut hs = HandshakeState::new_initiator(our_keypair, peer_pubkey); + our_keypair.non_secure_erase(); hs.set_local_epoch(self.startup_epoch()); let noise_msg1 = match hs.write_message_1() { @@ -605,8 +608,11 @@ impl Node { let dest_pubkey = *entry.remote_pubkey(); // Create Noise XK initiator handshake - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut handshake = HandshakeState::new_xk_initiator(our_keypair, dest_pubkey); + our_keypair.non_secure_erase(); handshake.set_local_epoch(self.startup_epoch()); let msg1 = match handshake.write_xk_message_1() { diff --git a/src/node/handlers/session.rs b/src/node/handlers/session.rs index 2e892075..c8e11bcb 100644 --- a/src/node/handlers/session.rs +++ b/src/node/handlers/session.rs @@ -626,8 +626,11 @@ impl Node { .record_reject(RejectReason::Session(SessionReject::RekeyPending)); return; } - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; + // the handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut handshake = HandshakeState::new_xk_responder(our_keypair); + our_keypair.non_secure_erase(); handshake.set_local_epoch(self.startup_epoch()); if let Err(e) = handshake.read_xk_message_1(&setup.handshake_payload) { @@ -681,8 +684,11 @@ impl Node { } // Create XK responder handshake and process msg1 - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut handshake = HandshakeState::new_xk_responder(our_keypair); + our_keypair.non_secure_erase(); handshake.set_local_epoch(self.startup_epoch()); if let Err(e) = handshake.read_xk_message_1(&setup.handshake_payload) { @@ -720,7 +726,11 @@ impl Node { // Store session entry in AwaitingMsg3 state with ack payload for potential resend. // Use a dummy pubkey since we don't know the initiator's identity yet. // We use our own pubkey as placeholder; it will be replaced in handle_session_msg3. - let placeholder_pubkey = self.identity().keypair().public_key(); + // `keypair()` hands back a copy of the long-term private key, so the + // temporary is bound and erased rather than left to the statement end. + let mut our_keypair = self.identity().keypair(); + let placeholder_pubkey = our_keypair.public_key(); + our_keypair.non_secure_erase(); let now_ms = Self::now_ms(); let resend_interval = self.config().node.rate_limit.handshake_resend_interval_ms; let mut entry = SessionEntry::new( @@ -1728,8 +1738,11 @@ impl Node { } // Create Noise XK initiator handshake - let our_keypair = self.identity().keypair(); + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); let mut handshake = HandshakeState::new_xk_initiator(our_keypair, dest_pubkey); + our_keypair.non_secure_erase(); handshake.set_local_epoch(self.startup_epoch()); let msg1 = handshake .write_xk_message_1() diff --git a/src/node/lifecycle.rs b/src/node/lifecycle.rs index e2523976..e8a7b385 100644 --- a/src/node/lifecycle.rs +++ b/src/node/lifecycle.rs @@ -489,18 +489,22 @@ impl Node { }; // Start the Noise handshake and get message 1 - let our_keypair = self.identity().keypair(); - let noise_msg1 = - match connection.start_handshake(our_keypair, self.startup_epoch(), current_time_ms) { - Ok(msg) => msg, - Err(e) => { - // Clean up the index and link - let _ = self.index_allocator.free(our_index); - self.links.remove(&link_id); - self.addr_to_link.remove(&(transport_id, remote_addr)); - return Err(NodeError::HandshakeFailed(e.to_string())); - } - }; + // This frame's own copy of the node's long-term private key; the + // handshake state keeps its own and clears that on drop. + let mut our_keypair = self.identity().keypair(); + let start_result = + connection.start_handshake(our_keypair, self.startup_epoch(), current_time_ms); + our_keypair.non_secure_erase(); + let noise_msg1 = match start_result { + Ok(msg) => msg, + Err(e) => { + // Clean up the index and link + let _ = self.index_allocator.free(our_index); + self.links.remove(&link_id); + self.addr_to_link.remove(&(transport_id, remote_addr)); + return Err(NodeError::HandshakeFailed(e.to_string())); + } + }; // Set index and transport info on the connection connection.set_our_index(our_index); diff --git a/src/noise/handshake.rs b/src/noise/handshake.rs index b7dbb881..5116707f 100644 --- a/src/noise/handshake.rs +++ b/src/noise/handshake.rs @@ -179,7 +179,7 @@ impl HandshakeState { /// /// The initiator knows the responder's static key and will send first. /// Used by FMP (link layer). - pub fn new_initiator(static_keypair: Keypair, remote_static: PublicKey) -> Self { + pub fn new_initiator(mut static_keypair: Keypair, remote_static: PublicKey) -> Self { let secp = Secp256k1::new(); let mut state = Self { pattern: NoisePattern::Ik, @@ -201,6 +201,10 @@ impl HandshakeState { let normalized = Self::normalize_for_premessage(&remote_static); state.symmetric.mix_hash(&normalized); + // The struct now holds its own copy of the long-term private key and + // clears it on drop, so this frame's parameter copy is erased here. + static_keypair.non_secure_erase(); + state } @@ -208,7 +212,7 @@ impl HandshakeState { /// /// The responder does NOT know the initiator's static key - it will be /// learned from message 1. Used by FMP (link layer). - pub fn new_responder(static_keypair: Keypair) -> Self { + pub fn new_responder(mut static_keypair: Keypair) -> Self { let secp = Secp256k1::new(); let mut state = Self { pattern: NoisePattern::Ik, @@ -229,6 +233,10 @@ impl HandshakeState { let normalized = Self::normalize_for_premessage(&state.static_keypair.public_key()); state.symmetric.mix_hash(&normalized); + // The struct now holds its own copy of the long-term private key and + // clears it on drop, so this frame's parameter copy is erased here. + static_keypair.non_secure_erase(); + state } @@ -236,7 +244,7 @@ impl HandshakeState { /// /// The initiator knows the responder's static key. XK defers the /// initiator's static key reveal to msg3. Used by FSP (session layer). - pub fn new_xk_initiator(static_keypair: Keypair, remote_static: PublicKey) -> Self { + pub fn new_xk_initiator(mut static_keypair: Keypair, remote_static: PublicKey) -> Self { let secp = Secp256k1::new(); let mut state = Self { pattern: NoisePattern::Xk, @@ -256,6 +264,10 @@ impl HandshakeState { let normalized = Self::normalize_for_premessage(&remote_static); state.symmetric.mix_hash(&normalized); + // The struct now holds its own copy of the long-term private key and + // clears it on drop, so this frame's parameter copy is erased here. + static_keypair.non_secure_erase(); + state } @@ -263,7 +275,7 @@ impl HandshakeState { /// /// The responder does NOT know the initiator's static key - it will be /// learned from message 3. Used by FSP (session layer). - pub fn new_xk_responder(static_keypair: Keypair) -> Self { + pub fn new_xk_responder(mut static_keypair: Keypair) -> Self { let secp = Secp256k1::new(); let mut state = Self { pattern: NoisePattern::Xk, @@ -283,6 +295,10 @@ impl HandshakeState { let normalized = Self::normalize_for_premessage(&state.static_keypair.public_key()); state.symmetric.mix_hash(&normalized); + // The struct now holds its own copy of the long-term private key and + // clears it on drop, so this frame's parameter copy is erased here. + static_keypair.non_secure_erase(); + state } @@ -322,9 +338,15 @@ impl HandshakeState { let mut secret_bytes = [0u8; 32]; rng.fill_bytes(&mut secret_bytes); - let secret_key = + let mut secret_key = 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`. + secret_key.non_secure_erase(); } /// Perform ECDH between our secret and their public key. @@ -335,15 +357,26 @@ impl HandshakeState { /// may have the wrong parity for the responder's static key. Since P and /// -P produce ECDH result points with the same x-coordinate, hashing /// only x ensures both sides derive the same shared secret. + /// + /// `our_secret` is borrowed, so this frame makes no copy of it. Every + /// 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. fn ecdh(&self, our_secret: &SecretKey, their_public: &PublicKey) -> [u8; 32] { // Get raw (x, y) coordinates (64 bytes) without any hashing - let point = shared_secret_point(their_public, our_secret); + let mut point = shared_secret_point(their_public, our_secret); // Hash only the x-coordinate (first 32 bytes), ignoring y/parity let mut hasher = Sha256::new(); hasher.update(&point[..32]); - let hash = hasher.finalize(); + let mut hash = hasher.finalize(); let mut result = [0u8; 32]; result.copy_from_slice(&hash); + hash.as_mut_slice().zeroize(); + point.zeroize(); + // `result` is moved out, so clearing it belongs to the caller. result } @@ -388,8 +421,11 @@ impl HandshakeState { self.symmetric.mix_hash(&e_pub); // -> es: DH(e, rs), mix into key - let es = self.ecdh(&ephemeral.secret_key(), &remote_static); + let mut sk = ephemeral.secret_key(); + let mut es = self.ecdh(&sk, &remote_static); self.symmetric.mix_key(&es); + es.zeroize(); + sk.non_secure_erase(); // -> s: encrypt our static and send let our_static = self.static_keypair.public_key().serialize(); @@ -397,8 +433,11 @@ impl HandshakeState { message.extend_from_slice(&encrypted_static); // -> ss: DH(s, rs), mix into key - let ss = self.ecdh(&self.static_keypair.secret_key(), &remote_static); + let mut sk = self.static_keypair.secret_key(); + let mut ss = self.ecdh(&sk, &remote_static); self.symmetric.mix_key(&ss); + ss.zeroize(); + sk.non_secure_erase(); // -> epoch: encrypt startup epoch for restart detection let encrypted_epoch = self.symmetric.encrypt_and_hash(&epoch)?; @@ -441,8 +480,11 @@ impl HandshakeState { // -> es: DH(s, re), mix into key // (responder uses their static with initiator's ephemeral) - let es = self.ecdh(&self.static_keypair.secret_key(), &re); + let mut sk = self.static_keypair.secret_key(); + let mut es = self.ecdh(&sk, &re); self.symmetric.mix_key(&es); + es.zeroize(); + sk.non_secure_erase(); // -> s: decrypt initiator's static let encrypted_static_end = PUBKEY_SIZE + PUBKEY_SIZE + super::TAG_SIZE; @@ -453,8 +495,11 @@ impl HandshakeState { self.remote_static = Some(rs); // -> ss: DH(s, rs), mix into key - let ss = self.ecdh(&self.static_keypair.secret_key(), &rs); + let mut sk = self.static_keypair.secret_key(); + let mut ss = self.ecdh(&sk, &rs); self.symmetric.mix_key(&ss); + ss.zeroize(); + sk.non_secure_erase(); // -> epoch: decrypt initiator's startup epoch let encrypted_epoch = &message[encrypted_static_end..]; @@ -508,12 +553,18 @@ impl HandshakeState { self.symmetric.mix_hash(&e_pub); // <- ee: DH(e, re), mix into key - let ee = self.ecdh(&ephemeral.secret_key(), &re); + let mut sk = ephemeral.secret_key(); + let mut ee = self.ecdh(&sk, &re); self.symmetric.mix_key(&ee); + ee.zeroize(); + sk.non_secure_erase(); // <- se: DH(s, re), mix into key - let se = self.ecdh(&self.static_keypair.secret_key(), &re); + let mut sk = self.static_keypair.secret_key(); + let mut se = self.ecdh(&sk, &re); self.symmetric.mix_key(&se); + se.zeroize(); + sk.non_secure_erase(); // <- epoch: encrypt startup epoch for restart detection let encrypted_epoch = self.symmetric.encrypt_and_hash(&epoch)?; @@ -556,14 +607,20 @@ impl HandshakeState { // <- ee: DH(e, re), mix into key let ephemeral = self.ephemeral_keypair.as_ref().unwrap(); - let ee = self.ecdh(&ephemeral.secret_key(), &re); + let mut sk = ephemeral.secret_key(); + let mut ee = self.ecdh(&sk, &re); self.symmetric.mix_key(&ee); + ee.zeroize(); + sk.non_secure_erase(); // <- se: DH(e, rs), mix into key // (initiator uses their ephemeral with responder's static) let rs = self.remote_static.expect("initiator has remote static"); - let se = self.ecdh(&ephemeral.secret_key(), &rs); + let mut sk = ephemeral.secret_key(); + let mut se = self.ecdh(&sk, &rs); self.symmetric.mix_key(&se); + se.zeroize(); + sk.non_secure_erase(); // <- epoch: decrypt responder's startup epoch let encrypted_epoch = &message[PUBKEY_SIZE..]; @@ -620,8 +677,11 @@ impl HandshakeState { self.symmetric.mix_hash(&e_pub); // -> es: DH(e, rs), mix into key - let es = self.ecdh(&ephemeral.secret_key(), &remote_static); + let mut sk = ephemeral.secret_key(); + let mut es = self.ecdh(&sk, &remote_static); self.symmetric.mix_key(&es); + es.zeroize(); + sk.non_secure_erase(); self.progress = HandshakeProgress::Message1Done; @@ -660,8 +720,11 @@ impl HandshakeState { // -> es: DH(s, re), mix into key // (responder uses their static with initiator's ephemeral) - let es = self.ecdh(&self.static_keypair.secret_key(), &re); + let mut sk = self.static_keypair.secret_key(); + let mut es = self.ecdh(&sk, &re); self.symmetric.mix_key(&es); + es.zeroize(); + sk.non_secure_erase(); self.progress = HandshakeProgress::Message1Done; @@ -707,8 +770,11 @@ impl HandshakeState { self.symmetric.mix_hash(&e_pub); // <- ee: DH(e, re), mix into key - let ee = self.ecdh(&ephemeral.secret_key(), &re); + let mut sk = ephemeral.secret_key(); + let mut ee = self.ecdh(&sk, &re); self.symmetric.mix_key(&ee); + ee.zeroize(); + sk.non_secure_erase(); // <- epoch: encrypt startup epoch for restart detection let encrypted_epoch = self.symmetric.encrypt_and_hash(&epoch)?; @@ -752,8 +818,11 @@ impl HandshakeState { // <- ee: DH(e, re), mix into key let ephemeral = self.ephemeral_keypair.as_ref().unwrap(); - let ee = self.ecdh(&ephemeral.secret_key(), &re); + let mut sk = ephemeral.secret_key(); + let mut ee = self.ecdh(&sk, &re); self.symmetric.mix_key(&ee); + ee.zeroize(); + sk.non_secure_erase(); // <- epoch: decrypt responder's startup epoch let encrypted_epoch = &message[PUBKEY_SIZE..]; @@ -839,8 +908,11 @@ impl HandshakeState { message.extend_from_slice(&encrypted_static); // -> se: DH(s, re), mix into key - let se = self.ecdh(&self.static_keypair.secret_key(), &re); + let mut sk = self.static_keypair.secret_key(); + let mut se = self.ecdh(&sk, &re); self.symmetric.mix_key(&se); + se.zeroize(); + sk.non_secure_erase(); // -> epoch: encrypt startup epoch for restart detection let encrypted_epoch = self.symmetric.encrypt_and_hash(&epoch)?; @@ -890,8 +962,11 @@ impl HandshakeState { .ephemeral_keypair .as_ref() .expect("should have ephemeral after msg2"); - let se = self.ecdh(&ephemeral.secret_key(), &rs); + let mut sk = ephemeral.secret_key(); + let mut se = self.ecdh(&sk, &rs); self.symmetric.mix_key(&se); + se.zeroize(); + sk.non_secure_erase(); // -> epoch: decrypt initiator's startup epoch let encrypted_epoch = &message[encrypted_static_end..]; @@ -943,6 +1018,26 @@ impl HandshakeState { } } +impl Drop for HandshakeState { + /// Erase the two private keys this state holds. + /// + /// `static_keypair` is the node's long-term private key and + /// `ephemeral_keypair` is the handshake's own. Both live here for the + /// whole handshake, which is longer than in any other frame, so this is + /// where clearing them matters most. `Keypair` is `Copy` and so cannot + /// clear itself on drop; `HandshakeState` is not, so it does it for both. + /// + /// This clears the copies this crate owns, not every copy that ever + /// existed: `secp256k1` names its erase non-secure because the compiler + /// may duplicate or move the bytes to places no code here can name. + fn drop(&mut self) { + self.static_keypair.non_secure_erase(); + if let Some(ephemeral) = self.ephemeral_keypair.as_mut() { + ephemeral.non_secure_erase(); + } + } +} + impl fmt::Debug for HandshakeState { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("HandshakeState") diff --git a/src/noise/mod.rs b/src/noise/mod.rs index e2c21be0..85366e4f 100644 --- a/src/noise/mod.rs +++ b/src/noise/mod.rs @@ -229,14 +229,19 @@ impl Clone for CipherState { impl CipherState { /// Create a new cipher state with the given key. - pub(crate) fn new(key: [u8; 32]) -> Self { + /// + /// The parameter is this frame's own copy of live key material, so it is + /// cleared before returning. The caller's copy stays the caller's to clear. + pub(crate) fn new(mut key: [u8; 32]) -> Self { let cipher = Self::build_cipher(&key); - Self { + let state = Self { key, cipher, nonce: 0, has_key: true, - } + }; + key.zeroize(); + state } /// Create an empty cipher state (no key yet). @@ -250,11 +255,15 @@ impl CipherState { } /// Initialize with a key. - pub(super) fn initialize_key(&mut self, key: [u8; 32]) { + /// + /// The parameter is this frame's own copy of live key material, so it is + /// cleared before returning. The caller's copy stays the caller's to clear. + pub(super) fn initialize_key(&mut self, mut key: [u8; 32]) { self.key = key; self.cipher = Self::build_cipher(&key); self.nonce = 0; self.has_key = true; + key.zeroize(); } /// Build a ring `LessSafeKey` from raw key bytes. Centralized so the diff --git a/src/noise/tests.rs b/src/noise/tests.rs index c03a0ef8..82cd7a93 100644 --- a/src/noise/tests.rs +++ b/src/noise/tests.rs @@ -212,7 +212,11 @@ fn cipher_state_drop_impl_clears_the_retained_key() { assert_zeroize_on_drop::(); let mut cipher = CipherState::new([7u8; 32]); - assert_eq!(cipher.key_bytes(), [7u8; 32]); + // `key_bytes` hands back a copy of live key material, so the observation + // is bound and cleared rather than left in an unnamed temporary. + let mut observed = cipher.key_bytes(); + assert_eq!(observed, [7u8; 32]); + observed.zeroize(); assert!(cipher.has_key()); cipher.zeroize(); diff --git a/src/peer/connection.rs b/src/peer/connection.rs index bc6c6e21..8ba399cf 100644 --- a/src/peer/connection.rs +++ b/src/peer/connection.rs @@ -5,6 +5,7 @@ //! ActivePeer upon successful authentication. use crate::PeerIdentity; +use crate::identity::ErasingKeypair; use crate::noise::{self, NoiseError, NoiseSession}; use crate::transport::{LinkDirection, LinkId, LinkStats, TransportAddr, TransportId}; use crate::utils::index::SessionIndex; @@ -402,10 +403,15 @@ impl PeerConnection { /// The epoch is our startup epoch, encrypted into msg1 for restart detection. pub fn start_handshake( &mut self, - our_keypair: Keypair, + mut our_keypair: Keypair, epoch: [u8; 8], current_time_ms: u64, ) -> 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); + if self.direction != LinkDirection::Outbound { return Err(NoiseError::WrongState { expected: "outbound connection".to_string(), @@ -426,7 +432,9 @@ impl PeerConnection { .expect("outbound must have expected identity") .pubkey_full(); - let mut hs = noise::HandshakeState::new_initiator(our_keypair, remote_static); + let mut kp = *our_keypair.get(); + let mut hs = noise::HandshakeState::new_initiator(kp, remote_static); + kp.non_secure_erase(); hs.set_local_epoch(epoch); let msg1 = hs.write_message_1()?; @@ -443,11 +451,15 @@ impl PeerConnection { /// The epoch is our startup epoch, encrypted into msg2 for restart detection. pub fn receive_handshake_init( &mut self, - our_keypair: Keypair, + mut our_keypair: Keypair, epoch: [u8; 8], message: &[u8], 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); + if self.direction != LinkDirection::Inbound { return Err(NoiseError::WrongState { expected: "inbound connection".to_string(), @@ -462,7 +474,9 @@ impl PeerConnection { }); } - let mut hs = noise::HandshakeState::new_responder(our_keypair); + let mut kp = *our_keypair.get(); + let mut hs = noise::HandshakeState::new_responder(kp); + kp.non_secure_erase(); hs.set_local_epoch(epoch); // Process message 1 (this reveals the initiator's identity and epoch)