From f22c331c2e06635cba10bec40089e07376e7385e Mon Sep 17 00:00:00 2001 From: grunch Date: Tue, 23 Jun 2026 11:36:26 -0300 Subject: [PATCH 1/2] fix(security): harden permissions for local mnemonic database The mnemonic stored in ~/.mcli/mcli.db derives the user's Mostro identity and trade keys. Until now both the directory and the database file inherited the process umask, so under a permissive umask they could be left world-readable, exposing the mnemonic to other local users. Additionally, `connect()` (which creates the DB and generates the mnemonic on first run) ran before required config was validated, so a command run with missing MOSTRO_PUBKEY/RELAYS could silently create the sensitive database before failing. Changes (issue #179): - Create ~/.mcli with mode 0700 and mcli.db with mode 0600. The DB file is created atomically via create_new + mode, avoiding any window where it exists world-readable. - Defensively re-tighten an existing directory/DB on every run, so installs created by older versions (or a loose umask) get hardened. - Validate MOSTRO_PUBKEY, RELAYS and (for admin commands) ADMIN_NSEC before calling connect(), so missing config fails without creating any mnemonic material on disk. - connect_nostr returns an error instead of panicking when RELAYS is unset. - Add regression tests for 0700/0600 permissions, idempotent/tightening directory creation, and create_new semantics. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XW5NGXu1jYKQGgr7xqMs8c --- src/cli.rs | 53 +++++++++++++++++-------- src/db.rs | 98 +++++++++++++++++++++++++++++++++++++++++++++- src/util/misc.rs | 56 +++++++++++++++++++++----- src/util/mod.rs | 2 +- src/util/net.rs | 2 +- tests/util_misc.rs | 61 ++++++++++++++++++++++++++++- 6 files changed, 242 insertions(+), 30 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 834401d..f309e0f 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -440,25 +440,37 @@ fn resolve_mostro_pubkey(cli: &Cli) -> Result { }) } +/// Ensure `RELAYS` is configured (and non-empty) before we create any local +/// state. `connect_nostr` reads this variable; validating it here means a +/// command run without relays fails before the mnemonic database is created. +/// `get_env_var` has already mirrored `--relays` into the env var by this point. +fn resolve_relays() -> Result { + match std::env::var("RELAYS") { + Ok(relays) if !relays.trim().is_empty() => Ok(relays), + _ => Err(anyhow::anyhow!( + "RELAYS not set.\n\ + Provide it using one of the following methods:\n\ + 1) --relays \n\ + 2) export RELAYS=" + )), + } +} + async fn init_context(cli: &Cli) -> Result { // Get environment variables get_env_var(cli); - // Initialize database pool - let pool = connect().await?; - - // Get identity keys - let identity_keys = User::get_identity_keys(&pool) - .await - .map_err(|e| anyhow::anyhow!("Failed to get identity keys: {}", e))?; - - // Get trade keys - let (trade_keys, trade_index) = User::get_next_trade_keys(&pool) - .await - .map_err(|e| anyhow::anyhow!("Failed to get trade keys: {}", e))?; + // Validate all required configuration *before* touching the local database. + // `connect()` creates `~/.mcli/mcli.db` and generates the mnemonic on first + // run, so a mistaken command with missing config must fail here, without + // ever writing that sensitive material to disk. See issue #179. + let mostro_pubkey = PublicKey::from_str(&resolve_mostro_pubkey(cli)?)?; + resolve_relays()?; - // Load private key of admin - only required for admin commands - // For regular user commands, this will be None + // Load private key of admin - only required for admin commands. + // For regular user commands, this will be None. Validated up front so an + // admin command with a missing/invalid ADMIN_NSEC also fails before the + // database is created. let context_keys = if is_admin_command(&cli.command) { Some( std::env::var("ADMIN_NSEC") @@ -472,9 +484,18 @@ async fn init_context(cli: &Cli) -> Result { None }; - // Resolve Mostro pubkey from env (required for all flows) + // Initialize database pool (creates the DB and mnemonic on first run) + let pool = connect().await?; - let mostro_pubkey = PublicKey::from_str(&resolve_mostro_pubkey(cli)?)?; + // Get identity keys + let identity_keys = User::get_identity_keys(&pool) + .await + .map_err(|e| anyhow::anyhow!("Failed to get identity keys: {}", e))?; + + // Get trade keys + let (trade_keys, trade_index) = User::get_next_trade_keys(&pool) + .await + .map_err(|e| anyhow::anyhow!("Failed to get trade keys: {}", e))?; // Connect to Nostr relays let client = util::connect_nostr().await?; diff --git a/src/db.rs b/src/db.rs index d7a3725..18b31f6 100644 --- a/src/db.rs +++ b/src/db.rs @@ -6,7 +6,7 @@ use nostr_sdk::prelude::*; use sqlx::pool::Pool; use sqlx::Sqlite; use sqlx::SqlitePool; -use std::fs::File; +use std::io; use std::path::Path; pub async fn connect() -> Result> { @@ -15,7 +15,11 @@ pub async fn connect() -> Result> { let db_url = format!("sqlite://{}", mcli_db_path); let pool: Pool; if !Path::exists(Path::new(&mcli_db_path)) { - if let Err(res) = File::create(&mcli_db_path) { + // Create the database file with owner-only permissions (0600 on Unix) + // *before* writing anything to it. This file stores the mnemonic that + // derives the user's identity and trade keys, so it must not be readable + // by other local users under a permissive umask. See issue #179. + if let Err(res) = create_private_db_file(&mcli_db_path) { println!("Error in creating db file: {}", res); return Err(res.into()); } @@ -63,6 +67,17 @@ pub async fn connect() -> Result> { let user = User::new(mnemonic, &pool).await?; println!("User created with pubkey: {}", user.i0_pubkey); } else { + // Defensively re-tighten the permissions of a database that an older + // version (or a permissive umask) may have left world-readable, so + // existing installs get hardened on the next run too. See issue #179. + #[cfg(unix)] + if let Err(e) = crate::util::misc::set_mode(&mcli_db_path, 0o600) { + eprintln!( + "Warning: could not restrict permissions on {}: {}", + mcli_db_path, e + ); + } + pool = SqlitePool::connect(&db_url).await?; // Migration: Drop buyer_token and seller_token columns if they exist @@ -72,6 +87,30 @@ pub async fn connect() -> Result> { Ok(pool) } +/// Create the SQLite database file restricted to the owner (mode `0600` on +/// Unix). On non-Unix platforms this falls back to a plain create. +/// +/// Uses `create_new` so the file is created atomically with the right +/// permissions instead of being created world-readable and tightened +/// afterwards, which would leave a brief window where another local user could +/// open it. +fn create_private_db_file(path: &str) -> io::Result<()> { + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(path) + .map(|_| ()) + } + #[cfg(not(unix))] + { + std::fs::File::create(path).map(|_| ()) + } +} + async fn migrate_remove_token_columns(pool: &SqlitePool) -> Result<()> { println!("Checking for legacy token columns..."); @@ -577,3 +616,58 @@ impl Order { Ok(rows_affected > 0) } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Unique, non-existent path inside the OS temp dir (no extra crates, no + /// forbidden `Date`/random APIs). + fn unique_temp_path(label: &str) -> std::path::PathBuf { + use std::sync::atomic::{AtomicU64, Ordering}; + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + std::env::temp_dir().join(format!( + "mcli-db-test-{}-{}-{}", + label, + std::process::id(), + n + )) + } + + #[cfg(unix)] + #[test] + fn test_create_private_db_file_is_0600() { + use std::os::unix::fs::PermissionsExt; + + let path = unique_temp_path("perms"); + let path_str = path.to_str().unwrap(); + + create_private_db_file(path_str).expect("should create db file"); + + assert!(path.is_file(), "db file should exist"); + let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode, 0o600, + "mnemonic db must be readable only by the owner" + ); + + std::fs::remove_file(&path).ok(); + } + + #[test] + fn test_create_private_db_file_fails_if_exists() { + let path = unique_temp_path("exists"); + let path_str = path.to_str().unwrap(); + + create_private_db_file(path_str).expect("first create should succeed"); + // `create_new` semantics: a second create on the same path must error + // rather than truncate an existing (possibly populated) database. + assert!( + create_private_db_file(path_str).is_err(), + "creating over an existing db file must fail" + ); + + std::fs::remove_file(&path).ok(); + } +} diff --git a/src/util/misc.rs b/src/util/misc.rs index efcf073..182581a 100644 --- a/src/util/misc.rs +++ b/src/util/misc.rs @@ -1,4 +1,4 @@ -use std::{fs, path::Path}; +use std::io; pub fn uppercase_first(s: &str) -> String { let mut c = s.chars(); @@ -11,14 +11,52 @@ pub fn uppercase_first(s: &str) -> String { pub fn get_mcli_path() -> String { let home_dir = dirs::home_dir().expect("Couldn't get home directory"); let mcli_path = format!("{}/.mcli", home_dir.display()); - if !Path::new(&mcli_path).exists() { - match fs::create_dir(&mcli_path) { - Ok(_) => println!("Directory {} created.", mcli_path), - Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { - // Directory was created by another thread/process, which is fine - } - Err(e) => panic!("Couldn't create mostro-cli directory in HOME: {}", e), - } + if let Err(e) = ensure_private_dir(&mcli_path) { + panic!("Couldn't create mostro-cli directory in HOME: {}", e); } mcli_path } + +/// Ensure `path` exists as a directory that is private to the current user. +/// +/// The `.mcli` directory holds `mcli.db`, which in turn stores the mnemonic that +/// derives the user's Mostro identity and trade keys. To avoid that secret being +/// exposed to other local users under a permissive `umask`, the directory is +/// created with mode `0700` on Unix. We also tighten the permissions of a +/// pre-existing directory, so users who created `~/.mcli` with an older version +/// (or a loose `umask`) get hardened on the next run. See issue #179. +pub fn ensure_private_dir(path: &str) -> io::Result<()> { + match create_dir_private(path) { + Ok(()) => {} + // Another thread/process won the race, or the directory already existed. + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => {} + Err(e) => return Err(e), + } + + #[cfg(unix)] + set_mode(path, 0o700)?; + + Ok(()) +} + +/// Create a directory restricted to the owner (mode `0700` on Unix). +/// +/// Returns an `AlreadyExists` error when the directory is already present, so +/// callers can treat that case as success. +#[cfg(unix)] +fn create_dir_private(path: &str) -> io::Result<()> { + use std::os::unix::fs::DirBuilderExt; + std::fs::DirBuilder::new().mode(0o700).create(path) +} + +#[cfg(not(unix))] +fn create_dir_private(path: &str) -> io::Result<()> { + std::fs::create_dir(path) +} + +/// Set the Unix permission bits of `path` to `mode`. +#[cfg(unix)] +pub fn set_mode(path: &str, mode: u32) -> io::Result<()> { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(mode)) +} diff --git a/src/util/mod.rs b/src/util/mod.rs index 7d36c8b..043076e 100644 --- a/src/util/mod.rs +++ b/src/util/mod.rs @@ -15,7 +15,7 @@ pub use messaging::{ send_admin_chat_message_via_shared_key, send_dm, send_plain_text_dm, wait_for_dm, PowRequirementUnmet, WaitForDmTimeout, }; -pub use misc::{get_mcli_path, uppercase_first}; +pub use misc::{ensure_private_dir, get_mcli_path, uppercase_first}; pub use net::connect_nostr; pub use storage::{admin_send_dm, run_simple_order_msg, save_order}; pub use types::{Event, ListKind}; diff --git a/src/util/net.rs b/src/util/net.rs index 4d46eda..57f1cc5 100644 --- a/src/util/net.rs +++ b/src/util/net.rs @@ -14,7 +14,7 @@ const CONNECT_TIMEOUT: Duration = Duration::from_secs(10); pub async fn connect_nostr() -> Result { let my_keys = Keys::generate(); - let relays = var("RELAYS").expect("RELAYS is not set"); + let relays = var("RELAYS").map_err(|_| anyhow::anyhow!("RELAYS is not set"))?; let relays = relays.split(',').collect::>(); let client = Client::new(my_keys); for r in relays.into_iter() { diff --git a/tests/util_misc.rs b/tests/util_misc.rs index 85f48c8..4c85d01 100644 --- a/tests/util_misc.rs +++ b/tests/util_misc.rs @@ -1,4 +1,4 @@ -use mostro_client::util::misc::{get_mcli_path, uppercase_first}; +use mostro_client::util::misc::{ensure_private_dir, get_mcli_path, uppercase_first}; #[test] fn test_uppercase_first_empty_string() { @@ -86,3 +86,62 @@ fn test_get_mcli_path_consistent() { // Should return the same path on multiple calls assert_eq!(path1, path2); } + +/// Build a unique, non-existent path inside the OS temp dir without pulling in +/// extra crates or the forbidden `Date`/random APIs. +fn unique_temp_path(label: &str) -> std::path::PathBuf { + use std::sync::atomic::{AtomicU64, Ordering}; + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + std::env::temp_dir().join(format!("mcli-test-{}-{}-{}", label, std::process::id(), n)) +} + +#[cfg(unix)] +#[test] +fn test_ensure_private_dir_creates_with_0700() { + use std::os::unix::fs::PermissionsExt; + + let dir = unique_temp_path("dir"); + let dir_str = dir.to_str().unwrap(); + + ensure_private_dir(dir_str).expect("should create directory"); + + assert!(dir.is_dir(), "directory should exist"); + let mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o700, "directory must be private to the owner"); + + std::fs::remove_dir_all(&dir).ok(); +} + +#[cfg(unix)] +#[test] +fn test_ensure_private_dir_tightens_existing_dir() { + use std::os::unix::fs::PermissionsExt; + + let dir = unique_temp_path("loose"); + let dir_str = dir.to_str().unwrap(); + + // Simulate a directory left world-readable by a permissive umask. + std::fs::create_dir(&dir).unwrap(); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + + ensure_private_dir(dir_str).expect("should tighten existing directory"); + + let mode = std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o700, "existing directory must be tightened to 0700"); + + std::fs::remove_dir_all(&dir).ok(); +} + +#[test] +fn test_ensure_private_dir_is_idempotent() { + let dir = unique_temp_path("idem"); + let dir_str = dir.to_str().unwrap(); + + ensure_private_dir(dir_str).expect("first call should succeed"); + ensure_private_dir(dir_str).expect("second call should succeed"); + + assert!(dir.is_dir()); + + std::fs::remove_dir_all(&dir).ok(); +} From 100b8f977a98ff61699742eb5cb95c5848f0b527 Mon Sep 17 00:00:00 2001 From: grunch Date: Tue, 23 Jun 2026 12:39:50 -0300 Subject: [PATCH 2/2] fix(security): address PR review feedback - db.rs: use create_new on all platforms in create_private_db_file so the non-Unix fallback no longer truncates an existing database; only the 0600 mode bit stays Unix-gated. - db.rs: fail closed when an existing mnemonic DB can't be re-tightened to 0600 instead of only warning, so we never keep using a world-readable DB. - misc.rs: ensure_private_dir now verifies the pre-existing path is a directory before chmod-ing it and reporting success. - net.rs: trim relay entries and drop empty ones so stray whitespace or trailing commas don't reach add_relay. - tests: cover ensure_private_dir rejecting a non-directory path. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01XW5NGXu1jYKQGgr7xqMs8c --- src/db.rs | 36 +++++++++++++++++++----------------- src/util/misc.rs | 12 +++++++++++- src/util/net.rs | 11 ++++++++++- tests/util_misc.rs | 17 +++++++++++++++++ 4 files changed, 57 insertions(+), 19 deletions(-) diff --git a/src/db.rs b/src/db.rs index 18b31f6..3754597 100644 --- a/src/db.rs +++ b/src/db.rs @@ -69,14 +69,17 @@ pub async fn connect() -> Result> { } else { // Defensively re-tighten the permissions of a database that an older // version (or a permissive umask) may have left world-readable, so - // existing installs get hardened on the next run too. See issue #179. + // existing installs get hardened on the next run too. Fail closed: if we + // can't make the mnemonic DB owner-only, refuse to open it rather than + // keep using a potentially group/world-readable file. See issue #179. #[cfg(unix)] - if let Err(e) = crate::util::misc::set_mode(&mcli_db_path, 0o600) { - eprintln!( - "Warning: could not restrict permissions on {}: {}", - mcli_db_path, e - ); - } + crate::util::misc::set_mode(&mcli_db_path, 0o600).map_err(|e| { + anyhow::anyhow!( + "could not restrict permissions on {}; refusing to open mnemonic DB: {}", + mcli_db_path, + e + ) + })?; pool = SqlitePool::connect(&db_url).await?; @@ -95,20 +98,19 @@ pub async fn connect() -> Result> { /// afterwards, which would leave a brief window where another local user could /// open it. fn create_private_db_file(path: &str) -> io::Result<()> { + let mut options = std::fs::OpenOptions::new(); + options.write(true).create_new(true); + + // The owner-only mode bit is Unix-specific; `create_new` (fail on existing) + // applies on every platform so a second create never truncates an existing + // database. #[cfg(unix)] { use std::os::unix::fs::OpenOptionsExt; - std::fs::OpenOptions::new() - .write(true) - .create_new(true) - .mode(0o600) - .open(path) - .map(|_| ()) - } - #[cfg(not(unix))] - { - std::fs::File::create(path).map(|_| ()) + options.mode(0o600); } + + options.open(path).map(|_| ()) } async fn migrate_remove_token_columns(pool: &SqlitePool) -> Result<()> { diff --git a/src/util/misc.rs b/src/util/misc.rs index 182581a..ac1809c 100644 --- a/src/util/misc.rs +++ b/src/util/misc.rs @@ -28,11 +28,21 @@ pub fn get_mcli_path() -> String { pub fn ensure_private_dir(path: &str) -> io::Result<()> { match create_dir_private(path) { Ok(()) => {} - // Another thread/process won the race, or the directory already existed. + // Another thread/process won the race, or the path already existed. Err(e) if e.kind() == io::ErrorKind::AlreadyExists => {} Err(e) => return Err(e), } + // `AlreadyExists` only tells us *something* is there. If it's a regular file + // (or anything other than a directory) we must not chmod it and claim + // success — callers were promised a private directory. + if !std::fs::metadata(path)?.is_dir() { + return Err(io::Error::new( + io::ErrorKind::AlreadyExists, + format!("{} exists but is not a directory", path), + )); + } + #[cfg(unix)] set_mode(path, 0o700)?; diff --git a/src/util/net.rs b/src/util/net.rs index 57f1cc5..1a22a4a 100644 --- a/src/util/net.rs +++ b/src/util/net.rs @@ -15,7 +15,16 @@ pub async fn connect_nostr() -> Result { let my_keys = Keys::generate(); let relays = var("RELAYS").map_err(|_| anyhow::anyhow!("RELAYS is not set"))?; - let relays = relays.split(',').collect::>(); + // Trim each entry and drop empty ones so stray whitespace or trailing commas + // (e.g. `RELAYS="wss://a, ,wss://b,"`) don't reach `add_relay` and fail. + let relays = relays + .split(',') + .map(str::trim) + .filter(|r| !r.is_empty()) + .collect::>(); + if relays.is_empty() { + return Err(anyhow::anyhow!("RELAYS is not set")); + } let client = Client::new(my_keys); for r in relays.into_iter() { client.add_relay(r).await?; diff --git a/tests/util_misc.rs b/tests/util_misc.rs index 4c85d01..ddae008 100644 --- a/tests/util_misc.rs +++ b/tests/util_misc.rs @@ -145,3 +145,20 @@ fn test_ensure_private_dir_is_idempotent() { std::fs::remove_dir_all(&dir).ok(); } + +#[test] +fn test_ensure_private_dir_rejects_non_directory() { + let path = unique_temp_path("file"); + let path_str = path.to_str().unwrap(); + + // A regular file already occupies the path: we must not chmod it and report + // success, since callers were promised a directory. + std::fs::write(&path, b"not a dir").unwrap(); + + assert!( + ensure_private_dir(path_str).is_err(), + "must fail when the path exists but is not a directory" + ); + + std::fs::remove_file(&path).ok(); +}