diff --git a/storage/sqlite/src/lib.rs b/storage/sqlite/src/lib.rs index 8f63b095cd..7b77048066 100644 --- a/storage/sqlite/src/lib.rs +++ b/storage/sqlite/src/lib.rs @@ -33,6 +33,57 @@ use rusqlite::{Connection, OpenFlags, OptionalExtension}; use error::process_sqlite_error; use storage_core::{Data, DbDesc, DbMapId, backend}; +/// The database can contain highly sensitive data (wallet databases store private keys and +/// optionally the seed phrase), so it must never be readable by other users. +/// +/// If the directory does not exist, it is created with owner-only permissions (0700), which +/// also protects the auxiliary files that Sqlite creates (rollback journal, WAL, +/// shared-memory, temporary files). The permissions of pre-existing directories are left +/// untouched, since they may be shared with unrelated data. +#[cfg(unix)] +fn ensure_private_directory(dir: &Path) -> std::io::Result<()> { + use std::os::unix::fs::PermissionsExt; + + // Determine whether this call creates the directory by attempting an atomic create_dir + // first, instead of a racy exists() check: if the leaf already exists its permissions + // are deliberately left untouched (it may be shared with unrelated data), otherwise it + // was created by this call and is immediately tightened to 0700. + let created = match std::fs::create_dir(dir) { + Ok(()) => true, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => { + // Some parent component was missing; create the whole path. The leaf did not + // exist in this case either, so it was created by this call as well. + std::fs::create_dir_all(dir)?; + true + } + Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => false, + Err(err) => return Err(err), + }; + + if created { + std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o700))?; + } + + Ok(()) +} + +/// Create the database file atomically with owner-only permissions (0600), so that its +/// (temporarily empty) contents are never observable by other users. If the file already +/// exists, its permissions are repaired to 0600 instead. Returns whether the file was created. +#[cfg(unix)] +fn create_private_file(path: &Path) -> std::io::Result { + use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; + + match std::fs::OpenOptions::new().write(true).create_new(true).mode(0o600).open(path) { + Ok(_file) => Ok(true), + Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => { + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?; + Ok(false) + } + Err(err) => Err(err), + } +} + use crate::queries::SqliteQueries; // Note: DbTx holds the mutex itself and locks it on every operation instead of just holding a lock @@ -441,6 +492,9 @@ impl backend::Backend for Sqlite { if let SqliteStorageMode::File(ref path) = self.backend { if let Some(parent) = path.parent() { + #[cfg(unix)] + ensure_private_directory(parent).map_err(error::process_io_error)?; + #[cfg(not(unix))] std::fs::create_dir_all(parent).map_err(error::process_io_error)?; } else { return Err(storage_core::error::Fatal::Io( @@ -449,6 +503,11 @@ impl backend::Backend for Sqlite { ) .into()); } + + // Pre-create the database file with owner-only permissions so that Sqlite never + // creates it with the default (world-readable) permissions. + #[cfg(unix)] + create_private_file(path).map_err(error::process_io_error)?; } let queries = desc.db_maps().transform(queries::SqliteQuery::from_desc); diff --git a/storage/sqlite/src/tests.rs b/storage/sqlite/src/tests.rs index 5e8412fb33..9d8c0f379b 100644 --- a/storage/sqlite/src/tests.rs +++ b/storage/sqlite/src/tests.rs @@ -222,3 +222,82 @@ fn db_open_in_memory_named() { assert!(dbtx.get(MAPID.0, b"hello").unwrap().is_none()); } } + +/// Verify that newly created (and pre-existing) wallet databases get owner-only permissions +/// on Unix, protecting the sensitive data (private keys, seed phrase) stored inside. +#[cfg(unix)] +mod permissions_tests { + use std::os::unix::fs::PermissionsExt; + use std::path::Path; + + use super::Sqlite; + use storage_backend_test_suite::prelude::desc; + use storage_core::{DbDesc, backend::Backend}; + + fn mode(path: &Path) -> u32 { + std::fs::metadata(path).unwrap().permissions().mode() & 0o777 + } + + fn make_desc() -> DbDesc { + desc(1) + } + + #[test] + fn newly_created_db_has_owner_only_permissions() { + let tmp = tempfile::TempDir::new().unwrap(); + let db_dir = tmp.path().join("subdir"); + let db_path = db_dir.join("wallet.db"); + + let db = Sqlite::new(&db_path).open(make_desc()).unwrap(); + drop(db); + + assert_eq!(mode(&db_dir), 0o700, "directory must be 0700"); + assert_eq!(mode(&db_path), 0o600, "database file must be 0600"); + } + + #[test] + fn insecure_permissions_of_existing_db_are_repaired() { + let tmp = tempfile::TempDir::new().unwrap(); + let tmp = tmp.path(); + let db_path = tmp.join("wallet.db"); + + // Create the database, then weaken the file permissions like a pre-fix installation. + // Note: the directory here is pre-existing (tempfile), so its permissions are left + // untouched (it may be shared with unrelated data); only the database file itself + // must be repaired. + let db = Sqlite::new(&db_path).open(make_desc()).unwrap(); + drop(db); + std::fs::set_permissions(&db_path, std::fs::Permissions::from_mode(0o644)).unwrap(); + + // Re-open: the file permissions must be repaired. + let db = Sqlite::new(&db_path).open(make_desc()).unwrap(); + drop(db); + + assert_eq!( + mode(&db_path), + 0o600, + "database permissions must be repaired" + ); + } + + #[test] + fn pre_existing_directory_permissions_are_left_untouched() { + let tmp = tempfile::TempDir::new().unwrap(); + let db_dir = tmp.path().join("existing"); + std::fs::create_dir(&db_dir).unwrap(); + std::fs::set_permissions(&db_dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + let db_path = db_dir.join("wallet.db"); + + let db = Sqlite::new(&db_path).open(make_desc()).unwrap(); + drop(db); + + // Document the intentional behavior: a pre-existing (possibly permissive) directory + // is not modified; only the database file itself is protected. + assert_eq!( + mode(&db_dir), + 0o755, + "pre-existing directory permissions must be left untouched" + ); + assert_eq!(mode(&db_path), 0o600, "database file must still be 0600"); + } +}