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..3754597 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,20 @@ 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. 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)] + 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?; // Migration: Drop buyer_token and seller_token columns if they exist @@ -72,6 +90,29 @@ 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<()> { + 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; + options.mode(0o600); + } + + options.open(path).map(|_| ()) +} + async fn migrate_remove_token_columns(pool: &SqlitePool) -> Result<()> { println!("Checking for legacy token columns..."); @@ -577,3 +618,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..ac1809c 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,62 @@ 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 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)?; + + 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..1a22a4a 100644 --- a/src/util/net.rs +++ b/src/util/net.rs @@ -14,8 +14,17 @@ 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 = relays.split(',').collect::>(); + let relays = var("RELAYS").map_err(|_| anyhow::anyhow!("RELAYS is not set"))?; + // 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 85f48c8..ddae008 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,79 @@ 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(); +} + +#[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(); +}