diff --git a/rust/core/migrations/009_purge_legacy_secrets.sql b/rust/core/migrations/009_purge_legacy_secrets.sql new file mode 100644 index 0000000..4cd81f9 --- /dev/null +++ b/rust/core/migrations/009_purge_legacy_secrets.sql @@ -0,0 +1,10 @@ +-- Profiles saved by the Go app kept proxyPassword and totpSecret inside ssh_config. They live in +-- the keychain or vault now and are dropped on read, but the old JSON stayed in the column until +-- the profile was next saved. Remove them (the store also VACUUMs after this migration so the +-- old pages don't linger in the file). +UPDATE profiles +SET ssh_config = json_remove(ssh_config, '$.proxyPassword', '$.totpSecret') +WHERE json_valid(ssh_config) + AND (json_extract(ssh_config, '$.proxyPassword') IS NOT NULL OR json_extract(ssh_config, '$.totpSecret') IS NOT NULL); + +INSERT INTO schema_version (version, applied_at) VALUES (9, datetime('now')); diff --git a/rust/core/src/storage.rs b/rust/core/src/storage.rs index 0662dc5..406b9c5 100644 --- a/rust/core/src/storage.rs +++ b/rust/core/src/storage.rs @@ -17,6 +17,7 @@ const MIGRATIONS: &[(i64, &str)] = &[ (6, include_str!("../migrations/006_irc_channel_state.sql")), (7, include_str!("../migrations/007_serial_protocol.sql")), (8, include_str!("../migrations/008_rdp_protocol.sql")), + (9, include_str!("../migrations/009_purge_legacy_secrets.sql")), ]; const PROFILE_COLS: &str = "id, display_name, description, protocol, host, port, username, @@ -118,23 +119,50 @@ fn scan_snippet(r: &Row) -> rusqlite::Result { impl Store { pub fn open(path: &Path) -> Result { + // Owner-only from the first byte (the data folder is 0700 too, but the file may be copied + // or backed up on its own). + #[cfg(unix)] + if !path.exists() { + use std::os::unix::fs::OpenOptionsExt; + std::fs::OpenOptions::new().write(true).create_new(true).mode(0o600).open(path).map_err(|e| format!("storage: failed to create database: {e}"))?; + } let db = Connection::open(path).map_err(|e| format!("storage: failed to open database: {e}"))?; db.busy_timeout(std::time::Duration::from_secs(5))?; db.pragma_update(None, "journal_mode", "WAL")?; + // Deleted rows (edited profiles, removed audit entries) are zeroed rather than left in + // free pages. + db.pragma_update(None, "secure_delete", "ON")?; + #[cfg(unix)] + for suffix in ["", "-wal", "-shm"] { + let mut p = path.as_os_str().to_owned(); + p.push(suffix); + let _ = std::fs::set_permissions(&p, std::os::unix::fs::PermissionsExt::from_mode(0o600)); + } let s = Store { db: Mutex::new(db) }; s.migrate().map_err(|e| format!("storage: migration failed: {e}"))?; Ok(s) } + /// Applies pending migrations, each in its own transaction: several rebuild a table + /// (create new, copy, DROP old, rename), and a crash between the DROP and the rename used to + /// leave a database that failed to open on every later launch. fn migrate(&self) -> Result<()> { - let db = self.db.lock(); + let mut db = self.db.lock(); db.execute_batch("CREATE TABLE IF NOT EXISTS schema_version (version INTEGER PRIMARY KEY, applied_at TEXT NOT NULL)")?; let current: i64 = db.query_row("SELECT COALESCE(MAX(version), 0) FROM schema_version", [], |r| r.get(0))?; for (ver, sql) in MIGRATIONS { if *ver > current { - db.execute_batch(sql).map_err(|e| format!("failed to apply migration {ver:03}: {e}"))?; + let tx = db.transaction()?; + tx.execute_batch(sql).map_err(|e| format!("failed to apply migration {ver:03}: {e}"))?; + tx.commit()?; } } + if current < 9 && current > 0 { + // Migration 009 removed plaintext secrets; rewrite the file so their pages go too. + // In WAL mode the rewrite lands in the WAL; checkpoint it into the main file and + // truncate the WAL so neither keeps the old pages. + let _ = db.execute_batch("VACUUM; PRAGMA wal_checkpoint(TRUNCATE);"); + } Ok(()) } @@ -561,6 +589,51 @@ mod tests { } } + #[test] + fn a_failed_migration_rolls_back_and_legacy_secrets_are_purged() { + let tmp = std::env::temp_dir().join(format!("nexus-mig-{}.db", std::process::id())); + let _ = std::fs::remove_file(&tmp); + let s = Store::open(&tmp).unwrap(); + let mut p = Profile::new(crate::models::Protocol::Ssh); + p.id = "legacy".into(); + p.display_name = "old".into(); + p.host = "h".into(); + s.create_profile(&p).unwrap(); + { + // Put the database back at version 8 with a Go-era ssh_config carrying secrets. + let db = s.db.lock(); + db.execute_batch( + "UPDATE profiles SET ssh_config = json_set(ssh_config, '$.proxyPassword', 'hunter2', '$.totpSecret', 'JBSWY3DP') WHERE id = 'legacy'; + DELETE FROM schema_version WHERE version = 9;", + ) + .unwrap(); + } + drop(s); + let s = Store::open(&tmp).unwrap(); + let raw: String = s.db.lock().query_row("SELECT ssh_config FROM profiles WHERE id = 'legacy'", [], |r| r.get(0)).unwrap(); + assert!(!raw.contains("hunter2") && !raw.contains("JBSWY3DP"), "{raw}"); + for suffix in ["", "-wal"] { + let bytes = std::fs::read(format!("{}{suffix}", tmp.display())).unwrap_or_default(); + assert!(!String::from_utf8_lossy(&bytes).contains("hunter2"), "old secret left in nexus.db{suffix}"); + } + // A migration that fails part-way leaves nothing behind. + let mut db = s.db.lock(); + let tx = db.transaction().unwrap(); + assert!(tx.execute_batch("CREATE TABLE half (x); SELECT * FROM no_such_table;").is_err()); + drop(tx); + assert!(db.prepare("SELECT * FROM half").is_err(), "rolled back"); + drop(db); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + assert_eq!(std::fs::metadata(&tmp).unwrap().permissions().mode() & 0o777, 0o600); + } + drop(s); + for suffix in ["", "-wal", "-shm"] { + let _ = std::fs::remove_file(format!("{}{suffix}", tmp.display())); + } + } + #[test] fn known_hosts_are_looked_up_per_key_type() { let tmp = std::env::temp_dir().join(format!("nexus-kh-{}.db", std::process::id())); diff --git a/rust/core/src/validation.rs b/rust/core/src/validation.rs index 3178ef5..77f2171 100644 --- a/rust/core/src/validation.rs +++ b/rust/core/src/validation.rs @@ -60,6 +60,10 @@ pub fn validate_profile(p: &Profile) -> Result<(), ValidationErrors> { let mut errs = ValidationErrors::default(); if p.id.trim().is_empty() { errs.add("id", "profile ID must not be empty"); + } else if p.id.len() > 64 || !p.id.chars().all(|c| c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.')) { + // Ids name keychain/vault entries (`profile::`): a crafted id containing `:` + // could shadow or purge another profile's credentials. + errs.add("id", "profile ID may only contain letters, digits, '-', '_' and '.'"); } if p.display_name.trim().is_empty() { errs.add("displayName", "display name must not be empty"); @@ -155,6 +159,12 @@ pub fn validate_host(host: &str) -> Result<(), ValidationError> { if h.contains([' ', '\t', '\n', '\r']) { return err("host", "host must not contain whitespace"); } + if h.chars().any(is_control) { + return err("host", "host must not contain control characters"); + } + if h.len() > 253 { + return err("host", "host is too long"); + } if h.contains("://") { return err("host", "host must not include a URI scheme"); } @@ -206,8 +216,8 @@ pub fn validate_ssh_config(cfg: &SshConfig) -> Result<(), ValidationErrors> { } for (i, jh) in cfg.jump_hosts.iter().enumerate() { let prefix = format!("ssh.jumpHosts[{i}]"); - if jh.host.trim().is_empty() { - errs.add(format!("{prefix}.host"), "jump host address must not be empty"); + if let Err(e) = validate_host(&jh.host) { + errs.add(format!("{prefix}.host"), format!("jump host: {}", e.message)); } if !(1..=65535).contains(&jh.port) { errs.add(format!("{prefix}.port"), "jump host port must be between 1 and 65535"); @@ -224,8 +234,13 @@ pub fn validate_ssh_config(cfg: &SshConfig) -> Result<(), ValidationErrors> { if !(1..=65535).contains(&pf.dest_port) { errs.add(format!("{prefix}.destPort"), "destination port must be between 1 and 65535"); } - if pf.dest_host.trim().is_empty() { - errs.add(format!("{prefix}.destHost"), "destination host must not be empty"); + if let Err(e) = validate_host(&pf.dest_host) { + errs.add(format!("{prefix}.destHost"), format!("destination: {}", e.message)); + } + if !pf.bind_host.trim().is_empty() + && let Err(e) = validate_host(&pf.bind_host) + { + errs.add(format!("{prefix}.bindHost"), format!("bind address: {}", e.message)); } } errs.into_result() @@ -626,4 +641,21 @@ mod tests { assert!(validate_irc_config(&c).is_err(), "{rooms:?}"); } } + + #[test] + fn hosts_and_ids_reject_control_characters_and_separators() { + assert!(validate_host("a\0b").is_err() && validate_host("a\x1bb").is_err()); + assert!(validate_host(&"a".repeat(254)).is_err() && validate_host("db.example.com").is_ok() && validate_host("::1").is_ok()); + let mut p = Profile::new(Protocol::Ssh); + p.display_name = "x".into(); + p.host = "h".into(); + p.username = "u".into(); + assert!(validate_profile(&p).is_ok(), "a generated id is valid"); + for bad in ["victim:password", "../x", "a b", &"a".repeat(65)] { + p.id = bad.into(); + assert!(validate_profile(&p).is_err(), "{bad:?}"); + } + p.id = "rdp-test_1.a".into(); + assert!(validate_profile(&p).is_ok()); + } }