fix: create Sqlite databases with owner-only permissions (CWE-732)

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,6 +57,97 @@ use rusqlite::{Connection, OpenFlags, OptionalExtension};
 use error::process_sqlite_error;
 use storage_core::{Data, DbDesc, DbMapId, backend};
 
+/// 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;
+
+    // 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),
+        }
+    }
+
+    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. 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<bool> {
+    use std::os::unix::fs::{OpenOptionsExt, PermissionsExt};
+
+    match std::fs::OpenOptions::new().write(true).create_new(true).mode(0o600).open(path) {
+        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),
+    }
+}
+
 use crate::queries::SqliteQueries;
 
 // Note: DbTx holds the mutex itself and locks it on every operation instead of just holding a lock
@@ -441,14 +556,22 @@ impl backend::Backend for Sqlite {
 
         if let SqliteStorageMode::File(ref path) = self.backend {
             if let Some(parent) = path.parent() {
-                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,
                     "Cannot find the parent directory".to_string(),
                 )
                 .into());
             }
+
+            // 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)?;
         }
 
         let queries = desc.db_maps().transform(queries::SqliteQuery::from_desc);

storage/sqlite/src/tests.rs

@@ -222,3 +222,148 @@ 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");
+    }
+
+    #[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());
+    }
+}