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 ddf9104cc2..8de0078734 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -72,12 +72,10 @@ use crate::gossip::GossipSource; 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::{ - 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, - read_scorer, + create_dir_all_private_checked, 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, read_scorer, }; #[cfg(feature = "storage-vss")] use crate::io::vss_store::VssStoreBuilder; @@ -210,6 +208,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`]. /// @@ -711,8 +712,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()), @@ -2633,18 +2636,31 @@ 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)); + // Only follow symlinks for log paths the user explicitly asked for. + let (log_file_path, follow_symlinks) = match log_file_path { + Some(log_file_path) => (log_file_path.clone(), true), + None => { + setup_default_log_dir(config)?; + (format!("{}/{}", config.storage_dir_path, DEFAULT_LOG_FILENAME), false) + }, + }; let max_log_level = max_log_level.unwrap_or_else(|| DEFAULT_LOG_LEVEL); - Logger::new_fs_writer(log_file_path, max_log_level) + Logger::new_fs_writer(log_file_path, max_log_level, follow_symlinks) .map_err(|_| BuildError::LoggerSetupFailed)? }, Some(LogWriterConfig::Log) => Logger::new_log_facade(), @@ -2654,9 +2670,10 @@ 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) + Logger::new_fs_writer(log_file_path, log_level, false) .map_err(|_| BuildError::LoggerSetupFailed)? }, }; @@ -2710,6 +2727,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..05ed8c3353 100644 --- a/src/config.rs +++ b/src/config.rs @@ -277,6 +277,17 @@ 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. + /// + /// [`Node`]: crate::Node + /// [`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 fb295d3ff9..e904156a1a 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..dcbcca6ad7 100644 --- a/src/logger.rs +++ b/src/logger.rs @@ -26,11 +26,18 @@ use log::{Level as LogFacadeLevel, Record as LogFacadeRecord}; use crate::io::utils::create_dir_all_private; -fn open_log_file(file_path: &str) -> std::io::Result { +fn open_log_file(file_path: &str, follow_symlinks: bool) -> std::io::Result { let mut options = fs::OpenOptions::new(); options.create(true).append(true); #[cfg(unix)] - options.mode(0o600); + { + options.mode(0o600); + if !follow_symlinks { + options.custom_flags(libc::O_NOFOLLOW); + } + } + #[cfg(not(unix))] + let _ = follow_symlinks; options.open(file_path) } @@ -189,7 +196,7 @@ pub trait LogWriter: Send + Sync { /// Defines a writer for [`Logger`]. pub(crate) enum Writer { /// Writes logs to the file system. - FileWriter { file_path: String, max_log_level: LogLevel }, + FileWriter { file_path: String, max_log_level: LogLevel, follow_symlinks: bool }, /// Forwards logs to the `log` facade. LogFacadeWriter, /// Forwards logs to a custom writer. @@ -205,7 +212,7 @@ impl LogWriter for Writer { }; match self { - Writer::FileWriter { file_path, max_log_level } => { + Writer::FileWriter { file_path, max_log_level, follow_symlinks } => { if record.level < *max_log_level { return; } @@ -220,7 +227,7 @@ impl LogWriter for Writer { context, ); - open_log_file(file_path) + open_log_file(file_path, *follow_symlinks) .expect("Failed to open log file") .write_all(log.as_bytes()) .expect("Failed to write to log file") @@ -274,18 +281,25 @@ pub(crate) struct Logger { impl Logger { /// Creates a new logger with a filesystem writer. The parameters to this function - /// are the path to the log file, and the log level. - pub fn new_fs_writer(file_path: String, max_log_level: LogLevel) -> Result { + /// are the path to the log file, the log level, and whether a log file that is a symlink + /// should be followed. + /// + /// Following symlinks should only be allowed for log paths that were explicitly configured by + /// the user, as otherwise somebody able to write to the log directory could redirect our log + /// output to an arbitrary file. + pub fn new_fs_writer( + file_path: String, max_log_level: LogLevel, follow_symlinks: bool, + ) -> Result { if let Some(parent_dir) = Path::new(&file_path).parent() { create_dir_all_private(parent_dir) .map_err(|e| eprintln!("ERROR: Failed to create log parent directory: {}", e))?; // make sure the file exists. - open_log_file(&file_path) + open_log_file(&file_path, follow_symlinks) .map_err(|e| eprintln!("ERROR: Failed to open log file: {}", e))?; } - Ok(Self { writer: Writer::FileWriter { file_path, max_log_level } }) + Ok(Self { writer: Writer::FileWriter { file_path, max_log_level, follow_symlinks } }) } pub fn new_log_facade() -> Self { @@ -300,7 +314,7 @@ impl Logger { impl LdkLogger for Logger { fn log(&self, record: LdkRecord) { match &self.writer { - Writer::FileWriter { file_path: _, max_log_level } => { + Writer::FileWriter { file_path: _, max_log_level, .. } => { if record.level < *max_log_level { return; } @@ -332,13 +346,33 @@ mod tests { let log_dir = random_storage_path(); let log_path = log_dir.join("ldk_node.log"); let _logger = - Logger::new_fs_writer(log_path.to_str().unwrap().to_string(), LogLevel::Info).unwrap(); + Logger::new_fs_writer(log_path.to_str().unwrap().to_string(), LogLevel::Info, false) + .unwrap(); let mode = log_path.metadata().unwrap().permissions().mode(); assert_eq!(mode & 0o077, 0); 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(); + + let log_path_str = log_path.to_str().unwrap().to_string(); + assert!(Logger::new_fs_writer(log_path_str.clone(), LogLevel::Info, false).is_err()); + assert!(fs::read(&target).unwrap().is_empty()); + + // Explicitly configured log paths may be symlinks. + assert!(Logger::new_fs_writer(log_path_str, LogLevel::Info, true).is_ok()); + fs::remove_dir_all(log_dir).unwrap(); + } + /// A minimal log facade logger that captures log output for testing. struct TestLogger { log: Arc>,