From: Vihiga Tyonum Date: Tue, 22 Sep 2026 09:06:03 +0000 (+0100) Subject: fix: harden datadir and config file permissions X-Git-Url: http://internal-gitweb-vhost/tx_graph/%22https:/encode/script/struct.Script.html?a=commitdiff_plain;h=b2b99b4297a955c4ff5aee59475fc4e64e6f2868;p=bdk-cli fix: harden datadir and config file permissions - Replaces `fs::create_dir_all` with `DirBuilder` enforcing 0700 on Unix. - Replaces `fs::write` with `OpenOptions` enforcing 0600 on Unix when writing `config.toml`. - Adds permission hardening on startup for existing configurations. - Add tests for permissions --- diff --git a/CHANGELOG.md b/CHANGELOG.md index 098eee1..ec50ea1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ page. See [DEVELOPMENT_CYCLE.md](DEVELOPMENT_CYCLE.md) for more details. ## [Unreleased] - Added support for Multipath (two-paths) descriptors. - +- Fixed the data directory and config.toml permission being world-readable (0755/0644) to 0700/0600 on Unix. ## [4.0.0] diff --git a/src/config.rs b/src/config.rs index 6058003..e905c42 100644 --- a/src/config.rs +++ b/src/config.rs @@ -9,6 +9,7 @@ use crate::commands::WalletOpts; use crate::error::BDKCliError as Error; #[cfg(feature = "sqlite")] use crate::persister::DatabaseType; +use crate::utils::{FILE_MODE, create_restricted_dir, limit_access, write_file_content}; use bdk_wallet::bitcoin::Network; #[cfg(any(feature = "sqlite", feature = "redb"))] use clap::ValueEnum; @@ -74,6 +75,9 @@ impl WalletConfig { if !config_path.exists() { return Ok(None); } + if let Err(e) = limit_access(&config_path, FILE_MODE) { + eprintln!("WARNING: could not restrict {config_path:?}: {e}\n"); + } let config_content = fs::read_to_string(&config_path) .map_err(|e| Error::Generic(format!("Failed to read config file: {e}")))?; let config: WalletConfig = toml::from_str(&config_content) @@ -86,9 +90,10 @@ impl WalletConfig { let config_path = datadir.join("config.toml"); let config_content = toml::to_string_pretty(self) .map_err(|e| Error::Generic(format!("Failed to serialize config: {e}")))?; - fs::create_dir_all(datadir) + create_restricted_dir(datadir) .map_err(|e| Error::Generic(format!("Failed to create directory {datadir:?}: {e}")))?; - fs::write(&config_path, config_content).map_err(|e| { + // The config can hold secret descriptors, so it must never be left readable by other users. + write_file_content(&config_path, &config_content).map_err(|e| { Error::Generic(format!("Failed to write config file {config_path:?}: {e}")) })?; log::debug!("Saved config to {config_path:?}"); @@ -346,4 +351,43 @@ mod tests { let result: Result = (&inner).try_into(); assert!(result.is_err()); } + + #[cfg(unix)] + #[test] + fn test_config_is_unreadable_by_other_users() { + use std::os::unix::fs::PermissionsExt; + use tempfile::TempDir; + + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("config.toml"); + + WalletConfig { + wallets: HashMap::new(), + } + .save(temp_dir.path()) + .unwrap(); + + let mode = fs::metadata(&config_path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600); + } + + #[cfg(unix)] + #[test] + fn test_default_permissions_restricted_on_load_and_save() { + use std::os::unix::fs::PermissionsExt; + use tempfile::TempDir; + + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("config.toml"); + let mode = |path: &Path| fs::metadata(path).unwrap().permissions().mode() & 0o777; + fs::write(&config_path, "[wallets]\n").unwrap(); + fs::set_permissions(&config_path, fs::Permissions::from_mode(0o644)).unwrap(); + + let config = WalletConfig::load(temp_dir.path()).unwrap().unwrap(); + assert_eq!(mode(&config_path), 0o600); + + fs::set_permissions(&config_path, fs::Permissions::from_mode(0o644)).unwrap(); + config.save(temp_dir.path()).unwrap(); + assert_eq!(mode(&config_path), 0o600); + } } diff --git a/src/handlers/payjoin/db.rs b/src/handlers/payjoin/db.rs index 1b75f12..f28ffc4 100644 --- a/src/handlers/payjoin/db.rs +++ b/src/handlers/payjoin/db.rs @@ -11,7 +11,7 @@ use payjoin::receive::v2::SessionEvent as ReceiverSessionEvent; use payjoin::send::v2::SessionEvent as SenderSessionEvent; use crate::error::BDKCliError; -use crate::utils::prepare_home_dir; +use crate::utils::{create_restricted_dir, prepare_home_dir}; pub type Result = std::result::Result; @@ -67,7 +67,7 @@ pub fn open_payjoin_db( wallet_name: &str, ) -> std::result::Result, BDKCliError> { let wallet_dir = prepare_home_dir(datadir)?.join(wallet_name); - std::fs::create_dir_all(&wallet_dir).map_err(|e| BDKCliError::Generic(e.to_string()))?; + create_restricted_dir(&wallet_dir).map_err(|e| BDKCliError::Generic(e.to_string()))?; let db = Arc::new(Database::create(wallet_dir.join(DB_FILENAME))?); db.prune_expired_sessions()?; Ok(db) diff --git a/src/utils/common.rs b/src/utils/common.rs index 136c987..5102c03 100644 --- a/src/utils/common.rs +++ b/src/utils/common.rs @@ -20,6 +20,12 @@ use std::{ str::FromStr, }; +/// Limit directory permissions of reading, writing, opening and listing to only owner. +pub(crate) const DIR_MODE: u32 = 0o700; + +/// Only owner has full read and write access. +pub(crate) const FILE_MODE: u32 = 0o600; + /// Determine if PSBT has final script sigs or witnesses for all unsigned tx inputs. #[cfg(any( feature = "electrum", @@ -106,7 +112,9 @@ pub(crate) fn prepare_home_dir(home_path: Option) -> Result Result<(String, u64), String> { let sending_amount = u64::from_str(parts[1]).map_err(|e| e.to_string())?; Ok((parts[0].to_string(), sending_amount)) } + +/// Create `dir` and limit access to only owner. +#[cfg(unix)] +pub(crate) fn create_restricted_dir(dir: &Path) -> std::io::Result<()> { + use std::os::unix::fs::DirBuilderExt; + + std::fs::DirBuilder::new() + .recursive(true) + .mode(DIR_MODE) + .create(dir) +} + +#[cfg(not(unix))] +pub(crate) fn create_restricted_dir(dir: &Path) -> std::io::Result<()> { + std::fs::create_dir_all(dir) +} + +/// Restrict access to existing `path` to only owner. +/// +/// A path that is already owner-only or does not exist, is unaltered +/// and is not an error. +/// Only applicable to platforms with Unix permission bits. +#[cfg(unix)] +pub(crate) fn limit_access(path: &Path, mode: u32) -> std::io::Result<()> { + use std::os::unix::fs::PermissionsExt; + + let metadata = match std::fs::metadata(path) { + Ok(metadata) => metadata, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(e) => return Err(e), + }; + let current = metadata.permissions().mode(); + if current & 0o077 == 0 { + return Ok(()); + } + + eprintln!( + "WARNING: {} was accessible to other users on this system ({:03o}). + Restricting it to {:03o}.\n", + path.display(), + current & 0o777, + mode + ); + std::fs::set_permissions(path, std::fs::Permissions::from_mode(mode)) +} + +#[cfg(not(unix))] +pub(crate) fn limit_access(_path: &Path, _mode: u32) -> std::io::Result<()> { + Ok(()) +} + +/// Write `contents` to `path`, readable by its owner only. +#[cfg(unix)] +pub(crate) fn write_file_content(path: &Path, contents: &str) -> std::io::Result<()> { + use std::io::Write; + use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; + + let mut file = std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(FILE_MODE) + .open(path)?; + + file.set_permissions(std::fs::Permissions::from_mode(FILE_MODE))?; + + file.write_all(contents.as_bytes()) +} + +#[cfg(not(unix))] +pub(crate) fn write_file_content(path: &Path, contents: &str) -> std::io::Result<()> { + std::fs::write(path, contents) +} + +#[cfg(all(test, unix))] +mod datadir_file_permissions_tests { + use super::*; + use std::fs; + use std::os::unix::fs::PermissionsExt; + use tempfile::TempDir; + + fn mode_of(path: &Path) -> u32 { + fs::metadata(path).unwrap().permissions().mode() & 0o777 + } + + #[test] + fn test_write_file_content_creates_an_owner_only_file() { + let temp_dir = TempDir::new().unwrap(); + let path = temp_dir.path().join("config.toml"); + + write_file_content(&path, "tprv").unwrap(); + + assert_eq!(mode_of(&path), FILE_MODE); + assert_eq!(fs::read_to_string(&path).unwrap(), "tprv"); + } + + #[test] + fn test_limit_access_hardens_only_exposed_file() { + let temp_dir = TempDir::new().unwrap(); + let exposed = temp_dir.path().join("exposed.toml"); + let private = temp_dir.path().join("private.toml"); + fs::write(&exposed, "tprv").unwrap(); + fs::write(&private, "tprv").unwrap(); + fs::set_permissions(&exposed, fs::Permissions::from_mode(0o644)).unwrap(); + fs::set_permissions(&private, fs::Permissions::from_mode(0o400)).unwrap(); + + limit_access(&exposed, FILE_MODE).unwrap(); + limit_access(&private, FILE_MODE).unwrap(); + + assert_eq!(mode_of(&exposed), FILE_MODE); + assert_eq!( + mode_of(&private), + 0o400, + "an owner-only file is not updated" + ); + } + + #[test] + fn test_limit_access_hardens_a_directory() { + let temp_dir = TempDir::new().unwrap(); + let dir = temp_dir.path().join("datadir"); + fs::create_dir(&dir).unwrap(); + fs::set_permissions(&dir, fs::Permissions::from_mode(0o755)).unwrap(); + + limit_access(&dir, DIR_MODE).unwrap(); + + assert_eq!(mode_of(&dir), DIR_MODE); + } + + #[test] + fn test_limit_access_ignores_a_missing_path() { + let temp_dir = TempDir::new().unwrap(); + + limit_access(&temp_dir.path().join("absent"), FILE_MODE).unwrap(); + } + + #[test] + fn test_write_file_content_restricts_a_pre_existing_exposed_file() { + let temp_dir = TempDir::new().unwrap(); + let path = temp_dir.path().join("config.toml"); + fs::write(&path, "stale").unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o644)).unwrap(); + + write_file_content(&path, "tprv").unwrap(); + + assert_eq!(mode_of(&path), FILE_MODE); + assert_eq!(fs::read_to_string(&path).unwrap(), "tprv"); + } + + #[test] + fn test_prepare_home_dir_creates_owner_only_datadir() { + let temp_dir = TempDir::new().unwrap(); + let dir = temp_dir.path().join("datadir").join("nested"); + + prepare_home_dir(Some(dir.clone())).unwrap(); + + assert_eq!(mode_of(&dir), DIR_MODE); + assert_eq!(mode_of(dir.parent().unwrap()), DIR_MODE); + } + + #[test] + fn test_prepare_home_dir_hardens_an_exposed_chosen_datadir() { + let temp_dir = TempDir::new().unwrap(); + let dir = temp_dir.path().join("shared"); + fs::create_dir(&dir).unwrap(); + fs::set_permissions(&dir, fs::Permissions::from_mode(0o755)).unwrap(); + + prepare_home_dir(Some(dir.clone())).unwrap(); + + assert_eq!(mode_of(&dir), DIR_MODE); + } + + #[test] + fn test_prepare_wallet_db_dir_is_owner_only() { + let temp_dir = TempDir::new().unwrap(); + let home = temp_dir.path(); + + let dir = prepare_wallet_db_dir(home, "hot").unwrap(); + + assert_eq!(mode_of(&dir), DIR_MODE); + } +} diff --git a/tests/integration/init.rs b/tests/integration/init.rs index 17fdfce..0a2af96 100644 --- a/tests/integration/init.rs +++ b/tests/integration/init.rs @@ -291,8 +291,53 @@ mod test_config { assert_eq!(config["ext_descriptor"].as_str().unwrap(), ext_desc); assert_eq!(config["int_descriptor"].as_str().unwrap(), int_desc); } -} + #[cfg(unix)] + #[test] + fn test_config_with_private_keys_is_unreadable_by_other_users() { + use std::os::unix::fs::PermissionsExt; + + let temp_dir = TempDir::new().unwrap(); + let cli = BdkCli::new("regtest", Some(temp_dir.path().to_path_buf())); + + let desc = cli + .cmd("descriptor", &["--type", "tr"]) + .output() + .expect("Command to generate descriptors failed"); + let desc_values: Value = + serde_json::from_slice(&desc.stdout).expect("Invalid JSON from output descriptor"); + let priv_desc = &desc_values["private_descriptors"]; + + cli.build_base_cmd() + .arg("wallet") + .arg("--wallet") + .arg("secret_wallet") + .arg("config") + .arg("--ext-descriptor") + .arg(priv_desc["external"].as_str().unwrap()) + .arg("--int-descriptor") + .arg(priv_desc["internal"].as_str().unwrap()) + .arg("--client-type") + .arg("rpc") + .arg("--database-type") + .arg("sqlite") + .arg("--url") + .arg("http://localhost:18443") + .assert() + .success() + .stderr(predicate::str::contains("PRIVATE KEYS")); + + let mode = std::fs::metadata(temp_dir.path().join("config.toml")) + .unwrap() + .permissions() + .mode() + & 0o777; + assert_eq!( + mode, 0o600, + "config.toml holds a private descriptor and must not be readable by other users" + ); + } +} // SILENT PAYMENTS #[cfg(feature = "silent-payments")] mod test_silent_payments {