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

[erubboli]

Fixes a High-severity local vulnerability (CWE-732, suggested CVSS 3.1: 7.8) in wallet database creation.

Issue

Wallet databases contain highly sensitive material: the root extended private key, the root VRF private key, and — when store-seed-phrase is enabled — the complete BIP-39 mnemonic and optional passphrase, in plaintext. With the common 0022 umask:

• missing parent directories were created as 0755 (create_dir_all)
• the database file was created by Sqlite as 0644

Any other local unprivileged user could copy a traversable database and recover full signing authority without the victim's RPC credentials.

Fix (storage/sqlite, Unix)

• Directories: missing parent directories are created 0700 — this also shields 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.
• Database file: created atomically with 0600 (pre-created via OpenOptions::create_new().mode(0o600) before Sqlite opens it), so it is never observable with looser permissions, even momentarily.
• Repair: opening an existing database created by a pre-fix version tightens the file to 0600.

Non-Unix platforms keep the previous behavior.

Tests

Two new Unix-only tests in storage/sqlite:

• newly created DB → directory 0700, file 0600
• existing DB with 0644 file → repaired to 0600 on open

Verification

• cargo test -p storage-sqlite: 27/27 ✓, cargo test -p wallet-controller: 24/24 ✓, cargo test -p wallet: 114/114 ✓
• full ./do_checks.sh green under the CI toolchain (1.92.0)

[github-actions]

🔍 OpenCodeReview found 5 issue(s) in this PR.

• ✅ Successfully posted inline: 1 comment(s)
• 📋 Routed to summary by policy: 4 comment(s)

---

📄 storage/sqlite/src/lib.rs (L79-L82)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category other)

Treating AlreadyExists as "existing regular database file" is too coarse: the path could be a directory, a symlink, or a dangling symlink. set_permissions succeeds on directories, so SQLite then fails later with a confusing error. A symlink_metadata check before repairing would surface a clear diagnostic (and could also refuse to follow symlinks, which matters for sensitive wallet files).

💡 Suggested Change

Before:

        Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {
            std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?;
            Ok(false)
        }

After:

        Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {
            let meta = std::fs::symlink_metadata(path)?;
            if !meta.is_file() {
                return Err(std::io::Error::new(
                    std::io::ErrorKind::AlreadyExists,
                    "database path exists but is not a regular file",
                ));
            }
            std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?;
            Ok(false)
        }

---

📄 storage/sqlite/src/lib.rs (L79-L82)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category other)

Policy inconsistency worth confirming: for directories, pre-existing permissions are deliberately left untouched (documented and tested), but for the file this code unconditionally resets mode to 0600 on every open. That silently overrides intentional group-shared setups or backup tooling that relies on group read. If forced repair is the intended product policy, it is fine as-is, but the doc comment should state that it overrides pre-existing intentional permissions; otherwise consider repairing only when the mode is more permissive than 0600.

---

📄 storage/sqlite/src/lib.rs (L51-L52)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category security)

There is a brief TOCTOU window here: create_dir makes the directory exist with the process umask permissions (potentially group/world-readable) before set_permissions tightens it to 0700. On multi-user systems, another user could traverse/observe the directory contents (and later the DB file path) during that window. Consider creating the directory directly with the desired mode (e.g. via Mode::from_bits(0o700) with nix/libc::mkdir, or an OpenOptions-based approach) so the mode is set atomically at creation time, or at least chmod immediately in the same operation without an intervening fallible call.

💡 Suggested Change

Before:

    let created = match std::fs::create_dir(dir) {
        Ok(()) => true,

After:

    // Prefer creating the directory with mode 0700 atomically (e.g. libc::mkdir) to
    // avoid a window where it exists with umask-derived permissions.

---

📄 storage/sqlite/src/lib.rs (L495-L498)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category maintainability)

The #[cfg(unix)] / #[cfg(not(unix))] statement duplication is correct but easy to misread; a small cfg-gated helper (e.g. fn ensure_parent_dir(path: &Path) -> io::Result<()> implemented once per platform) at the top of the file would keep the open flow single-line and platform-neutral, matching the existing ensure_private_directory/create_private_file pattern.

💡 Suggested Change

Before:

                #[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)?;

After:

                ensure_parent_dir(parent).map_err(error::process_io_error)?;

[github-actions]

The AlreadyExists branch unconditionally resets permissions to 0600 on every open. This silently repairs pre-existing files (good for upgrading old installations), but also silently overrides any intentional relaxation by the user (e.g., group-readable for backup tooling) on each launch. Consider documenting this repair-on-open behavior, or only repairing when permissions are world-readable, so user intent other than the insecure default is preserved.

Suggestion:

Suggested change

	Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {
	std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?;
	Ok(false)
	}
	Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {
	// Only repair when the file is exposed to other users, so an intentionally
	// relaxed (but still private-ish) configuration is not silently overwritten.
	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)
	}

[github-actions]

The security hardening is Unix-only. On non-Unix platforms (e.g., Windows), SQLite will still create the database file with default permissions, so wallet data may be readable by other local users there. If other platforms are supported, consider equivalent hardening (e.g., Windows ACL restriction) or at least document the known gap in the function's doc comment.

Suggestion:

Suggested change

	#[cfg(unix)]
	create_private_file(path).map_err(error::process_io_error)?;
	// TODO(hardening): on non-Unix platforms the file is created by SQLite with
	// default permissions; add equivalent ACL hardening or document the gap.
	#[cfg(unix)]
	create_private_file(path).map_err(error::process_io_error)?;

[erubboli]

Addressed the OpenCodeReview findings (run) in bb948a7:

Adopted:

• Race-free directory ownership (comments 3+4): ensure_private_directory now attempts an atomic create_dir first and derives ownership from its result instead of a racy !dir.exists() check — this also removes the case where a directory created concurrently between the check and the chmod would have been tightened against the documented intent. Chose NotFound => create_dir_all + AlreadyExists => leave untouched over the suggested catch-all fallback so the ownership conclusion stays sound (NotFound proves the leaf didn't exist).
• New test pre_existing_directory_permissions_are_left_untouched: creates a 0755 directory, opens the DB inside, and asserts the directory stays 0755 while the file becomes 0600 — pinning the intentional "pre-existing dirs are not modified" behavior (comment 2).

Declined:

• Holding the file handle from create_private_file (comment 1): an open fd doesn't prevent another local process from unlink+recreating the path before SQLite opens it by path, so the suggested change doesn't actually narrow the TOCTOU window — it's security theater. The meaningful mitigations (open-by-fd, unsupported by rusqlite; or post-open inode verification) are disproportionate for a wallet directory under the user's own HOME.

[github-actions]

Two gaps: (1) .mode(0o600) is ANDed with the process umask, so the resulting mode depends on the environment — unlike the directory case, no explicit set_permissions enforces 0600. (2) SQLite itself creates the rollback journal / WAL / SHM files next to the DB with umask-influenced default permissions (e.g. 0644); the pre-created DB file does not protect them. In the documented pre-existing-directory case (directory left at 0755), those sidecar files are world-readable while containing sensitive data. Consider an explicit set_permissions(0o600) after creation, and documenting/mitigating the sidecar-file exposure.

Suggestion:

Suggested change

	match std::fs::OpenOptions::new().write(true).create_new(true).mode(0o600).open(path) {
	Ok(_file) => Ok(true),
	let file = std::fs::OpenOptions::new().write(true).create_new(true).mode(0o600).open(path);
	match file {
	Ok(_file) => {
	// `mode` is masked by the umask; enforce 0600 explicitly for umask independence.
	std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?;
	Ok(true)
	}

[github-actions]

When intermediate parent components are missing, create_dir_all creates them with the process umask (potentially group/world-readable), and only the leaf is tightened to 0700. Sensitive intermediate directories on the wallet path (e.g. ~/.wallet/_{/) can remain readable by others. Additionally there is a small TOCTOU window between directory creation and set_permissions(0700) during which the new leaf has umask permissions. Consider creating components one at a time with create_dir + immediate set_permissions, or tightening every component this call created.}

Suggestion:

Suggested change

	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))?;
	}
	// Walk components so every directory this call creates gets 0700 immediately.
	let mut prefix = None;
	for component in dir.components() {
	let mut next = std::path::PathBuf::new();
	if let Some(p) = &prefix {
	next.push(p);
	}
	next.push(component);
	std::fs::create_dir(&next).ok();
	let _ = std::fs::set_permissions(&next, std::fs::Permissions::from_mode(0o700));
	prefix = Some(next);
	}