fix: create Sqlite databases with owner-only permissions (CWE-732) - #2114
Conversation
|
🔍 OpenCodeReview found 6 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => { | ||
| std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?; | ||
| Ok(false) | ||
| } |
There was a problem hiding this comment.
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:
| 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) | |
| } |
| #[cfg(unix)] | ||
| create_private_file(path).map_err(error::process_io_error)?; |
There was a problem hiding this comment.
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:
| #[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)?; |
|
Addressed the OpenCodeReview findings (run) in bb948a7: Adopted:
Declined:
|
| match std::fs::OpenOptions::new().write(true).create_new(true).mode(0o600).open(path) { | ||
| Ok(_file) => Ok(true), |
There was a problem hiding this comment.
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:
| 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) | |
| } |
bb948a7 to
f869714
Compare
| 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))?; | ||
| } |
There was a problem hiding this comment.
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:
| 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); | |
| } |
|
Addressed the OpenCodeReview findings (run rounds 1–3) in cdad745: Adopted:
Documented (declined as code changes):
New tests: |
| // 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 => {} |
There was a problem hiding this comment.
create_dir returns AlreadyExists not only for an existing directory but also for an existing symlink or a regular file at that component, and both cases are silently accepted here. A symlinked path component is followed (e.g. db -> /tmp/attacker_dir), which redirects the database, its 0600 file, and all Sqlite auxiliary files into a location chosen by whoever could create that symlink — contradicting the module docs' claim that symlinks "are rejected rather than followed" and weakening the 0700 protection this function is supposed to guarantee. Consider checking symlink_metadata/file_type().is_dir() on the AlreadyExists branch (and rejecting symlinks, mirroring create_private_file), or using open with O_DIRECTORY | O_NOFOLLOW to verify each component.
Suggestion:
| // 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 => {} | |
| Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => { | |
| // Reject symlinked (or non-directory) components, mirroring the | |
| // file-path handling in `create_private_file`. | |
| if !std::fs::symlink_metadata(&prefix) | |
| .map(|m| m.is_dir()) | |
| .unwrap_or(false) | |
| { | |
| return Err(std::io::Error::new( | |
| std::io::ErrorKind::AlreadyExists, | |
| "database directory component exists but is not a directory", | |
| )); | |
| } | |
| } |
| 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))?; | ||
| } |
There was a problem hiding this comment.
TOCTOU window between the symlink/regular-file check and the permission repair: symlink_metadata(path), metadata(path), and set_permissions(path) each re-resolve path, so a local attacker with write access to the directory can swap the file (e.g. with a symlink or hardlink) between the check and the repair — or after the repair but before Sqlite opens it by path. The module docs claim symlinks are "rejected rather than followed", but this guarantee is not race-free. Consider pinning the inode instead: open the existing file with OpenOptionsExt::custom_flags(libc::O_NOFOLLOW) (plus write access), then fchmod on the held handle (e.g. via std::os::unix::fs::FileExt::set_permissions), so the check and repair operate on the same inode. Alternatively, soften the documentation to state the guarantee only holds absent a concurrent local attacker.
Suggestion:
| 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))?; | |
| } | |
| let file = std::fs::OpenOptions::new() | |
| .write(true) | |
| .custom_flags(libc::O_NOFOLLOW) | |
| .open(path)?; | |
| // All subsequent operations act on the pinned inode via the held handle. | |
| let meta = file.metadata()?; | |
| if !meta.is_file() { | |
| return Err(std::io::Error::new( | |
| std::io::ErrorKind::AlreadyExists, | |
| "database path exists but is not a regular file", | |
| )); | |
| } | |
| if meta.permissions().mode() & 0o077 != 0 { | |
| file.set_permissions(std::fs::Permissions::from_mode(0o600))?; | |
| } |
| // 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()); |
There was a problem hiding this comment.
This test discards the Result of the re-open (let _ = ...), so it cannot distinguish "the repair logic intentionally preserved the private 0o400 mode" from "open failed early (e.g. before any chmod) and nothing was touched". If the repair branch in create_private_file regressed (e.g. always overwriting to 0o600 before attempting open), a future failure mode where open errors before chmod would still pass here. Consider asserting on the error kind (or successfully opening after temporarily restoring write permission) so the preservation path is actually exercised.
Suggestion:
| // 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()); | |
| let result = Sqlite::new(&db_path).open(make_desc()); | |
| // The permissions must be preserved regardless of the open outcome. | |
| assert_eq!( | |
| mode(&db_path), | |
| 0o400, | |
| "already private permissions must not be overwritten" | |
| ); | |
| // If the open happened to succeed, the Result should still be well-formed. | |
| if let Ok(db) = result { | |
| drop(db); | |
| } |
| 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()); |
There was a problem hiding this comment.
This test only asserts that open() fails, but Sqlite would fail to open target.db anyway since it is an empty, non-database file, regardless of whether the symlink rejection exists. The test therefore passes even if the symlink check in create_private_file were removed, so it does not verify the security property it claims to. Assert on the specific error (e.g. check that the error kind/message matches the "not a regular file" rejection) or create a valid Sqlite database as the symlink target so only the rejection can cause the failure.
Suggestion:
| 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()); | |
| 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 on the | |
| // specific rejection, not just any open failure (Sqlite would also fail on | |
| // an empty target file even without the check). | |
| let err = Sqlite::new(&db_path).open(make_desc()).unwrap_err(); | |
| assert!(err.to_string().contains("not a regular file"), "unexpected error: {err}"); |
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.
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.
- 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.
cdad745 to
00eb68a
Compare
| // 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) |
There was a problem hiding this comment.
TOCTOU race between the regular-file/permission checks and the subsequent open by Sqlite: an attacker with write access to the directory can swap the checked file (e.g. replace it with a symlink) after symlink_metadata/metadata succeed but before Sqlite opens it, so the symlink rejection and the 0600 repair can be applied to a file that is no longer the one Sqlite uses. Consider documenting this as a known limitation in the module security notes (like the auxiliary-files caveat), or re-verifying the path (e.g. comparing symlink_metadata before/after) if stronger guarantees are needed.
Suggestion:
| // 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) | |
| // Note: this check is subject to a TOCTOU race with an attacker who can write | |
| // to the containing directory; see the module security notes. | |
| 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) |
2 participants
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-phraseis enabled — the complete BIP-39 mnemonic and optional passphrase, in plaintext. With the common0022umask:0755(create_dir_all)0644Any other local unprivileged user could copy a traversable database and recover full signing authority without the victim's RPC credentials.
Fix (
storage/sqlite, Unix)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.0600(pre-created viaOpenOptions::create_new().mode(0o600)before Sqlite opens it), so it is never observable with looser permissions, even momentarily.0600.Non-Unix platforms keep the previous behavior.
Tests
Two new Unix-only tests in
storage/sqlite:0700, file06000644file → repaired to0600on openVerification
cargo test -p storage-sqlite: 27/27 ✓,cargo test -p wallet-controller: 24/24 ✓,cargo test -p wallet: 114/114 ✓./do_checks.shgreen under the CI toolchain (1.92.0)