fix(storage): transactional migrations, purge legacy plaintext secrets, private DB files, stricter ids and hosts
- Each migration runs in a transaction. Several rebuild the profiles
table (create, copy, DROP, rename); a crash between the DROP and the
rename left a database that failed to open on every later launch.
- Migration 009 removes the Go-era proxyPassword/totpSecret keys still
sitting in old ssh_config JSON, then VACUUMs and checkpoints the WAL so
the old pages leave the file. secure_delete is on from now on.
- nexus.db (and -wal/-shm) are created and kept 0600.
- Profile ids are limited to [A-Za-z0-9._-]{1,64}: ids name keychain and
vault entries (profile:<id>:<kind>), so a crafted id containing ':'
could shadow or purge another profile's credentials on import/sync.
- Hosts reject control characters (NUL got through) and >253 bytes; jump
hosts and forward destination/bind addresses get the same host check.
This commit is contained in:
@@ -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'));
|
||||
@@ -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<Snippet> {
|
||||
|
||||
impl Store {
|
||||
pub fn open(path: &Path) -> Result<Store> {
|
||||
// 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()));
|
||||
|
||||
@@ -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:<id>:<kind>`): 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());
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user