From 0fcb4a3d63d524bc4a023358eebc66a288f1794b Mon Sep 17 00:00:00 2001 From: Enrico Rubboli Date: Tue, 15 Sep 2026 18:23:04 +0400 Subject: [PATCH 1/3] fix: create Sqlite databases with owner-only permissions Wallet databases contain highly sensitive data: the root extended private key, the root VRF private key, and (optionally) the BIP-39 mnemonic and passphrase in plaintext. With the common 0022 umask, the database file was created as 0644 and missing parent directories as 0755, allowing another local unprivileged user (when the path is traversable) to copy the database and recover the signing authority. - Create missing parent directories as 0700, which also protects the auxiliary Sqlite files (rollback journal, WAL, shared-memory, temporary files) under the same private directory boundary. Permissions of pre-existing directories are left untouched, since they may be shared with unrelated data. - Create the database file atomically as 0600 (pre-creating it before Sqlite opens it), so its contents are never observable with looser permissions, even momentarily. - Repair the database file permissions to 0600 when opening an existing database created by a pre-fix version. Unix only; other platforms keep the previous behavior. --- storage/sqlite/src/lib.rs | 46 +++++++++++++++++++++++++++++ storage/sqlite/src/tests.rs | 58 +++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+) diff --git a/storage/sqlite/src/lib.rs b/storage/sqlite/src/lib.rs index 8f63b095cd..97593e41f9 100644 --- a/storage/sqlite/src/lib.rs +++ b/storage/sqlite/src/lib.rs @@ -33,6 +33,44 @@ 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; + + let need_create = !dir.exists(); + std::fs::create_dir_all(dir)?; + + if need_create { + 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 +479,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 +490,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..e6eadb4b33 100644 --- a/storage/sqlite/src/tests.rs +++ b/storage/sqlite/src/tests.rs @@ -222,3 +222,61 @@ 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" + ); + } +} From 895e678e1a925626b238e2406b21b1431d47cfb5 Mon Sep 17 00:00:00 2001 From: Enrico Rubboli Date: Tue, 15 Sep 2026 18:57:30 +0400 Subject: [PATCH 2/3] fix: make directory ownership detection race-free Replace the racy exists() check with an atomic create_dir call: if the leaf is created by this call it is tightened to 0700, if it already existed its permissions are left untouched as documented. Pin the latter behavior with a dedicated test. --- storage/sqlite/src/lib.rs | 19 ++++++++++++++++--- storage/sqlite/src/tests.rs | 21 +++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/storage/sqlite/src/lib.rs b/storage/sqlite/src/lib.rs index 97593e41f9..7b77048066 100644 --- a/storage/sqlite/src/lib.rs +++ b/storage/sqlite/src/lib.rs @@ -44,10 +44,23 @@ use storage_core::{Data, DbDesc, DbMapId, backend}; fn ensure_private_directory(dir: &Path) -> std::io::Result<()> { use std::os::unix::fs::PermissionsExt; - let need_create = !dir.exists(); - std::fs::create_dir_all(dir)?; + // 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 need_create { + if created { std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o700))?; } diff --git a/storage/sqlite/src/tests.rs b/storage/sqlite/src/tests.rs index e6eadb4b33..9d8c0f379b 100644 --- a/storage/sqlite/src/tests.rs +++ b/storage/sqlite/src/tests.rs @@ -279,4 +279,25 @@ mod permissions_tests { "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"); + } } From 00eb68a7ff8de1f9567e96373376fe9826cf9dd2 Mon Sep 17 00:00:00 2001 From: Enrico Rubboli Date: Wed, 16 Sep 2026 13:07:43 +0400 Subject: [PATCH 3/3] Address OpenCodeReview findings on the sqlite permissions PR - Repair the file permissions on open only when the file is actually exposed to group/other users, so an intentionally chosen, already private mode is not silently overwritten. - Reject non-regular files (directories, symlinks) at the database path up front with a clear error instead of following them. - Enforce 0600 explicitly after creating the file, since the mode of OpenOptions is masked by the process umask. - Create missing directory components one by one, tightening each to 0700 immediately, instead of relying on create_dir_all, which leaves intermediate components with umask-derived permissions. - Extract an ensure_parent_dir helper to remove the cfg-gated duplication in Backend::open. - Document the known gaps: Sqlite sidecar files are only protected by the containing directory's permissions, and the hardening is Unix-only. - Add tests for nested directory creation, symlink/directory rejection and preservation of already-private permissions. --- storage/sqlite/src/lib.rs | 124 +++++++++++++++++++++++++++--------- storage/sqlite/src/tests.rs | 66 +++++++++++++++++++ 2 files changed, 160 insertions(+), 30 deletions(-) diff --git a/storage/sqlite/src/lib.rs b/storage/sqlite/src/lib.rs index 7b77048066..76cb25b90d 100644 --- a/storage/sqlite/src/lib.rs +++ b/storage/sqlite/src/lib.rs @@ -15,6 +15,30 @@ //! A `Backend` implementation for Sqlite whose transactions are `Send`, so it's usable //! in an async context. +//! +//! # Security notes (Unix) +//! +//! The database can contain highly sensitive data (wallet databases store private keys +//! and optionally the seed phrase), so on Unix this backend takes care to never expose +//! it to other local users: +//! +//! * a missing database directory (including any missing parent components) is created +//! with owner-only permissions (0700); +//! * the database file is created with owner-only permissions (0600) and, if a +//! pre-existing database file is exposed to group/other users, its permissions are +//! repaired to 0600 on open; +//! * symlinks and non-regular files at the database path are rejected rather than +//! followed. +//! +//! Note that the permissions of pre-existing directories are left untouched, since they +//! may be shared with unrelated data. Keep in mind that the auxiliary files that Sqlite +//! itself creates next to the database (rollback journal, WAL, shared-memory) inherit +//! umask-derived default permissions and are only protected by the permissions of the +//! containing directory, so a pre-existing world-accessible database directory may +//! briefly expose those files. +//! +//! On non-Unix platforms the file-level hardening is not implemented (known gap); the +//! files are created by Sqlite with the platform-default permissions. extern crate core; @@ -33,51 +57,91 @@ 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, +/// Ensure that the directory of the database file exists. If the directory (or any of its +/// missing parents) 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 + // Create the missing components one by one, tightening each directory created by + // this call to 0700 immediately. This avoids both the window in which a freshly + // created directory exists with umask-derived permissions (TOCTOU) and the issue of + // `create_dir_all` leaving intermediate components with such permissions. + let mut prefix = PathBuf::new(); + for component in dir.components() { + prefix.push(component); + match std::fs::create_dir(&prefix) { + Ok(()) => { + std::fs::set_permissions(&prefix, std::fs::Permissions::from_mode(0o700))?; + } + // The component already exists; its permissions are deliberately left + // untouched (it may be shared with unrelated data). + Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {} + // All the previous components exist (we have processed them above), so a + // `NotFound` here could only be caused by a concurrent change, which we + // don't try to paper over. + Err(err) => return Err(err), } - 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(()) } +/// Ensure that the parent directory of the database file exists. On Unix it is created +/// with owner-only permissions (see [`ensure_private_directory`]); on other platforms +/// the platform-default permissions are used, as the file-level hardening is not +/// implemented there (see the security notes in the module documentation). +#[cfg(unix)] +fn ensure_parent_dir(parent: &Path) -> std::io::Result<()> { + ensure_private_directory(parent) +} + +#[cfg(not(unix))] +fn ensure_parent_dir(parent: &Path) -> std::io::Result<()> { + std::fs::create_dir_all(parent) +} + /// 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. +/// (temporarily empty) contents are never observable by other users. Note that the +/// `mode` of `OpenOptions` is masked by the process umask, so an explicit +/// `set_permissions` call is needed to enforce 0600 in any environment. +/// +/// If the file already exists: +/// * a non-regular file (e.g. a directory or a symlink) is rejected with an error +/// instead of being followed; +/// * if its permissions are exposed to group/other users, they are repaired to 0600. +/// Other (already private) permissions are left untouched, so that an intentionally +/// chosen, non-exposed mode is not silently overwritten. +/// +/// 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 => { + Ok(_file) => { + // `.mode(0o600)` is masked by the umask, so enforce the mode explicitly. std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?; + Ok(true) + } + Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => { + // Sensitive wallet data must not end up behind a symlink, and "repairing" + // something that isn't a regular file (e.g. a directory) would produce a + // confusing error from Sqlite later, so reject such paths up front. + if !std::fs::symlink_metadata(path)?.is_file() { + return Err(std::io::Error::new( + std::io::ErrorKind::AlreadyExists, + "database path exists but is not a regular file", + )); + } + // Only repair the permissions if the file is exposed to group/other users. + let mode = std::fs::metadata(path)?.permissions().mode(); + if mode & 0o077 != 0 { + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?; + } Ok(false) } Err(err) => Err(err), @@ -492,10 +556,7 @@ 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)?; + ensure_parent_dir(parent).map_err(error::process_io_error)?; } else { return Err(storage_core::error::Fatal::Io( std::io::ErrorKind::NotFound, @@ -506,6 +567,9 @@ impl backend::Backend for Sqlite { // Pre-create the database file with owner-only permissions so that Sqlite never // creates it with the default (world-readable) permissions. + // Note: this hardening is Unix-only; on other platforms the file is created by + // Sqlite with the platform-default permissions (see the security notes in the + // module documentation). #[cfg(unix)] create_private_file(path).map_err(error::process_io_error)?; } diff --git a/storage/sqlite/src/tests.rs b/storage/sqlite/src/tests.rs index 9d8c0f379b..6cb1f720a3 100644 --- a/storage/sqlite/src/tests.rs +++ b/storage/sqlite/src/tests.rs @@ -300,4 +300,70 @@ mod permissions_tests { ); assert_eq!(mode(&db_path), 0o600, "database file must still be 0600"); } + + #[test] + fn nested_directories_are_created_private() { + let tmp = tempfile::TempDir::new().unwrap(); + let db_dir = tmp.path().join("level1").join("level2"); + let db_path = db_dir.join("wallet.db"); + + let db = Sqlite::new(&db_path).open(make_desc()).unwrap(); + drop(db); + + // Every directory created by this call must be owner-only, including the + // intermediate components. + assert_eq!( + mode(&tmp.path().join("level1")), + 0o700, + "intermediate directory must be 0700" + ); + assert_eq!(mode(&db_dir), 0o700, "leaf directory must be 0700"); + assert_eq!(mode(&db_path), 0o600, "database file must be 0600"); + } + + #[test] + fn already_private_permissions_are_left_untouched() { + let tmp = tempfile::TempDir::new().unwrap(); + let tmp = tmp.path(); + let db_path = tmp.join("wallet.db"); + + // Create the database, then restrict it to an owner-only, read-only mode, i.e. + // one that is not exposed to group/other users. + let db = Sqlite::new(&db_path).open(make_desc()).unwrap(); + drop(db); + std::fs::set_permissions(&db_path, std::fs::Permissions::from_mode(0o400)).unwrap(); + + // Opening may fail afterwards (Sqlite needs write access), depending on the + // environment, but the intentionally chosen permissions must be left untouched + // either way. + let _ = Sqlite::new(&db_path).open(make_desc()); + assert_eq!( + mode(&db_path), + 0o400, + "already private permissions must not be overwritten" + ); + } + + #[test] + fn symlinked_database_path_is_rejected() { + let tmp = tempfile::TempDir::new().unwrap(); + let tmp = tmp.path(); + let target = tmp.join("target.db"); + std::fs::File::create(&target).unwrap(); + let db_path = tmp.join("wallet.db"); + std::os::unix::fs::symlink(&target, &db_path).unwrap(); + + // Sensitive wallet data must not end up behind a symlink. + assert!(Sqlite::new(&db_path).open(make_desc()).is_err()); + } + + #[test] + fn directory_at_database_path_is_rejected() { + let tmp = tempfile::TempDir::new().unwrap(); + let db_path = tmp.path().join("wallet.db"); + std::fs::create_dir(&db_path).unwrap(); + + // A clear error instead of a confusing failure from inside Sqlite. + assert!(Sqlite::new(&db_path).open(make_desc()).is_err()); + } }