Refuse shared storage directories and don't follow symlinks

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

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(),

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<Node, BuildError> {
 		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<LogWriterConfig>, config: &Config,
 ) -> Result<Arc<Logger>, 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 {

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,

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<FilesystemStoreV2, BuildError> {
 	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)

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 [

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();

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(),

src/logger.rs

@@ -30,7 +30,7 @@ fn open_log_file(file_path: &str) -> std::io::Result<fs::File> {
 	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<Mutex<String>>,