From 341828534fdd3c97ac0b2e89e0ef2a3ccac31c89 Mon Sep 17 00:00:00 2001 From: ualtinok Date: Fri, 31 Jul 2026 19:55:28 +0200 Subject: [PATCH] store: make the database, its WAL and lease files owner-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SQLite sets no mode of its own, so open_sqlite left the file mode to the caller's umask and the shipped default was world-readable 0644. Measured on a real deployment: 11 of 11 module stores at 0644, including cortexkit-credentials. A crate that already decides WAL mode, busy timeout and foreign keys has taken responsibility for how the file behaves on disk. Leaving permissions to callers means the decision is made by the ambient umask, which is to say not made at all. A group-readable store is not a configuration this crate supports anyway: it hands out an exclusive single-writer lease, so an out-of-band reader is already outside the contract. The exposure, stated honestly rather than as an implied multi-user threat: a single-account host has no other human to read these files. What 0644 does expose them to is every process running as this user — every module, every worker, every tool — and anything that copies the tree: a backup, a restore, an install into a shared location, a container bind-mount. THE WAL IS THE HALF THAT GETS MISSED. Recently committed rows live there until a checkpoint, so protecting only the database leaves the newest data readable while the database file itself reads as correct. On the measured host the WALs are routinely larger than their databases — alfonso-core's is 23MB. Lease files are hardened too, in cortexkit-lease where they are created. Their exposure is integrity rather than privacy: the lease carries the persisted epoch that is the single-writer fence token, so a writable lease file lets a stale writer's fence be forged. Applied on OPEN rather than only at creation, because every store already deployed was created at 0644 — a creation-time-only fix protects exactly the installations with no history. A path that is not a regular file is refused rather than adjusted: following a symlink would chmod a file the caller never named, which is a privilege-escalation primitive wearing a hardening step's clothes. Each assertion is mutation-proved separately. Worth recording that the first version of the WAL test could not fail: it used a FIRST open, where SQLite creates the WAL inheriting the database's already-corrected mode. Dropping '-wal' from the protected suffixes passed it. The test now reopens a store with a leftover permissive WAL — the state an unclean shutdown leaves behind, and the only one where a permissive WAL can be waiting at open time. --- crates/cortexkit-lease/src/lib.rs | 154 ++++++++++++++++++++++++++++++ crates/cortexkit-store/src/lib.rs | 117 ++++++++++++++++++++++- 2 files changed, 270 insertions(+), 1 deletion(-) diff --git a/crates/cortexkit-lease/src/lib.rs b/crates/cortexkit-lease/src/lib.rs index f316ecd..da8f95b 100644 --- a/crates/cortexkit-lease/src/lib.rs +++ b/crates/cortexkit-lease/src/lib.rs @@ -35,6 +35,55 @@ use std::{ use fs2::FileExt; +/// Force owner-only permissions on a file this process owns the lifecycle of. +/// +/// Files created through `File::create` or `OpenOptions::create` get their mode +/// from the process umask, which on a default system means `0644` — readable by +/// every other account and, more to the point, by every other process running +/// as this user. That is the exposure that actually exists on a +/// single-account machine: every module, every worker, every tool, plus +/// anything that copies the tree (a backup, a restore, an `install` into a +/// shared location, a container bind-mount). +/// +/// Applied on OPEN rather than only at creation, because a file that already +/// exists carries whatever mode it was given — by an older build, a copy, or a +/// restore. A creation-time-only fix protects exactly the installations with no +/// history and leaves the ones that matter permissive forever. +/// +/// A path that is not a regular file is REFUSED rather than adjusted. Following +/// a symlink here would chmod a file the caller never named, which is a +/// privilege-escalation primitive wearing a hardening step's clothes. +/// +/// A missing file is not an error: callers pass optional sidecars (a WAL that +/// exists only while the journal is active) and the absence of a file is +/// nothing to protect. +pub fn protect_file(path: &std::path::Path) -> std::io::Result<()> { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let metadata = match std::fs::symlink_metadata(path) { + Ok(metadata) => metadata, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(error) => return Err(error), + }; + if !metadata.is_file() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "{} is not a regular file; refusing to change its permissions", + path.display() + ), + )); + } + if metadata.permissions().mode() & 0o777 != 0o600 { + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?; + } + } + #[cfg(not(unix))] + let _ = path; + Ok(()) +} + /// Identifies the thing being single-writer-guarded, namespaced so distinct /// modules cannot collide on a shared lease root. /// @@ -196,6 +245,11 @@ impl LeaseStore for FileLeaseStore { .truncate(false) .open(&path) .map_err(LeaseError::Io)?; + // The lease is the single-writer fence, so a world-WRITABLE lease file is + // an integrity question rather than a privacy one: anything able to write + // the persisted epoch can forge the fence token that readers use to + // detect a stale writer. + protect_file(&path).map_err(LeaseError::Io)?; // Liveness gate: a live holder still owns the lock, so the try-lock fails // with the OS "contended" error. @@ -230,6 +284,7 @@ impl LeaseStore for FileLeaseStore { .truncate(false) .open(&path) .map_err(LeaseError::Io)?; + protect_file(&path).map_err(LeaseError::Io)?; // Shared liveness gate: only an exclusive holder contends; other shared // holders coexist. On unix this is flock(LOCK_SH|LOCK_NB); on Windows, @@ -323,6 +378,105 @@ mod tests { (FileLeaseStore::new(&dir), dir) } + /// An acquired lease file is owner-only on disk, including one that already + /// exists with a permissive mode. + /// + /// The pre-existing half is the case that bites: every lease already on a + /// deployed machine was created at the umask default, so correcting only at + /// creation time would protect exactly the installations with no history. + /// + /// The lease is the single-writer FENCE, so the exposure here is integrity + /// rather than privacy — anything able to write the persisted epoch can + /// forge the token readers use to detect a stale writer. + /// + /// Mutation-proved: removing the `protect_file` call from `acquire` fails + /// this on the pre-existing case. + #[cfg(unix)] + #[test] + fn an_acquired_lease_file_is_owner_only() { + use std::os::unix::fs::PermissionsExt; + + let (store, dir) = tmp_store(); + let k = key("perm"); + + // Reproduce a lease left behind by an older build at the umask default. + std::fs::create_dir_all(&dir).expect("create lease dir"); + let path = store.lease_path(&k); + std::fs::write(&path, b"").expect("pre-create lease file"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)) + .expect("set permissive mode"); + + let guard = store.acquire(&k).expect("acquire"); + let mode = std::fs::metadata(&path) + .expect("stat lease") + .permissions() + .mode() + & 0o777; + assert_eq!( + mode, 0o600, + "the lease file stayed group/world writable at {mode:o}" + ); + + drop(guard); + let _ = std::fs::remove_dir_all(&dir); + } + + /// `protect_file` refuses a symlink rather than following it. + /// + /// Following one would change the mode of a file the caller never named, + /// which is a privilege-escalation primitive wearing a hardening step's + /// clothes. The assertion is that the TARGET's mode is unchanged, not + /// merely that an error came back — an implementation could chmod the + /// target and still return Err. + #[cfg(unix)] + #[test] + fn protect_file_refuses_a_symlink_and_leaves_its_target_untouched() { + use std::os::unix::fs::PermissionsExt; + + let dir = std::env::temp_dir().join(format!( + "cortexkit-lease-symlink-{}-{}", + std::process::id(), + fnv1a_hex(&format!("{:?}", std::time::Instant::now())) + )); + std::fs::create_dir_all(&dir).expect("create dir"); + let target = dir.join("target"); + std::fs::write(&target, b"not mine").expect("write target"); + std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o644)) + .expect("set target mode"); + let link = dir.join("link"); + std::os::unix::fs::symlink(&target, &link).expect("symlink"); + + assert!( + protect_file(&link).is_err(), + "a symlink must be refused rather than followed" + ); + let mode = std::fs::metadata(&target) + .expect("stat target") + .permissions() + .mode() + & 0o777; + assert_eq!( + mode, 0o644, + "the symlink target was chmod-ed through the link" + ); + + let _ = std::fs::remove_dir_all(&dir); + } + + /// A path that does not exist is not an error: callers pass optional + /// sidecars (a WAL that exists only while the journal is active), and the + /// absence of a file is nothing to protect. Without this, a first open of a + /// fresh database would fail on its missing WAL. + #[test] + fn protect_file_ignores_a_missing_path() { + let missing = std::env::temp_dir().join(format!( + "cortexkit-lease-absent-{}-{}", + std::process::id(), + fnv1a_hex(&format!("{:?}", std::time::Instant::now())) + )); + assert!(protect_file(&missing).is_ok()); + } + #[test] fn acquire_then_second_holder_is_rejected() { let (store, dir) = tmp_store(); diff --git a/crates/cortexkit-store/src/lib.rs b/crates/cortexkit-store/src/lib.rs index e8cfd0d..b4f0db7 100644 --- a/crates/cortexkit-store/src/lib.rs +++ b/crates/cortexkit-store/src/lib.rs @@ -102,7 +102,7 @@ mod sqlite_backend { time::{Duration, SystemTime, UNIX_EPOCH}, }; - use cortexkit_lease::{FileLeaseStore, LeaseHandle}; + use cortexkit_lease::{protect_file, FileLeaseStore, LeaseHandle}; use rusqlite::Connection; /// A lease-guarded, migrated sqlite store. Holds the single-writer lease for @@ -291,6 +291,34 @@ mod sqlite_backend { conn.pragma_update(None, "foreign_keys", "ON") .map_err(|e| StoreError::Backend(e.to_string()))?; + // Owner-only, decided here rather than left to the caller's umask. + // + // SQLite sets no mode of its own, so without this the shipped default is + // world-readable `0644` — measured across every module store on a real + // deployment. A crate that already decides WAL mode, busy timeout and + // foreign keys has taken responsibility for how this file behaves on + // disk; leaving permissions to callers means the decision is made by the + // ambient umask, which is to say not made at all. + // + // The WAL and SHM siblings are the half that gets missed. Recently + // committed rows live in the WAL until a checkpoint, so protecting only + // the database file leaves the NEWEST data permissive while the database + // itself reads as correct — and on real hosts the WAL is routinely + // larger than the database. + // + // Ordering: after the pragma that ENABLES WAL, so the sibling files + // exist to be protected on a first open rather than being created + // unprotected immediately afterwards. + // + // A group-readable store is not a configuration this crate supports: it + // hands out an exclusive single-writer lease, so an out-of-band reader + // is already outside the contract. That need is a read replica or an + // export operation, not a looser file mode. + for suffix in ["", "-wal", "-shm"] { + protect_file(Path::new(&format!("{path}{suffix}"))) + .map_err(|e| StoreError::Backend(e.to_string()))?; + } + Ok(SqliteStore { conn: Mutex::new(conn), epoch, @@ -371,6 +399,93 @@ pub use sqlite_backend::{open_sqlite, SqliteStore}; mod tests { use super::*; + /// The database file AND its WAL sibling are owner-only on disk after + /// `open_sqlite`, including a database that already exists permissively. + /// + /// Two properties, asserted separately because they fail independently. + /// SQLite sets no mode, so without this the shipped default is `0644`. + /// And the WAL is the half that gets missed: recently committed rows live + /// there until a checkpoint, so protecting only the database leaves the + /// NEWEST data readable while the database file itself looks correct. + /// + /// Asserts the mode ON DISK rather than that `open_sqlite` returned Ok — a + /// hardening step that silently did nothing would still return Ok. + /// + /// The scenario is REOPENING an already-deployed store, which is the only + /// shape in which the WAL assertion means anything. A first open cannot + /// exercise it: SQLite creates a fresh WAL inheriting the database's mode, + /// so by then the database is already `0600` and the WAL follows for free. + /// A permissive WAL only exists because a PREVIOUS process wrote one under + /// the old umask — exactly the state every deployed machine is in. + /// + /// Mutation-proved, both suffixes independently: dropping `""` fails the + /// database assertion, dropping `"-wal"` fails the WAL assertion, and + /// neither is carried by the other. An earlier version of this test used a + /// first open and the `"-wal"` mutation SURVIVED it — the assertion was + /// there, it just could not fail. + #[cfg(unix)] + #[test] + fn reopening_a_permissive_store_protects_the_database_and_its_wal() { + use std::os::unix::fs::PermissionsExt; + + let (root, descriptor) = tmp(); + let StorageBackend::Sqlite { path } = &descriptor.backend else { + panic!("sqlite descriptor"); + }; + let path = std::path::PathBuf::from(path); + let wal = std::path::PathBuf::from(format!("{}-wal", path.display())); + + // First open: create a real database, then close it. + { + let store = open_sqlite(&descriptor).expect("first open"); + store + .migrate( + "perm", + &[Migration { + version: 1, + statements: "CREATE TABLE t (k TEXT);", + }], + ) + .expect("migrate"); + } + + // A clean close checkpoints and REMOVES the WAL, so one has to be put + // back deliberately. That is not artificial: a WAL surviving on disk is + // precisely what an unclean shutdown leaves behind, and it is the only + // state in which a permissive WAL can be waiting at open time. + std::fs::write(&wal, b"").expect("leave a WAL behind"); + + // Reproduce the deployed state: both files permissive, as every store + // created before this hardening actually is on disk. + for file in [&path, &wal] { + std::fs::set_permissions(file, std::fs::Permissions::from_mode(0o644)) + .expect("set permissive mode"); + } + + let store = open_sqlite(&descriptor).expect("reopen"); + + let mode = |p: &std::path::Path| { + std::fs::metadata(p) + .unwrap_or_else(|error| panic!("stat {}: {error}", p.display())) + .permissions() + .mode() + & 0o777 + }; + assert_eq!( + mode(&path), + 0o600, + "the database stayed group/world readable on reopen" + ); + assert_eq!( + mode(&wal), + 0o600, + "the WAL stayed group/world readable while the database looked correct" + ); + + drop(store); + let _ = std::fs::remove_dir_all(&root); + } + /// A unique temp root + a descriptor whose sqlite file lives under it. The /// lease is derived from the db path (its parent), so no separate lease dir. fn tmp() -> (std::path::PathBuf, StorageDescriptor) {