From 1b328b86b3efe9272aa5786bff7154441c95327c Mon Sep 17 00:00:00 2001 From: ajaysehwal Date: Sun, 18 Oct 2026 22:03:27 +0530 Subject: [PATCH] Refuse shared storage directories and don't follow symlinks By default we store all node data, including the `ChannelMonitor`s, under `/tmp/ldk_node`. If that directory had been pre-created by another local user, we'd happily use it (`create_dir_all_private` accepts existing directories as-is), allowing them to read, replace or roll back our state, or to redirect our log or database file via a symlink. Here, we add `create_dir_all_private_checked`, which on Unix additionally refuses to use an existing directory if it is owned by a user other than us or `root`, or if it is writable by all users. We use it for the configured storage directory when building the SQLite- and fs-store-based `Node`, and for the default log directory. Further, we open the log file with `O_NOFOLLOW` and refuse to open a symlinked SQLite database file. We also document that the default location is only suitable for testing and add `set_storage_dir_path` to the README/crate-level example. This change was prepared with the assistance of AI tools (Claude Code). --- CHANGELOG.md | 5 +++ README.md | 1 + src/builder.rs | 50 +++++++++++++++++++++++++---- src/config.rs | 10 ++++++ src/io/fs_store.rs | 4 +-- src/io/sqlite_store/mod.rs | 28 ++++++++++++++++ src/io/utils.rs | 65 +++++++++++++++++++++++++++++++++++++- src/lib.rs | 1 + src/logger.rs | 19 ++++++++++- 9 files changed, 173 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d7c631bb3..9d3e6c7453 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,11 @@ # Pending ## Compatibility Notes +- On Unix, building a `Node` now fails with `BuildError::StoragePathAccessFailed` if the + configured `storage_dir_path` already exists but is owned by a user other than the current + user or `root`, or is writable by all users. Symlinked SQLite database and log files are no + longer followed. The default storage directory remains `/tmp/ldk_node`, but should not be used + in production. - The language bindings now expose `Mnemonic` as an object instead of a string alias. Existing mnemonic phrases must be passed through its fallible constructor, which returns `NodeError::InvalidMnemonic` for invalid input; generated mnemonics can be converted back to a diff --git a/README.md b/README.md index 1652541137..1510be4094 100644 --- a/README.md +++ b/README.md @@ -27,6 +27,7 @@ use std::str::FromStr; fn main() { let mut builder = Builder::new(); builder.set_network(Network::Testnet); + builder.set_storage_dir_path("/path/to/ldk_node_data".to_string()); builder.set_chain_source_esplora("https://blockstream.info/testnet/api".to_string(), None); builder.set_gossip_source_rgs( "https://rapidsync.lightningdevkit.org/testnet/v2/snapshot".to_string(), diff --git a/src/builder.rs b/src/builder.rs index f2704b6425..958d438380 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -74,7 +74,7 @@ use crate::io::fs_store::open_or_migrate_fs_store; #[cfg(feature = "storage-sqlite")] use crate::io::sqlite_store::SqliteStore; #[cfg(feature = "storage-sqlite")] -use crate::io::utils::create_dir_all_private; +use crate::io::utils::create_dir_all_private_checked; use crate::io::utils::{ read_all_objects, read_event_queue, read_external_pathfinding_scores_from_cache, read_n_objects, read_network_graph, read_node_metrics, read_output_sweeper, read_peer_info, @@ -211,6 +211,9 @@ pub enum BuildError { /// [`KVStore`]: lightning::util::persist::KVStore WriteFailed, /// We failed to access the given `storage_dir_path`. + /// + /// This is also returned on Unix if the directory exists but is owned by another user (other + /// than `root`) or is writable by all users. StoragePathAccessFailed, /// We failed to setup our [`KVStore`]. /// @@ -712,8 +715,10 @@ impl NodeBuilder { pub fn build(&self, node_entropy: NodeEntropy) -> Result { let logger = setup_logger(&self.log_writer_config, &self.config)?; let storage_dir_path = self.config.storage_dir_path.clone(); - create_dir_all_private(storage_dir_path.as_ref()) - .map_err(|_| BuildError::StoragePathAccessFailed)?; + create_dir_all_private_checked(storage_dir_path.as_ref()).map_err(|e| { + log_error!(logger, "Failed to set up storage directory: {}", e); + BuildError::StoragePathAccessFailed + })?; let kv_store = SqliteStore::new( storage_dir_path.into(), Some(io::sqlite_store::SQLITE_DB_FILE_NAME.to_string()), @@ -2634,15 +2639,27 @@ fn optionally_install_rustls_cryptoprovider() { }); } +/// Makes sure the default log directory, i.e., our storage directory, exists and is safe to use. +fn setup_default_log_dir(config: &Config) -> Result<(), BuildError> { + create_dir_all_private_checked(config.storage_dir_path.as_ref()).map_err(|e| { + eprintln!("ERROR: Failed to set up storage directory: {}", e); + BuildError::StoragePathAccessFailed + }) +} + /// Sets up the node logger. fn setup_logger( log_writer_config: &Option, config: &Config, ) -> Result, BuildError> { let logger = match log_writer_config { Some(LogWriterConfig::File { log_file_path, max_log_level }) => { - let log_file_path = log_file_path - .clone() - .unwrap_or_else(|| format!("{}/{}", config.storage_dir_path, DEFAULT_LOG_FILENAME)); + let log_file_path = match log_file_path { + Some(log_file_path) => log_file_path.clone(), + None => { + setup_default_log_dir(config)?; + format!("{}/{}", config.storage_dir_path, DEFAULT_LOG_FILENAME) + }, + }; let max_log_level = max_log_level.unwrap_or_else(|| DEFAULT_LOG_LEVEL); Logger::new_fs_writer(log_file_path, max_log_level) @@ -2655,6 +2672,7 @@ fn setup_logger( }, None => { // Default to use `FileWriter` + setup_default_log_dir(config)?; let log_file_path = format!("{}/{}", config.storage_dir_path, DEFAULT_LOG_FILENAME); let log_level = DEFAULT_LOG_LEVEL; Logger::new_fs_writer(log_file_path, log_level) @@ -2711,6 +2729,26 @@ mod tests { use crate::io::test_utils::InMemoryStore; use crate::logger::Logger; + #[cfg(all(unix, feature = "storage-sqlite"))] + #[test] + fn build_rejects_world_writable_storage_dir() { + use std::os::unix::fs::PermissionsExt; + + let storage_dir = crate::io::test_utils::random_storage_path(); + std::fs::create_dir_all(&storage_dir).unwrap(); + std::fs::set_permissions(&storage_dir, std::fs::Permissions::from_mode(0o777)).unwrap(); + + let mut builder = NodeBuilder::new(); + builder.set_storage_dir_path(storage_dir.to_str().unwrap().to_string()); + #[cfg(not(feature = "uniffi"))] + let node_entropy = NodeEntropy::from_seed_bytes([42; 64]); + #[cfg(feature = "uniffi")] + let node_entropy = NodeEntropy::from_seed_bytes(vec![42; 64]).unwrap(); + assert!(matches!(builder.build(node_entropy), Err(BuildError::StoragePathAccessFailed))); + + std::fs::remove_dir_all(storage_dir).unwrap(); + } + struct ChannelManagerReadFailingStore(InMemoryStore); impl KVStore for ChannelManagerReadFailingStore { diff --git a/src/config.rs b/src/config.rs index d524eac58e..efcaeea199 100644 --- a/src/config.rs +++ b/src/config.rs @@ -277,6 +277,16 @@ impl Default for ForwardedPaymentTrackingMode { /// [`Node`]: crate::Node pub struct Config { /// The path where the underlying LDK and BDK persist their data. + /// + /// The default is located in `/tmp`, which is typically cleared on reboot and shared with other + /// local users. It is therefore only suitable for testing and **should always be set to a + /// dedicated, persistent directory in production**. + /// + /// On Unix, building a [`Node`] fails with [`BuildError::StoragePathAccessFailed`] if the + /// directory already exists but is owned by a user other than the current user or `root`, or + /// if it is writable by all users. + /// + /// [`BuildError::StoragePathAccessFailed`]: crate::BuildError::StoragePathAccessFailed pub storage_dir_path: String, /// The used Bitcoin network. pub network: Network, diff --git a/src/io/fs_store.rs b/src/io/fs_store.rs index 88c4749617..7415994480 100644 --- a/src/io/fs_store.rs +++ b/src/io/fs_store.rs @@ -12,7 +12,7 @@ use lightning::util::persist::migrate_kv_store_data_async; use lightning_persister::fs_store::v1::FilesystemStore; use lightning_persister::fs_store::v2::{FilesystemStoreV2, FilesystemStoreV2Error}; -use crate::io::utils::create_dir_all_private; +use crate::io::utils::{create_dir_all_private, create_dir_all_private_checked}; use crate::BuildError; /// Opens a [`FilesystemStoreV2`], automatically migrating from v1 format if necessary. @@ -24,7 +24,7 @@ pub(crate) async fn open_or_migrate_fs_store( storage_dir_path: PathBuf, ) -> Result { let parent_dir = storage_dir_path.parent().ok_or(BuildError::StoragePathAccessFailed)?; - create_dir_all_private(parent_dir).map_err(|_| BuildError::StoragePathAccessFailed)?; + create_dir_all_private_checked(parent_dir).map_err(|_| BuildError::StoragePathAccessFailed)?; recover_incomplete_fs_store_migration(&storage_dir_path)?; if !storage_dir_path.exists() { create_dir_all_private(&storage_dir_path) diff --git a/src/io/sqlite_store/mod.rs b/src/io/sqlite_store/mod.rs index 8112140a73..c08827b556 100644 --- a/src/io/sqlite_store/mod.rs +++ b/src/io/sqlite_store/mod.rs @@ -287,6 +287,15 @@ impl SqliteStoreInner { }, } + #[cfg(unix)] + if std::fs::symlink_metadata(&db_file_path).map_or(false, |m| m.file_type().is_symlink()) { + let msg = format!( + "Refusing to open database file {} as it is a symlink", + db_file_path.display() + ); + return Err(io::Error::new(io::ErrorKind::Other, msg)); + } + let mut connection = Connection::open(db_file_path.clone()).map_err(|e| { let msg = format!("Failed to open/create database file {}: {}", db_file_path.display(), e); @@ -913,6 +922,25 @@ mod tests { assert_eq!(file_mode & 0o077, 0); } + #[cfg(unix)] + #[test] + fn rejects_symlinked_database_file() { + let data_dir = random_storage_path(); + fs::create_dir_all(&data_dir).unwrap(); + let target = data_dir.join("target.sqlite"); + fs::write(&target, b"").unwrap(); + std::os::unix::fs::symlink(&target, data_dir.join("linked.sqlite")).unwrap(); + + let result = SqliteStore::new( + data_dir.clone(), + Some("linked.sqlite".to_string()), + Some("test_table".to_string()), + ); + assert!(result.is_err()); + + fs::remove_dir_all(data_dir).unwrap(); + } + #[test] fn rejects_sqlite_pseudo_filenames() { for (data_dir, db_file_name) in [ diff --git a/src/io/utils.rs b/src/io/utils.rs index 31f3166597..35a1b7f92f 100644 --- a/src/io/utils.rs +++ b/src/io/utils.rs @@ -60,6 +60,46 @@ pub(crate) fn create_dir_all_private(path: &Path) -> std::io::Result<()> { builder.create(path) } +/// Creates the given directory (see [`create_dir_all_private`]) and, on Unix, additionally ensures +/// that it is safe to store secrets and channel state in. +/// +/// If the directory already existed (e.g., because it was pre-created by somebody else, as is +/// possible for the default `/tmp` location), we refuse to use it if it is not owned by us or +/// `root`, or if it is writable by everybody, as other local users could otherwise read, replace +/// or roll back our data. +pub(crate) fn create_dir_all_private_checked(path: &Path) -> std::io::Result<()> { + create_dir_all_private(path)?; + #[cfg(unix)] + verify_dir_not_shared(path)?; + Ok(()) +} + +#[cfg(unix)] +fn verify_dir_not_shared(path: &Path) -> std::io::Result<()> { + use std::os::unix::fs::MetadataExt; + + let metadata = fs::metadata(path)?; + // SAFETY: `geteuid` is always safe to call and can't fail. + let euid = unsafe { libc::geteuid() }; + if metadata.uid() != euid && metadata.uid() != 0 { + let msg = format!( + "Refusing to use directory {} as it is owned by another user (uid {})", + path.display(), + metadata.uid() + ); + return Err(std::io::Error::new(std::io::ErrorKind::PermissionDenied, msg)); + } + if metadata.mode() & 0o002 != 0 { + let msg = format!( + "Refusing to use directory {} as it is writable by all users (mode {:o})", + path.display(), + metadata.mode() & 0o7777 + ); + return Err(std::io::Error::new(std::io::ErrorKind::PermissionDenied, msg)); + } + Ok(()) +} + pub(crate) fn read_or_generate_seed_file( keys_seed_path: &str, ) -> std::io::Result<[u8; WALLET_KEYS_SEED_LEN]> { @@ -770,7 +810,9 @@ pub(crate) async fn read_bdk_wallet_change_set( #[cfg(test)] mod tests { use super::test_utils::random_storage_path; - use super::{create_dir_all_private, read_or_generate_seed_file}; + use super::{ + create_dir_all_private, create_dir_all_private_checked, read_or_generate_seed_file, + }; #[cfg(unix)] #[test] @@ -789,6 +831,27 @@ mod tests { std::fs::remove_dir_all(base_path).unwrap(); } + #[cfg(unix)] + #[test] + fn checked_dir_creation_rejects_shared_dirs() { + use std::os::unix::fs::PermissionsExt; + + let base_path = random_storage_path(); + let dir_path = base_path.join("storage"); + create_dir_all_private_checked(&dir_path).unwrap(); + // Re-opening our own private directory is fine, as is a group-writable one. + create_dir_all_private_checked(&dir_path).unwrap(); + std::fs::set_permissions(&dir_path, std::fs::Permissions::from_mode(0o770)).unwrap(); + create_dir_all_private_checked(&dir_path).unwrap(); + + // A world-writable directory must be refused. + std::fs::set_permissions(&dir_path, std::fs::Permissions::from_mode(0o777)).unwrap(); + let err = create_dir_all_private_checked(&dir_path).unwrap_err(); + assert_eq!(err.kind(), std::io::ErrorKind::PermissionDenied); + + std::fs::remove_dir_all(base_path).unwrap(); + } + #[test] fn generated_seed_is_readable() { let mut rand_path = random_storage_path(); diff --git a/src/lib.rs b/src/lib.rs index 83bcc6afa4..27cafe5aef 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -38,6 +38,7 @@ //! fn main() { //! let mut builder = Builder::new(); //! builder.set_network(Network::Testnet); +//! builder.set_storage_dir_path("/path/to/ldk_node_data".to_string()); //! builder.set_chain_source_esplora("https://blockstream.info/testnet/api".to_string(), None); //! builder.set_gossip_source_rgs( //! "https://rapidsync.lightningdevkit.org/testnet/v2/snapshot".to_string(), diff --git a/src/logger.rs b/src/logger.rs index c0b0082278..cc0c50df3f 100644 --- a/src/logger.rs +++ b/src/logger.rs @@ -30,7 +30,7 @@ fn open_log_file(file_path: &str) -> std::io::Result { let mut options = fs::OpenOptions::new(); options.create(true).append(true); #[cfg(unix)] - options.mode(0o600); + options.mode(0o600).custom_flags(libc::O_NOFOLLOW); options.open(file_path) } @@ -339,6 +339,23 @@ mod tests { fs::remove_dir_all(log_dir).unwrap(); } + #[cfg(unix)] + #[test] + fn does_not_follow_symlinked_log_file() { + let log_dir = random_storage_path(); + fs::create_dir_all(&log_dir).unwrap(); + let target = log_dir.join("target.log"); + fs::write(&target, b"").unwrap(); + let log_path = log_dir.join("ldk_node.log"); + std::os::unix::fs::symlink(&target, &log_path).unwrap(); + + assert!( + Logger::new_fs_writer(log_path.to_str().unwrap().to_string(), LogLevel::Info).is_err() + ); + assert!(fs::read(&target).unwrap().is_empty()); + fs::remove_dir_all(log_dir).unwrap(); + } + /// A minimal log facade logger that captures log output for testing. struct TestLogger { log: Arc>,