From 9cf221779b3a826a43b2b6fc676870c012d9637d Mon Sep 17 00:00:00 2001 From: Andy Dixon Date: Mon, 12 Oct 2026 07:20:11 +0100 Subject: [PATCH] fix: purge keychain credentials on profile delete, atomic owner-only secret writes, zeroize derived keys - Secrets::delete_profile removes every profile credential kind from the OS keychain as well as the vault (previously only the vault was purged, so keychain passwords/tokens outlived the profile). - write_private writes to a sibling temp file and renames it into place, so a crash mid-write cannot truncate the vault or an export, and a pre-existing world-readable file is replaced rather than reused. - Argon2-derived vault/export keys are Zeroizing and wiped on drop. - Recordings (which capture typed input), JSON exports and --data-dir directories are created owner-only. --- rust/Cargo.lock | 1 + rust/app/src/main.rs | 3 +- rust/app/src/transfer.rs | 7 ++- rust/core/Cargo.toml | 1 + rust/core/src/config.rs | 3 +- rust/core/src/exportimport.rs | 4 +- rust/core/src/recording.rs | 12 ++++- rust/core/src/secrets.rs | 94 +++++++++++++++++++++++++++++------ 8 files changed, 102 insertions(+), 23 deletions(-) diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 985c147..be8aab0 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -2843,6 +2843,7 @@ dependencies = [ "ureq", "uuid", "webpki-roots", + "zeroize", ] [[package]] diff --git a/rust/app/src/main.rs b/rust/app/src/main.rs index 7c7066d..d6faa54 100644 --- a/rust/app/src/main.rs +++ b/rust/app/src/main.rs @@ -82,8 +82,7 @@ fn main() -> eframe::Result { let config = match data_dir { Some(d) => { let c = AppConfig::in_dir(d.into()); - std::fs::create_dir_all(&c.vault_dir).ok(); - Ok(c) + nexus_core::config::create_private_dir(&c.data_dir).and_then(|_| nexus_core::config::create_private_dir(&c.vault_dir)).map(|_| c) } None => AppConfig::load(), }; diff --git a/rust/app/src/transfer.rs b/rust/app/src/transfer.rs index 024d45b..12d89cd 100644 --- a/rust/app/src/transfer.rs +++ b/rust/app/src/transfer.rs @@ -210,7 +210,12 @@ impl Transfer { .add_filter("JSON", &["json"]) .save_file() { - match s.svc.export_json(*include_secrets).map_err(|e| e.to_string()).and_then(|j| std::fs::write(&path, j).map_err(|e| e.to_string())) { + match s + .svc + .export_json(*include_secrets) + .map_err(|e| e.to_string()) + .and_then(|j| nexus_core::secrets::write_private(&path, j.as_bytes()).map_err(|e| e.to_string())) + { Ok(()) => { s.success(format!("Exported {} profile(s) to {}", s.profiles.len(), path.display())); close = true; diff --git a/rust/core/Cargo.toml b/rust/core/Cargo.toml index c95c29b..e5bfeab 100644 --- a/rust/core/Cargo.toml +++ b/rust/core/Cargo.toml @@ -21,6 +21,7 @@ tokio = { version = "1.53.2", features = ["full"] } sha2 = "0.11.0" hmac = "0.13.0" sha1 = "0.11.0" +zeroize = "1" portable-pty = "0.9.0" serialport = { version = "4", default-features = false } des = "0.9" diff --git a/rust/core/src/config.rs b/rust/core/src/config.rs index af90e2b..d71166f 100644 --- a/rust/core/src/config.rs +++ b/rust/core/src/config.rs @@ -51,7 +51,8 @@ impl AppConfig { } } -fn create_private_dir(d: &Path) -> std::io::Result<()> { +/// Creates `d` (and parents) readable only by the owner. +pub fn create_private_dir(d: &Path) -> std::io::Result<()> { std::fs::create_dir_all(d)?; #[cfg(unix)] std::fs::set_permissions(d, std::os::unix::fs::PermissionsExt::from_mode(0o700))?; diff --git a/rust/core/src/exportimport.rs b/rust/core/src/exportimport.rs index 774f09b..14f79b1 100644 --- a/rust/core/src/exportimport.rs +++ b/rust/core/src/exportimport.rs @@ -41,7 +41,7 @@ pub fn encrypt_and_export(profiles: &[Profile], password: &str, path: &Path) -> let salt = random_bytes::<32>(); let nonce = random_bytes::<12>(); let key = derive_key(password, &salt)?; - let ct = Aes256Gcm::new((&key).into()).encrypt(Nonce::from_slice(&nonce), plain.as_slice()).map_err(|e| e.to_string())?; + let ct = Aes256Gcm::new((&*key).into()).encrypt(Nonce::from_slice(&nonce), plain.as_slice()).map_err(|e| e.to_string())?; let mut out = vec![VERSION]; out.extend(salt); out.extend(nonce); @@ -64,7 +64,7 @@ pub fn decrypt_and_import(path: &Path, password: &str) -> Result> { return Err(format!("exportimport: unsupported format version {}", data[0]).into()); } let key = derive_key(password, &data[1..33])?; - let plain = Aes256Gcm::new((&key).into()) + let plain = Aes256Gcm::new((&*key).into()) .decrypt(Nonce::from_slice(&data[33..45]), &data[45..]) .map_err(|_| "exportimport: decryption failed — wrong password or corrupt file")?; let bundle: Bundle = serde_json::from_slice(&plain).map_err(|e| format!("exportimport: invalid bundle JSON: {e}"))?; diff --git a/rust/core/src/recording.rs b/rust/core/src/recording.rs index 905869a..32a93e1 100644 --- a/rust/core/src/recording.rs +++ b/rust/core/src/recording.rs @@ -116,7 +116,12 @@ impl Recorder { fn with_limit(dir: &Path, id: &str, title: &str, cols: i64, rows: i64, max_bytes: u64) -> Result { mkdir_private(dir).map_err(|e| format!("recording: failed to create directory: {e}"))?; let path = dir.join(format!("{id}.cast")); - let mut f = File::create(&path).map_err(|e| format!("recording: failed to create file: {e}"))?; + // Recordings capture typed input (passwords included): owner-only, like the vault. + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).create(true).truncate(true); + #[cfg(unix)] + std::os::unix::fs::OpenOptionsExt::mode(&mut opts, 0o600); + let mut f = opts.open(&path).map_err(|e| format!("recording: failed to create file: {e}"))?; let start = Instant::now(); let hdr = Header { version: 2, @@ -391,6 +396,11 @@ mod tests { assert_eq!(rec.event_count(), 4); rec.close().unwrap(); rec.close().unwrap(); // double close is a no-op + #[cfg(unix)] + { + let mode = std::os::unix::fs::PermissionsExt::mode(&std::fs::metadata(dir.join("test-session-001.cast")).unwrap().permissions()); + assert_eq!(mode & 0o777, 0o600); + } let text = std::fs::read_to_string(dir.join("test-session-001.cast")).unwrap(); let lines: Vec<&str> = text.trim().lines().collect(); assert_eq!(lines.len(), 5); diff --git a/rust/core/src/secrets.rs b/rust/core/src/secrets.rs index 5bbde05..67cd2fc 100644 --- a/rust/core/src/secrets.rs +++ b/rust/core/src/secrets.rs @@ -11,16 +11,32 @@ use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; use std::path::PathBuf; use std::sync::Mutex; +use zeroize::Zeroizing; const VAULT_FILE: &str = "vault.json"; const MAX_VAULT_BYTES: u64 = 10 * 1024 * 1024; -/// Argon2id with the Go parameters: t=3, m=128 MiB, p=4, 32-byte key. -pub fn derive_key(password: &str, salt: &[u8]) -> Result<[u8; 32]> { +/// Every credential kind the session manager stores under `profile::`. +pub const PROFILE_SECRET_KINDS: [&str; 11] = [ + "password", + "passphrase", + "totp", + "github-token", + "vnc-password", + "vnc-ssh-password", + "vnc-ssh-passphrase", + "irc-server-password", + "irc-nickserv-password", + "irc-ssh-password", + "irc-ssh-passphrase", +]; + +/// Argon2id with the Go parameters: t=3, m=128 MiB, p=4, 32-byte key. The key is wiped on drop. +pub fn derive_key(password: &str, salt: &[u8]) -> Result> { let params = argon2::Params::new(128 * 1024, 3, 4, Some(32)).map_err(|e| e.to_string())?; - let mut key = [0u8; 32]; + let mut key = Zeroizing::new([0u8; 32]); argon2::Argon2::new(argon2::Algorithm::Argon2id, argon2::Version::V0x13, params) - .hash_password_into(password.as_bytes(), salt, &mut key) + .hash_password_into(password.as_bytes(), salt, &mut *key) .map_err(|e| e.to_string())?; Ok(key) } @@ -54,16 +70,10 @@ struct VaultFile { } struct Unlocked { - key: [u8; 32], + key: Zeroizing<[u8; 32]>, vault: VaultFile, } -impl Drop for Unlocked { - fn drop(&mut self) { - self.key.fill(0); - } -} - pub struct Vault { dir: PathBuf, state: Mutex>, @@ -155,17 +165,27 @@ impl Vault { } } -/// Writes a file readable only by the owner. +/// Writes a file readable only by the owner, atomically: the data goes to a sibling temp file +/// that replaces `path` only once it is fully written and synced, so a crash can't leave a +/// truncated vault or export behind (and an existing file's wider mode is never kept). pub fn write_private(path: &std::path::Path, data: &[u8]) -> Result<()> { use std::io::Write; + let name = path.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default(); + let tmp = path.with_file_name(format!(".{name}.tmp-{}", std::process::id())); let mut opts = std::fs::OpenOptions::new(); opts.write(true).create(true).truncate(true); #[cfg(unix)] std::os::unix::fs::OpenOptionsExt::mode(&mut opts, 0o600); - let mut f = opts.open(path)?; - f.write_all(data)?; - f.sync_all()?; - Ok(()) + let written = (|| { + let mut f = opts.open(&tmp)?; + f.write_all(data)?; + f.sync_all()?; + std::fs::rename(&tmp, path) + })(); + if written.is_err() { + let _ = std::fs::remove_file(&tmp); + } + Ok(written?) } pub const KEYCHAIN_SERVICE: &str = "NexusTerminal"; @@ -270,7 +290,13 @@ impl Secrets { let _ = self.vault.delete(key); } + /// Removes every credential saved for a profile from both stores. pub fn delete_profile(&self, profile_id: &str) { + if self.keychain_ok { + for kind in PROFILE_SECRET_KINDS { + self.keychain.delete(&format!("profile:{profile_id}:{kind}")); + } + } let _ = self.vault.delete_prefix(&format!("profile:{profile_id}:")); } } @@ -303,4 +329,40 @@ mod tests { assert_eq!(v.get("a").unwrap(), "1"); std::fs::remove_dir_all(dir).unwrap(); } + + #[test] + fn private_writes_are_atomic_and_owner_only() { + let dir = std::env::temp_dir().join(format!("nexus-wp-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("secret.bin"); + // A pre-existing world-readable file is replaced, not reused. + std::fs::write(&path, b"old").unwrap(); + #[cfg(unix)] + std::fs::set_permissions(&path, std::os::unix::fs::PermissionsExt::from_mode(0o644)).unwrap(); + write_private(&path, b"new").unwrap(); + assert_eq!(std::fs::read(&path).unwrap(), b"new"); + #[cfg(unix)] + assert_eq!(std::os::unix::fs::PermissionsExt::mode(&std::fs::metadata(&path).unwrap().permissions()) & 0o777, 0o600); + // No temp file is left behind, on success or when the target directory is unwritable. + assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 1); + assert!(write_private(&dir.join("missing").join("x"), b"x").is_err()); + std::fs::remove_dir_all(dir).unwrap(); + } + + #[test] + fn deleting_a_profile_clears_every_vault_kind() { + let dir = std::env::temp_dir().join(format!("nexus-delprof-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let s = Secrets { vault: Vault::new(dir.clone()), keychain: Keychain::new("NexusTerminalTest", None), keychain_ok: false }; + s.vault.initialise("pw").unwrap(); + for kind in PROFILE_SECRET_KINDS { + s.vault.set(&format!("profile:p1:{kind}"), "v").unwrap(); + } + s.vault.set("profile:p2:password", "keep").unwrap(); + s.delete_profile("p1"); + assert!(PROFILE_SECRET_KINDS.iter().all(|k| s.get(&format!("profile:p1:{k}")).is_none())); + assert_eq!(s.get("profile:p2:password").as_deref(), Some("keep")); + std::fs::remove_dir_all(dir).unwrap(); + } }