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) {