diff --git a/Cargo.lock b/Cargo.lock index 417a60e6..1419cb8a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6048,6 +6048,7 @@ dependencies = [ "willow-messaging", "willow-transport", "x25519-dalek", + "zeroize", ] [[package]] @@ -6060,9 +6061,11 @@ dependencies = [ "iroh-base", "rand 0.9.2", "serde", + "tempfile", "thiserror 1.0.69", "tokio", "willow-transport", + "zeroize", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index d1845ecd..baba6331 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,6 +20,7 @@ thiserror = "1" tokio = { version = "1" } # Crypto & Networking +zeroize = { version = "1", features = ["derive"] } iroh = "0.97" iroh-base = "0.97" iroh-gossip = "0.97" diff --git a/crates/crypto/Cargo.toml b/crates/crypto/Cargo.toml index 8e1046df..67035758 100644 --- a/crates/crypto/Cargo.toml +++ b/crates/crypto/Cargo.toml @@ -18,6 +18,7 @@ sha2 = "0.10" rand = "0.8" serde = { workspace = true } thiserror = { workspace = true } +zeroize = { workspace = true } [target.'cfg(target_arch = "wasm32")'.dependencies] getrandom_02 = { package = "getrandom", version = "0.2", features = ["js"] } diff --git a/crates/crypto/src/lib.rs b/crates/crypto/src/lib.rs index 84167a52..5bd30236 100644 --- a/crates/crypto/src/lib.rs +++ b/crates/crypto/src/lib.rs @@ -21,6 +21,7 @@ use rand::RngCore; use serde::{Deserialize, Serialize}; use sha2::Sha256; use x25519_dalek::{PublicKey as X25519Public, StaticSecret as X25519Secret}; +use zeroize::ZeroizeOnDrop; use willow_identity::Identity; @@ -54,8 +55,11 @@ pub enum CryptoError { // ───── Types ──────────────────────────────────────────────────────────────── /// A 256-bit symmetric key for encrypting a channel's messages. -#[derive(Clone, PartialEq)] -pub struct ChannelKey(pub(crate) [u8; 32]); +/// +/// The wrapped bytes are zeroized when the value is dropped so secret +/// material doesn't linger in freed memory. +#[derive(Clone, PartialEq, ZeroizeOnDrop)] +pub struct ChannelKey(#[zeroize] pub(crate) [u8; 32]); impl std::fmt::Debug for ChannelKey { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -749,6 +753,14 @@ mod tests { assert!(!debug.contains(&format!("{:02x}", key.as_bytes()[0]))); } + /// Compile-time guarantee that [`ChannelKey`] zeroizes its secret + /// bytes on drop. See issue #127. + #[test] + fn channel_key_is_zeroize_on_drop() { + fn assert_zeroize_on_drop() {} + assert_zeroize_on_drop::(); + } + // ── Issue #110: ratchet counter DoS bound ────────────────────── /// Regression guard for issue #110: a sealed packet with a huge diff --git a/crates/identity/Cargo.toml b/crates/identity/Cargo.toml index cf78b45c..6942b9ce 100644 --- a/crates/identity/Cargo.toml +++ b/crates/identity/Cargo.toml @@ -13,6 +13,7 @@ serde = { workspace = true } thiserror = { workspace = true } chrono = { workspace = true } rand = "0.9" +zeroize = { workspace = true } [target.'cfg(target_arch = "wasm32")'.dependencies] getrandom_02 = { package = "getrandom", version = "0.2", features = ["js"] } @@ -20,3 +21,4 @@ getrandom_03 = { package = "getrandom", version = "0.3", features = ["wasm_js"] [dev-dependencies] tokio = { workspace = true } +tempfile = "3" diff --git a/crates/identity/src/lib.rs b/crates/identity/src/lib.rs index 536222a4..d1945821 100644 --- a/crates/identity/src/lib.rs +++ b/crates/identity/src/lib.rs @@ -32,6 +32,7 @@ use chrono::{DateTime, Utc}; use serde::{de::DeserializeOwned, Deserialize, Serialize}; +use zeroize::{Zeroize, ZeroizeOnDrop}; // Re-export iroh identity types so downstream crates can use them // without depending on iroh-base directly. @@ -58,6 +59,19 @@ pub enum IdentityError { #[error("failed to decode public key: {0}")] PublicKeyDecode(String), + /// An on-disk key file has loose permissions (readable by group or + /// other). Refuse to load it rather than silently use a leaked key. + #[error( + "refusing to load key file {path} with insecure permissions {mode:#o} \ + (must not be readable or writable by group/other; expected mode 0o600)" + )] + InsecurePermissions { + /// Filesystem path of the offending key file. + path: String, + /// The Unix mode bits of the file. + mode: u32, + }, + /// An I/O or other error. #[error("{0}")] Other(String), @@ -72,8 +86,16 @@ pub enum IdentityError { /// /// `Identity` is cheap to clone (the secret key is 32 bytes, copied on clone) /// and is `Send + Sync` so it can be shared across tokio tasks. -#[derive(Clone)] +/// +/// The wrapped [`SecretKey`] is itself `ZeroizeOnDrop` (provided by +/// `iroh-base`), so the secret material is wiped from memory when the +/// last clone is dropped. We derive [`ZeroizeOnDrop`] on `Identity` +/// itself with `#[zeroize(skip)]` so the type-level guarantee is +/// visible to downstream crates and will fail to compile if the inner +/// representation ever changes to a non-zeroizing type. +#[derive(Clone, ZeroizeOnDrop)] pub struct Identity { + #[zeroize(skip)] secret_key: SecretKey, } @@ -96,10 +118,36 @@ impl Identity { } /// Export this identity as raw Ed25519 secret key bytes (32 bytes). + /// + /// **Security:** the returned `Vec` contains live secret key + /// material. The buffer is *not* automatically zeroized — callers + /// are responsible for wiping it as soon as they're done with it + /// (e.g. by passing it through [`zeroize::Zeroize::zeroize`] on a + /// mutable binding before drop, or by holding it in a + /// `zeroize::Zeroizing>` wrapper). + /// + /// Prefer [`Identity::with_secret_bytes`] when possible: it + /// confines the exposed bytes to the closure's scope and zeroizes + /// them automatically. + #[must_use = "the returned Vec contains secret key material; \ + zeroize it after use, or prefer Identity::with_secret_bytes"] pub fn to_bytes(&self) -> Vec { self.secret_key.to_bytes().to_vec() } + /// Run a closure with temporary access to this identity's raw 32-byte + /// Ed25519 secret key, then zeroize the buffer before returning. + /// + /// This is the preferred way to read the secret bytes — the buffer + /// never escapes the closure and is wiped from memory automatically, + /// so callers can't accidentally leave secret material lying around. + pub fn with_secret_bytes(&self, f: impl FnOnce(&[u8; 32]) -> R) -> R { + let mut bytes = self.secret_key.to_bytes(); + let result = f(&bytes); + bytes.zeroize(); + result + } + /// Derive the public [`EndpointId`] for this identity. /// /// This is the peer's address on the network — a 32-byte Ed25519 public key. @@ -124,29 +172,136 @@ impl Identity { /// Load an identity from a file, or generate and save a new one. /// - /// The file stores the raw 32-byte Ed25519 secret key. Parent directories - /// are created if they don't exist. + /// The file stores the raw 32-byte Ed25519 secret key. Parent + /// directories are created if they don't exist. + /// + /// On Unix the key file is created with mode `0o600` (owner-only + /// read/write) via an atomic temp-file + rename so a crash during + /// write can't leave a half-written key on disk. When loading an + /// existing file, the mode is checked: if any group/other bits are + /// set, [`IdentityError::InsecurePermissions`] is returned rather + /// than silently using a leaked key. #[cfg(not(target_arch = "wasm32"))] pub fn load_or_generate(path: impl AsRef) -> Result { use std::fs; let path = path.as_ref(); - if let Ok(bytes) = fs::read(path) { - Self::from_bytes(&bytes).ok_or_else(|| { - IdentityError::Other(format!( - "invalid key file: expected 32 bytes, got {}", - bytes.len() - )) - }) - } else { - let identity = Self::generate(); - if let Some(parent) = path.parent() { - fs::create_dir_all(parent).map_err(|e| IdentityError::Other(e.to_string()))?; + match fs::metadata(path) { + Ok(metadata) => { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = metadata.permissions().mode(); + // Reject any group/other access (the bottom 6 bits). + if mode & 0o077 != 0 { + return Err(IdentityError::InsecurePermissions { + path: path.display().to_string(), + mode, + }); + } + } + // `metadata` is consulted only for the perm check; the + // unused binding under non-unix targets is fine. + #[cfg(not(unix))] + let _ = metadata; + + let bytes = fs::read(path).map_err(|e| IdentityError::Other(e.to_string()))?; + Self::from_bytes(&bytes).ok_or_else(|| { + IdentityError::Other(format!( + "invalid key file: expected 32 bytes, got {}", + bytes.len() + )) + }) + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + let identity = Self::generate(); + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).map_err(|e| IdentityError::Other(e.to_string()))?; + } + Self::atomic_write_key(path, &identity)?; + Ok(identity) + } + Err(e) => Err(IdentityError::Other(e.to_string())), + } + } + + /// Atomically write `identity`'s secret bytes to `path`. + /// + /// Writes to a uniquely-named sibling temp file (created with + /// `O_CREAT | O_EXCL` so a concurrent startup race cannot clobber + /// it), `fsync`s on Unix, sets mode `0o600`, and `rename`s into + /// place. The rename is atomic on POSIX filesystems, so a crash at + /// any point either leaves the previous key intact or installs the + /// new one — never a truncated half-write. + #[cfg(not(target_arch = "wasm32"))] + fn atomic_write_key(path: &std::path::Path, identity: &Identity) -> Result<(), IdentityError> { + use std::fs::{self, OpenOptions}; + use std::io::Write; + + let parent = path + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .map(std::path::PathBuf::from) + .unwrap_or_else(|| std::path::PathBuf::from(".")); + let file_name = path + .file_name() + .ok_or_else(|| IdentityError::Other("key path has no file name".into()))?; + + // Pick a sibling temp path. Including the PID + a nanosecond + // counter makes accidental collisions astronomically unlikely + // and the `create_new(true)` open below catches the rest. + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0); + let mut tmp_name = std::ffi::OsString::from("."); + tmp_name.push(file_name); + tmp_name.push(format!(".tmp.{}.{}", std::process::id(), nanos)); + let tmp_path = parent.join(tmp_name); + + let mut open_opts = OpenOptions::new(); + open_opts.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + open_opts.mode(0o600); + } + let mut file = open_opts + .open(&tmp_path) + .map_err(|e| IdentityError::Other(format!("open temp key file: {e}")))?; + + // Write the secret bytes through `with_secret_bytes` so the + // intermediate buffer is zeroized as soon as the write returns. + let write_result = + identity.with_secret_bytes(|bytes| -> std::io::Result<()> { file.write_all(bytes) }); + if let Err(e) = write_result { + let _ = fs::remove_file(&tmp_path); + return Err(IdentityError::Other(format!("write temp key file: {e}"))); + } + if let Err(e) = file.sync_all() { + let _ = fs::remove_file(&tmp_path); + return Err(IdentityError::Other(format!("fsync temp key file: {e}"))); + } + // Drop the file handle before rename — Windows requires it, + // POSIX doesn't care. + drop(file); + + // On Unix, belt-and-braces: enforce mode 0o600 even if the + // umask was unusual when we opened the file. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + if let Err(e) = fs::set_permissions(&tmp_path, fs::Permissions::from_mode(0o600)) { + let _ = fs::remove_file(&tmp_path); + return Err(IdentityError::Other(format!("chmod temp key file: {e}"))); } - fs::write(path, identity.to_bytes()) - .map_err(|e| IdentityError::Other(e.to_string()))?; - Ok(identity) } + + if let Err(e) = fs::rename(&tmp_path, path) { + let _ = fs::remove_file(&tmp_path); + return Err(IdentityError::Other(format!("rename key file: {e}"))); + } + Ok(()) } } @@ -384,14 +539,8 @@ mod tests { #[test] #[cfg(not(target_arch = "wasm32"))] fn load_or_generate_persists_identity() { - let dir = std::env::temp_dir().join(format!( - "willow-test-{}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_nanos() - )); - let path = dir.join("identity.key"); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("identity.key"); // First call: generates and saves. let id1 = Identity::load_or_generate(&path).unwrap(); @@ -400,9 +549,96 @@ mod tests { let id2 = Identity::load_or_generate(&path).unwrap(); assert_eq!(id1.endpoint_id(), id2.endpoint_id()); + } + + /// Issue #126: a freshly generated key file must be created with + /// mode `0o600` so it isn't readable by other users on the system. + #[test] + #[cfg(unix)] + fn load_or_generate_creates_file_with_0600_on_unix() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("identity.key"); - // Cleanup. - let _ = std::fs::remove_dir_all(&dir); + let _id = Identity::load_or_generate(&path).unwrap(); + let mode = std::fs::metadata(&path).unwrap().permissions().mode(); + // Strip the file-type bits — we only care about the perm bits. + assert_eq!( + mode & 0o777, + 0o600, + "expected key file mode 0o600, got {:#o}", + mode & 0o777 + ); + } + + /// Issue #126: loading a pre-existing key file with loose + /// permissions (anything readable by group or other) must fail + /// loudly with [`IdentityError::InsecurePermissions`] rather than + /// silently use the leaked key. + #[test] + #[cfg(unix)] + fn load_existing_with_loose_perms_returns_insecure_permissions() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("identity.key"); + + // Plant a key file with mode 0o644 (world-readable). + let id = Identity::generate(); + let bytes = id.to_bytes(); + std::fs::write(&path, &bytes).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + // Wipe the temporary buffer; the file on disk is what matters. + let mut bytes = bytes; + zeroize::Zeroize::zeroize(&mut bytes); + + let err = Identity::load_or_generate(&path).unwrap_err(); + match err { + IdentityError::InsecurePermissions { path: p, mode } => { + assert_eq!(p, path.display().to_string()); + assert_eq!(mode & 0o777, 0o644); + } + other => panic!("expected InsecurePermissions, got {other:?}"), + } + } + + /// Issue #126: a key written by `load_or_generate` must round-trip + /// cleanly through a second call (the perm check on the just-written + /// 0o600 file must succeed). + #[test] + #[cfg(not(target_arch = "wasm32"))] + fn load_existing_with_secure_perms_succeeds() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("identity.key"); + + let id1 = Identity::load_or_generate(&path).unwrap(); + let id2 = Identity::load_or_generate(&path).unwrap(); + assert_eq!(id1.endpoint_id(), id2.endpoint_id()); + } + + /// Issue #127: type-level guarantee that [`Identity`] (and the + /// [`willow_crypto::ChannelKey`] referenced from the issue) zeroize + /// their secret material on drop. This will fail to compile if + /// `Identity` ever loses its `ZeroizeOnDrop` derive or grows a new + /// secret-bearing field that doesn't zeroize. + #[test] + fn identity_is_zeroize_on_drop() { + fn assert_zeroize_on_drop() {} + assert_zeroize_on_drop::(); + } + + /// Issue #127: `with_secret_bytes` exposes the raw 32-byte secret + /// to a closure and zeroizes the buffer afterwards. + #[test] + fn with_secret_bytes_exposes_full_secret() { + let id = Identity::generate(); + // Both of these hold raw secret bytes; wrap them in `Zeroizing` + // so the test itself doesn't leak unzeroized copies on the heap. + let from_to_bytes = zeroize::Zeroizing::new(id.to_bytes()); + let from_callback = zeroize::Zeroizing::new(id.with_secret_bytes(|bytes| bytes.to_vec())); + assert_eq!(*from_to_bytes, *from_callback); + assert_eq!(from_callback.len(), 32); } #[test]