From b055980531e86a6ca7c990a3ce80be00389cbf0e Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Tue, 4 Aug 2026 08:04:20 -0700 Subject: [PATCH 1/2] fix: close the four remaining trust-boundary gaps from the security sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 2026-07-29 review's cross-cutting security sweep found five places where a value read across the on-disk trust boundary was acted on before it was bounded. SECURITY-SWEEP-1 (Argon2 cost parameters) landed in PR #128; this closes the other four. The unifying defect is treating a valid XXH3 page checksum as if it authenticated the bytes. It does not: XXH3 is non-cryptographic and publicly recomputable, so an attacker with byte-level control over the file re-stamps it for free. Three of the four hazards below also bypass the poison model outright, because an allocator abort and a stack overflow are not Rust errors and cannot be intercepted by a Result-based contract. SECURITY-SWEEP-2 (overflow.rs): Overflow::read sized a Vec directly from the disk-controlled u64 at bytes 16..24. The preceding guards reject only a page that is not Overflow-typed, which is exactly the case a crafted file avoids; a forged u64::MAX therefore reached handle_alloc_error and aborted the process from a plain Chisel::read. Bound it against next_page_id * OVERFLOW_PAYLOAD. The ceiling is the allocator high-water mark rather than the file length so a read-your-own-writes of a large value inside the writing transaction still works. SECURITY-SWEEP-3 (transaction/recovery.rs, freemap_tree.rs): freemap_depth was copied out of the superblock into FreeMapTree::from_roots unvalidated, while the handle table and membership index both cap theirs. A forged depth drives scan_node's per-level recursion into a stack overflow and cow_descend into an O(depth^2) loop that materializes a page per absent child on the ordinary commit path. Gate it at open_existing, and add the depth check to the tree's own entry points as defense in depth, matching what the other two radixes do. The freemap's comment claimed capacity() saturation made this fail closed; saturation only prevents arithmetic overflow, it rejects nothing. SECURITY-SWEEP-4 (page_io.rs, spillway.rs): both files were created at 0666 & ~umask. The spillway is the sharper case — its path is derived from the database path, it is created lazily mid-transaction under cache pressure, and it is opened with truncate(true) on the assumption that pre-existing content is garbage from a crashed run. That assumption fails for a symlink planted by another local user, whose target would then be truncated to zero. Create both at 0600 and open the sidecar O_NOFOLLOW. Encryption does not help here: the hazard is the truncate, not the contents. SECURITY-SWEEP-5 (handle.rs, bench): the bench adapter reinterpreted &[Identifier] as &[Handle] on the strength of both being repr(transparent), guarded by const assertions that cannot detect removal of repr(transparent) — a repr(Rust) struct Handle(u64) has the same size and align. Rust offers no stable way to assert the attribute, so the guard was unclosable. Replace the transmute with a safe collect: it bought one Vec per delete_many against an operation that performs three fsyncs. Five tests cover the hazards, each forging bytes and re-stamping the checksum so the file passes validation exactly as an attacker's would. The freemap one points its root at a never-allocated page, so the only way to get a typed error rather than a read failure is for the guard to run before the first cache.get — an unguarded traversal at that depth is a stack overflow, not a failure a test harness could report. Closes #102. --- bench/src/chisel_engine.rs | 24 +++-- src/freemap_tree.rs | 93 ++++++++++++++++- src/handle.rs | 19 +++- src/overflow.rs | 40 +++++++- src/page_io.rs | 29 ++++-- src/recovery_tests.rs | 196 ++++++++++++++++++++++++++++++++++++ src/spillway.rs | 37 +++++-- src/transaction/recovery.rs | 29 ++++++ 8 files changed, 432 insertions(+), 35 deletions(-) diff --git a/bench/src/chisel_engine.rs b/bench/src/chisel_engine.rs index 96f6f1e..2d379c9 100644 --- a/bench/src/chisel_engine.rs +++ b/bench/src/chisel_engine.rs @@ -89,15 +89,21 @@ impl Engine for ChiselEngine { } fn delete_many(&mut self, ids: &[Identifier]) -> EngineResult<()> { - // SAFETY: Identifier and chisel::Handle are both - // #[repr(transparent)] over u64, so a slice of Identifier and a - // slice of Handle have identical layout. The borrow ends with - // this call; no aliasing concern; no 'static lifetime escapes. - // Saves the per-call Vec allocation that the safe-collect form - // would require (audit F5). - let handles: &[chisel::Handle] = - unsafe { std::slice::from_raw_parts(ids.as_ptr() as *const chisel::Handle, ids.len()) }; - Ok(self.db.delete_many(handles)?) + // This used to reinterpret `&[Identifier]` as `&[chisel::Handle]` + // through `slice::from_raw_parts`, on the strength of both types being + // `#[repr(transparent)]` over u64. The layout guarantee is real, but + // nothing enforced it across the crate boundary: chisel's const + // assertions check only size and align, which a `#[repr(Rust)] + // struct Handle(u64)` also satisfies. Deleting `#[repr(transparent)]` + // would have left both crates building green while this line became UB. + // + // The transmute bought one Vec allocation per `delete_many` call, + // against an operation that performs three fsyncs. That is far below the + // floor this harness can measure, so the safe form costs nothing real + // and removes an unsafe block that no compiler check was guarding. + let handles: Vec = + ids.iter().map(|id| chisel::Handle::from(id.0)).collect(); + Ok(self.db.delete_many(&handles)?) } fn file_size_bytes(&self) -> EngineResult { diff --git a/src/freemap_tree.rs b/src/freemap_tree.rs index 9d0ea02..a9bc845 100644 --- a/src/freemap_tree.rs +++ b/src/freemap_tree.rs @@ -58,9 +58,14 @@ const PTRS_PER_INTERIOR: usize = (CHECKSUM_OFFSET - DATA_PAGE_HEADER_SIZE) / PTR // The leaf fans out to 2^16 and each level multiplies by ~2^10, so depth 5 // already covers 2^16 * 2^50 = 2^66 > u64::MAX page ids — the bound is 5 // (distinct from the membership tree's 6, which has no 2^16 leaf factor). A -// spine or stored depth claiming deeper is corrupt; capacity() saturates so a -// bad on-disk depth fails closed (rejects the descent) rather than overflowing. -const MAX_DEPTH: u32 = 5; +// spine or stored depth claiming deeper is corrupt. capacity() saturates, but +// saturation only prevents arithmetic overflow — on its own it rejects nothing +// and does NOT make a bad on-disk depth fail closed. The actual rejection is +// the `freemap_depth > MAX_DEPTH` gate in `open_existing`, which is what keeps +// `scan_node`'s per-level recursion and `cow_descend`'s O(depth^2) loop from +// being driven by a hostile superblock. `pub(crate)` so that gate can name the +// constant instead of repeating the literal. +pub(crate) const MAX_DEPTH: u32 = 5; fn read_child(buf: &[u8; PAGE_SIZE], index: usize) -> u64 { let off = DATA_PAGE_HEADER_SIZE + index * PTR_SIZE; @@ -189,11 +194,37 @@ impl FreeMapTree { span } + // Reject an out-of-range `depth` before any traversal begins. `open_existing` + // already gates `freemap_depth > MAX_DEPTH` at the trust boundary, so this is + // defense in depth — it matches what HandleTable::recover_depth and + // RadixU64/MembershipIndex already do at every entry point, and it means a + // future path that builds a tree from roots WITHOUT passing through the open + // gate (a savepoint rewind, a rollback to a restored root) cannot resurrect + // the hazard. + // + // It must run BEFORE the first `cache.get`, not merely before the recursion: + // the traversals happen to fail type validation first for some root shapes, + // which is luck, not a guarantee. A FreeMapInterior root with a + // self-referential child pointer passes every type check and is precisely the + // shape that drives scan_node into a stack overflow and cow_descend into its + // O(depth^2) page-materializing loop. + // + // Note the saturating arithmetic above does NOT stand in for this: saturation + // keeps `capacity()`/`child_span()` from overflowing, but a saturated span + // simply yields child index 0 — it rejects nothing. + fn check_depth(&self) -> Result<()> { + if self.depth > MAX_DEPTH { + return Err(ChiselError::CorruptPage { page_id: self.root }); + } + Ok(()) + } + /// Descend (read-only) to the leaf covering `id`. Returns the leaf page id, /// or `None` if `id` is past the tree's reach OR any subtree on the path is /// absent (a zero child pointer = "that whole range is all in use"). Every /// page read is type-validated. fn find_leaf(&self, cache: &mut PageCache, id: u64) -> Result> { + self.check_depth()?; // Reject ids past the tree's reach. When capacity saturated to u64::MAX // (depth where the real capacity exceeds u64), every id is in reach, so // skip the guard — otherwise id == u64::MAX would be wrongly rejected. @@ -285,6 +316,7 @@ impl FreeMapTree { extend: &mut dyn FnMut(&mut PageCache) -> Result, leaf_op: &mut dyn FnMut(&mut [u8; PAGE_SIZE], u64), ) -> Result<()> { + self.check_depth()?; // COW the root first; the new root replaces self.root and the old one is // superseded. (grow has already ensured depth covers id, if needed.) // The root's position type follows depth exactly as the read paths see it: @@ -484,6 +516,7 @@ impl FreeMapTree { /// subtrees. Returns the global id (not leaf-local). Every page is type- /// validated on the way down. fn scan_from(&self, cache: &mut PageCache, lo: u64) -> Result> { + self.check_depth()?; self.scan_node(cache, self.root, self.depth, 0, lo) } @@ -547,6 +580,7 @@ impl FreeMapTree { &self, cache: &mut PageCache, ) -> Result> { + self.check_depth()?; let mut set = rustc_hash::FxHashSet::default(); if self.root != crate::page::PAGE_ID_NONE { self.collect_reachable(cache, self.root, self.depth, &mut set)?; @@ -882,4 +916,57 @@ mod tests { } } } + + // Defense in depth for the SECURITY-SWEEP-3 hazard. `open_existing` rejects + // an over-deep `freemap_depth` at the trust boundary; this pins the tree's + // own entry-point guards, so a path that builds a tree from roots without + // passing through that gate cannot reach the recursion. + // + // The root here is a page id that was never allocated, which is what makes + // the test meaningful: the ONLY way to get a typed error rather than a + // read failure is for check_depth to run before the first cache.get. That + // also keeps the test terminating — an unguarded traversal at this depth is + // a stack overflow, not a failure the harness could report. + #[test] + fn an_over_deep_depth_is_rejected_before_any_page_is_read() { + let mut c = make_cache(256); + let phantom_root = 4_242; + let bad_depth = MAX_DEPTH + 1; + + let assert_rejected = |label: &str, r: Result<()>| { + match r { + Err(ChiselError::CorruptPage { page_id }) => assert_eq!( + page_id, phantom_root, + "{label}: the guard must blame the tree root it refused to descend" + ), + other => panic!("{label}: expected CorruptPage, got {other:?}"), + }; + }; + + // Read paths. + let t = FreeMapTree::from_roots(phantom_root, bad_depth); + assert_rejected("is_free", t.is_free(&mut c, 7).map(|_| ())); + assert_rejected("reachable_pages", t.reachable_pages(&mut c).map(|_| ())); + + // Allocation path (scan_from), and the commit-path writer (cow_descend). + let mut t = FreeMapTree::from_roots(phantom_root, bad_depth); + let mut hint = 0u64; + assert_rejected( + "allocate_first", + t.allocate_first(&mut c, &mut hint, &mut extend).map(|_| ()), + ); + assert_rejected("mark_free", t.mark_free(&mut c, 7, &mut extend)); + + // A depth AT the bound is legal and must still be accepted — the guard + // is `>`, not `>=`. It fails on the phantom page instead, which proves + // it got past check_depth to the read. + let t = FreeMapTree::from_roots(phantom_root, MAX_DEPTH); + assert!( + !matches!( + t.is_free(&mut c, 7), + Err(ChiselError::CorruptPage { page_id }) if page_id == phantom_root + ), + "MAX_DEPTH itself must not be rejected by the depth guard" + ); + } } diff --git a/src/handle.rs b/src/handle.rs index 513672b..381d98d 100644 --- a/src/handle.rs +++ b/src/handle.rs @@ -26,11 +26,20 @@ use std::num::NonZeroU32; #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct Handle(u64); -// Pin the layout the bench adapter's `&[u64]` -> `&[Handle]` transmute depends -// on (bench/src/chisel_engine.rs). With this, dropping `#[repr(transparent)]` or -// adding a field fails THIS crate's build with a clear message, instead of -// turning the cross-crate transmute into silent UB the bench can't detect -// (review 2026-06-22). const assertions are evaluated at compile time. +// Layout tripwire for `#[repr(transparent)]` above, which the FFI surfaces +// depend on. +// +// Be precise about what these catch, because the previous comment was not: +// they fire if a field is ADDED (size changes) or alignment shifts. They do +// NOT catch removal of `#[repr(transparent)]` itself — a `#[repr(Rust)] +// struct Handle(u64)` has the same size and align, so both assertions still +// pass. Rust offers no stable way to assert `repr(transparent)`, so treat the +// attribute as the contract and these as a partial backstop. +// +// The bench adapter's `&[Identifier]` -> `&[Handle]` transmute, which these +// were originally written to guard, has been replaced with a safe collect +// (bench/src/chisel_engine.rs) precisely because that gap was unclosable — so +// no unsafe code currently rests on them. const _: () = { assert!( core::mem::size_of::() == core::mem::size_of::(), diff --git a/src/overflow.rs b/src/overflow.rs index 3c372ca..a6f2add 100644 --- a/src/overflow.rs +++ b/src/overflow.rs @@ -174,12 +174,42 @@ impl Overflow { page_id: first_page, }); } + // `total_length` is disk-controlled and, at this point, unauthenticated: + // the page checksum is XXH3, which is non-cryptographic and publicly + // recomputable, so an attacker with byte-level control over the file + // chooses this value freely. The type and zero-length guards above do + // NOT constrain it — they only reject a page that isn't Overflow-typed + // at all, which is exactly the case a crafted file avoids. + // + // Bound it against what the file could physically hold before it is + // used for anything. This single check covers both hazards: + // + // * the `Vec::with_capacity` below, which on a forged u64::MAX either + // panics with "capacity overflow" or reaches `handle_alloc_error` + // and ABORTS the process — bypassing the poison model entirely, from + // a plain `Chisel::read` on an untrusted file; and + // * `max_pages`, derived from the same value, which is the loop's only + // termination bound. A self-referential next_page link under a + // forged length would otherwise spin ~2^51 cache-hit iterations. + // + // The ceiling is the allocator's high-water mark, NOT the file length. + // A chain cannot span more pages than have ever been allocated, and + // `next_page_id` counts pages allocated in the current transaction + // too — pages that live in the cache and have not extended the file + // yet. Using `file_page_count` here instead would reject a perfectly + // valid read-your-own-writes of a large value inside the transaction + // that wrote it. + // + // Generous (it ignores the superblock, handle-table and data pages + // that must also be in there) but bounded by something an attacker + // cannot inflate without actually supplying the bytes. + let ceiling = (cache.next_page_id() as usize).saturating_mul(OVERFLOW_PAYLOAD); + if total_length > ceiling { + return Err(ChiselError::CorruptPage { + page_id: first_page, + }); + } let max_pages = total_length.div_ceil(OVERFLOW_PAYLOAD); - // Ordering matters: the wrong-type and zero-length guards above run - // BEFORE this allocation, so an untrusted `total_length` (e.g. a - // stale handle pointing at a non-overflow page whose bytes 16..24 - // read as u64::MAX) can never drive a speculative giant allocation - // here. The per-page loop guard alone would be too late. let mut result = Vec::with_capacity(total_length); let mut current_page = first_page; diff --git a/src/page_io.rs b/src/page_io.rs index 618f6c9..95142aa 100644 --- a/src/page_io.rs +++ b/src/page_io.rs @@ -125,6 +125,14 @@ impl PageIo { /// read-write path, so read-only opens of a missing file correctly fail /// rather than materializing an empty file. /// + /// On unix, a newly created database file is created with mode `0600` + /// (owner read/write only) rather than the process umask's default. This is + /// part of the contract, not an implementation detail: a caller may rely on + /// a database it creates not being readable by other users on the host. + /// The mode applies ONLY at creation — reopening an existing file never + /// alters permissions its owner has deliberately set, so widening access is + /// the owner's call to make with `chmod`. + /// /// The exclusive flock is taken even for `read_only` opens. This is /// intentional: even a reader needs to block concurrent writers, because /// shadow paging means a writer could be mid-commit (old superblock @@ -135,12 +143,21 @@ impl PageIo { let mut file = if read_only { OpenOptions::new().read(true).open(path)? } else { - OpenOptions::new() - .read(true) - .write(true) - .create(true) - .truncate(false) - .open(path)? + let mut opts = OpenOptions::new(); + opts.read(true).write(true).create(true).truncate(false); + // Create at 0600 rather than 0666 & ~umask (typically 0644). A + // storage engine's file holds whatever the caller stored in it; on + // a shared host the default made every value in a plaintext + // database world-readable, which is a surprising default for a + // crate that also ships at-rest encryption. + // + // `mode` applies ONLY when the file is created, so reopening an + // existing database never changes permissions the owner has + // deliberately set — this tightens new files without migrating old + // ones. + #[cfg(unix)] + std::os::unix::fs::OpenOptionsExt::mode(&mut opts, 0o600); + opts.open(path)? }; Self::try_lock(&file)?; // I51: seed the page-count cache from the current file length. diff --git a/src/recovery_tests.rs b/src/recovery_tests.rs index b0a6716..e608316 100644 --- a/src/recovery_tests.rs +++ b/src/recovery_tests.rs @@ -1716,3 +1716,199 @@ fn tampered_key_slot_cost_params_do_not_reach_the_allocator() { "expected InvalidEncryptionKey (the slot is skipped as non-matching), got {err:?}" ); } + +// ── SECURITY-SWEEP hardening (2026-07-29 review) ──────────────────────── +// +// These three model the review's stated threat: the attacker controls the +// on-disk bytes and can recompute the XXH3 page checksum, which is +// non-cryptographic and publicly computable. Each previously reached a +// process-level failure — an allocator abort, a stack overflow — that the +// poison model cannot intercept, because neither is a Rust error. The +// assertions are therefore "returns a typed error" rather than any +// particular value: the property under test is that the process survives. + +#[test] +fn forged_overflow_total_length_is_corrupt_page_not_an_allocation_abort() { + // SECURITY-SWEEP-2. `Overflow::read` sized a Vec directly from the u64 at + // bytes 16..24 of an Overflow-typed page. A forged u64::MAX either panics + // with "capacity overflow" or reaches handle_alloc_error and ABORTS — + // from a plain Chisel::read on an untrusted file. + // + // The old comment claimed the preceding guards made this unreachable, but + // they only reject a page that is not Overflow-typed. A crafted file keeps + // the type tag valid, which is exactly the case that was unguarded. + let file = NamedTempFile::new().unwrap(); + let path = file.path().to_owned(); + + let handle; + { + let mut db = Chisel::open(&path, Default::default()).unwrap(); + db.begin().unwrap(); + handle = db.allocate(&vec![0xAB; 20_000]).unwrap(); + db.commit().unwrap(); + } + + let overflow_page = find_page_of_type(&path, PageType::Overflow); + { + let mut f = fs::OpenOptions::new() + .read(true) + .write(true) + .open(&path) + .unwrap(); + let mut buf = [0u8; PAGE_SIZE]; + f.seek(SeekFrom::Start(overflow_page * PAGE_SIZE as u64)) + .unwrap(); + f.read_exact(&mut buf).unwrap(); + // Keep the type tag intact; forge only the length, then re-stamp the + // checksum so the page passes validation exactly as an attacker's would. + buf[16..24].copy_from_slice(&u64::MAX.to_le_bytes()); + page::stamp_checksum(&mut buf); + f.seek(SeekFrom::Start(overflow_page * PAGE_SIZE as u64)) + .unwrap(); + f.write_all(&buf).unwrap(); + f.sync_all().unwrap(); + } + + let db = Chisel::open(&path, Default::default()).expect("open reads no overflow page"); + match db.read(handle) { + Err(ChiselError::CorruptPage { .. }) => {} + other => panic!("expected CorruptPage for a forged total_length, got {other:?}"), + } +} + +#[test] +fn forged_freemap_depth_is_rejected_at_open() { + // SECURITY-SWEEP-3. `freemap_depth` was copied from the superblock into + // FreeMapTree::from_roots with no bound, while the handle table and the + // membership index both cap theirs at MAX_DEPTH. A forged depth drives + // scan_node's per-level recursion into a stack overflow (SIGSEGV) and + // cow_descend into an O(depth^2) loop that extends the file per absent + // child — the latter on the ordinary commit path. + // + // Rewriting the superblock through Superblock::serialize keeps magic, + // checksum and every other field valid, so `freemap_depth` is the single + // variable under test. + let file = NamedTempFile::new().unwrap(); + let path = file.path().to_owned(); + { + let mut db = Chisel::open(&path, Default::default()).unwrap(); + db.begin().unwrap(); + db.allocate(b"payload").unwrap(); + db.commit().unwrap(); + } + + // Rewrite every superblock slot so slot selection cannot dodge the forgery. + let sb_count = { + let mut f = fs::File::open(&path).unwrap(); + let mut buf = [0u8; PAGE_SIZE]; + f.read_exact(&mut buf).unwrap(); + Superblock::deserialize(&buf) + .map(|sb| sb.superblock_count) + .unwrap_or(2) + }; + { + let mut f = fs::OpenOptions::new() + .read(true) + .write(true) + .open(&path) + .unwrap(); + for slot in 0..sb_count as u64 { + let mut buf = [0u8; PAGE_SIZE]; + f.seek(SeekFrom::Start(slot * PAGE_SIZE as u64)).unwrap(); + if f.read_exact(&mut buf).is_err() { + continue; + } + if let Some(mut sb) = Superblock::deserialize(&buf) { + sb.freemap_depth = 500_000; + f.seek(SeekFrom::Start(slot * PAGE_SIZE as u64)).unwrap(); + f.write_all(&sb.serialize()).unwrap(); + } + } + f.sync_all().unwrap(); + } + + match Chisel::open(&path, Default::default()) { + Err(ChiselError::CorruptSuperblock { .. }) => {} + Ok(_) => panic!("a freemap_depth of 500_000 must not open"), + Err(other) => panic!("expected CorruptSuperblock, got {other:?}"), + } +} + +#[test] +#[cfg(unix)] +fn a_new_database_file_is_created_private_to_its_owner() { + // SECURITY-SWEEP-4. The file was created at 0666 & ~umask — typically + // 0644 — so on a shared host every value in a plaintext database was + // world-readable. `mode` applies only on creation, so an existing + // database's permissions are left exactly as its owner set them. + use std::os::unix::fs::PermissionsExt; + + let dir = TempDir::new().unwrap(); + let path = dir.path().join("perm.db"); + { + let mut db = Chisel::open(&path, Default::default()).unwrap(); + db.begin().unwrap(); + db.allocate(b"private").unwrap(); + db.commit().unwrap(); + } + + let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode, 0o600, + "a freshly created database must not be group- or world-readable (got {mode:o})" + ); +} + +#[test] +#[cfg(unix)] +fn a_symlinked_spillway_path_is_refused_rather_than_followed() { + // SECURITY-SWEEP-4, the sharp half. The sidecar path is fully derived from + // the database path, so it is predictable; it is created lazily mid + // transaction under cache pressure; and it is opened with truncate(true) on + // the assumption that anything already there is garbage from a crashed + // prior run. That assumption fails for a symlink: a local user who can + // create entries in the database's directory could plant .spillway + // pointing at a file the owner can write, and the first spill would + // truncate it to zero. + // + // O_NOFOLLOW turns that into a failed open. The engine surfaces it as an + // error and poisons, which is the right answer to "someone is tampering + // with my sidecar path" — the point of the test is that the victim file is + // still intact afterwards. + use std::os::unix::fs::symlink; + + let dir = TempDir::new().unwrap(); + let db_path = dir.path().join("spill.db"); + let victim = dir.path().join("victim.txt"); + fs::write(&victim, b"important contents that must survive").unwrap(); + + // Plant the sidecar as a symlink to the victim before it is ever opened. + symlink(&victim, dir.path().join("spill.db.spillway")).unwrap(); + + // Tiny cache so a modest transaction is forced to spill. + let opts = Options::default() + .cache_max_bytes(16 * PAGE_SIZE as u64) + .spillway_max_bytes(64 * PAGE_SIZE as u64); + let mut db = Chisel::open(&db_path, opts).unwrap(); + db.begin().unwrap(); + let mut spill_attempted = false; + for _ in 0..400 { + if db.allocate(&vec![0x5A; PAGE_SIZE + 64]).is_err() { + spill_attempted = true; + break; + } + } + let _ = db.rollback(); + drop(db); + + assert!( + spill_attempted, + "expected the spill to be refused; if this stops firing the workload no \ + longer reaches ensure_spillway and the test has stopped testing anything" + ); + assert_eq!( + fs::read(&victim).unwrap(), + b"important contents that must survive", + "the symlink target must not have been truncated" + ); +} diff --git a/src/spillway.rs b/src/spillway.rs index bcaf261..3e02c20 100644 --- a/src/spillway.rs +++ b/src/spillway.rs @@ -130,13 +130,36 @@ impl Spillway { // in-flight cache state, not a "fix your path and retry" condition, so it // must poison rather than mislead the caller into continuing (review // 2026-06-22). - let file = OpenOptions::new() - .read(true) - .write(true) - .create(true) - .truncate(true) - .open(&path) - .map_err(ChiselError::IoError)?; + let mut opts = OpenOptions::new(); + opts.read(true).write(true).create(true).truncate(true); + // The sidecar needs BOTH guards, and for a sharper reason than the main + // database file does. + // + // Its path is fully derived from the database path, so it is predictable + // to anyone who can see the database. It is created lazily, mid + // transaction, whenever cache pressure forces a spill — not at open, + // where a caller might notice something wrong. And it is opened with + // `truncate(true)`, on the documented assumption that any pre-existing + // content is garbage from a crashed prior process. + // + // That assumption does not hold for a pre-existing SYMLINK. A local user + // who can create entries in the database's directory can plant + // `.spillway` pointing at any file the database owner can write; the + // next spill would then follow it and truncate the victim's file to + // zero. O_NOFOLLOW makes that open fail (ELOOP) instead — a poisoned + // handle, which is the correct outcome for "someone is tampering with my + // sidecar path". Encryption does not help here: the hazard is the + // truncate, not the contents. + // + // 0600 for the same reason as the main file, and it matters more here: + // spilled pages are uncommitted user data written without the caller + // ever asking for a second file to exist. + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.mode(0o600).custom_flags(libc::O_NOFOLLOW); + } + let file = opts.open(&path).map_err(ChiselError::IoError)?; Ok(Spillway { backing: Backing::File { file }, slots: HashMap::new(), diff --git a/src/transaction/recovery.rs b/src/transaction/recovery.rs index 81c3cec..1e03106 100644 --- a/src/transaction/recovery.rs +++ b/src/transaction/recovery.rs @@ -410,6 +410,35 @@ impl TransactionManager { }); } + // Same trust boundary, same reasoning: `freemap_depth` comes off the + // superblock unvalidated (`deserialize` reads the field, `validate` + // checks only checksum/magic/superblock_count) and is then handed + // verbatim to `FreeMapTree::from_roots`. Depth 5 already covers every + // u64 page id, so anything larger is corrupt by construction. + // + // This bound is load-bearing, not tidiness. Without it a hostile file + // reaches two process-killing paths that the poison model cannot + // intercept, because neither is a Rust error: + // + // * `scan_node` recurses once per level. A forged depth of ~500k plus + // one self-referential FreeMapInterior page overflows the stack — + // SIGSEGV — from any allocation that consults the freemap, and from + // `defrag`'s `collect_reachable`. + // * `cow_descend` loops over the depth and calls `child_span`, itself + // O(level), inside — O(depth^2), ~10^19 operations at u32::MAX — + // while materializing a page per absent child, extending the file + // without bound. That one fires on the ordinary commit path. + // + // The handle table and the membership index both already cap their + // depth this way; the freemap was the odd one out. Its own comment + // claimed `capacity()` saturation "fails closed (rejects the descent)", + // but saturation only prevents arithmetic overflow — it rejects nothing. + if sb.freemap_depth > crate::freemap_tree::MAX_DEPTH { + return Err(ChiselError::CorruptSuperblock { + defects: Vec::new(), + }); + } + // I29 write-gate: a file whose MINOR exceeds this binary's may contain // version-requiring page layouts we cannot safely write — we would // stamp pages at our older minor and drop the newer fields. Reads ARE From 5ddd9a3c490d1c4837ce43d201b5f8740a2c3463 Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Tue, 4 Aug 2026 13:40:52 -0700 Subject: [PATCH 2/2] fix: close the gaps an adversarial review found in the sweep fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The trust-boundary fixes in the parent commit were reviewed adversarially against their own threat model — attacker controls the file bytes and can re-stamp the XXH3 checksum for free. Two of them were incomplete in ways that left the original hazard reachable, just through a different door. SECURITY-SWEEP-2, second door. `Overflow::read` got a ceiling on the disk-controlled `total_length`; `Overflow::collect_chain_pages` reads the same u64 at bytes 16..24 and derives `max_pages` from it with no ceiling at all. That path is reached from `Chisel::delete` and `Chisel::update` rather than `Chisel::read`. A forged u64::MAX yields max_pages ~2.26e15, so a chain whose `next_page` points at itself pushes into an unbounded Vec until the allocator aborts — the same poison-model bypass, entered from a different public method. Apply the identical ceiling. Verified by removing the guard and watching the new test run past 90 seconds instead of failing. SECURITY-SWEEP-4, second direction. O_NOFOLLOW closes "planted symlink gets the victim's file truncated" but not "planted regular file gets adopted": a plain file is not a symlink, so the open succeeds, and `mode(0600)` applies only when the open CREATES the file. The planter keeps ownership and their 0666, and the engine then writes spilled pages — uncommitted user values — into a file they can read. Measured at 0666 before the fix. Create the sidecar with O_EXCL instead, and unlink a pre-existing entry only after confirming it is a plain file this uid owns with one link. That keeps the documented crash-debris behaviour for the case it was written for and fails closed for the case it was not; a re-plant between the unlink and the retry loses to O_EXCL. The same class of hole existed on the main database file, where a planted EMPTY file is a legitimate create target under the PR #127 rules, so it was adopted with its permissive mode intact; tighten any zero-length file the create path adopts. Also from the review: * `mark_free_growing` — the manager-facing entry point — evaluates `capacity()` in its `while` condition before `depth < MAX_DEPTH`, and `capacity()` loops `depth` times. At a forged u32::MAX that is ~4.3e9 saturating multiplies (tens of seconds) before the guard inside `mark_free` fires. It failed closed, but not promptly, and not the way the sibling entry points promise. Depth-check first. * The over-deep freemap depth was reported as `CorruptSuperblock` with an empty defect list: no diagnosis, and the wrong recoverability class — that variant is documented as reopen-recoverable via slot selection, but every sibling slot carries the same rejected value, so a reopen fails identically forever. Give it a typed `InvalidFreemapDepth { stored, max }` carrying the offending value, matching what the `page_size` check twenty lines above already does. * `handle.rs` still documented the bench transmute that the parent commit deleted, including in the two const-assert messages, which contradicted the corrected comment four lines above them. * The permission test asserted `mode == 0o600` exactly, which fails under a umask that masks owner bits on a file that is if anything more restrictive than required. Assert the property (no group/world bits) instead. ARCHITECTURE.md gains a section for the permission and sidecar contracts, which were user-visible behaviour changes introduced with no documentation. --- ARCHITECTURE.md | 10 ++- README.md | 2 +- src/error.rs | 31 ++++++- src/freemap_tree.rs | 20 +++++ src/handle.rs | 14 ++-- src/overflow.rs | 17 ++++ src/page_io.rs | 25 +++++- src/recovery_tests.rs | 156 +++++++++++++++++++++++++++++++++++- src/spillway.rs | 127 +++++++++++++++++++++++------ src/transaction/recovery.rs | 5 +- 10 files changed, 363 insertions(+), 44 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index ce7ce37..2cd70e3 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -118,7 +118,7 @@ flowchart BT | 2 | `page_io.rs` | Raw `pread`/`pwrite` of fixed-**stride** pages, exclusive `flock`, `fsync`, in-memory `Vec` backing. Tracks cumulative successful `fsync_calls` via `Cell`. Stride = `PAGE_SIZE` (plaintext) or `ENC_PAGE_SIZE` (encrypted), set once via `set_stride`. | The **only** module that touches the filesystem; crypto-agnostic (moves `stride`-byte blobs at `page_id * stride`). | | 2 | `lru.rs` | O(1) intrusive doubly-linked LRU index over `u64` page ids (`FxHashMap`-backed, I77). | Replaces the O(n) `VecDeque::retain` LRU; consumed only by `page_cache`. | | 3 | `page_cache.rs` | LRU cache over `PageIo`, dirty tracking, checksum validation on load, `PageCipher` seal/open at the I/O boundary, spillway overflow, `CacheFull`/`SpillwayFull` errors. Owns three `Cell` engine-activity counters (cache hits/misses, pages allocated) and aggregates them with `PageIo::fsync_count` into `counters()`. | Soft eviction at `max_pages`; dirty overflow spills to a sidecar (cap = `spillway_max_bytes`); `CacheFull` at strict `max_pages` when spillway disabled (`spillway_max_bytes=0`); checksums verified on disk LOAD only. | -| 3 | `spillway.rs` | Sidecar `.spillway` file for dirty pages the LRU is forced to spill. Per-slot XXH3 over `page_id ‖ payload`; crypto-agnostic (plaintext 8192-byte page or sealed 8232-byte blob). | Never `fsync`ed; truncated at open/commit/rollback — its content is always discardable uncommitted state. | +| 3 | `spillway.rs` | Sidecar `.spillway` file for dirty pages the LRU is forced to spill. Per-slot XXH3 over `page_id ‖ payload`; crypto-agnostic (plaintext 8192-byte page or sealed 8232-byte blob). | Never `fsync`ed; truncated at commit/rollback and re-created at open — its content is always discardable uncommitted state. Created `O_EXCL | O_NOFOLLOW` mode 0600; a pre-existing entry is unlinked only if it is a plain file this uid owns (see "File permissions" below). | | 4 | `freemap.rs` | Single-page bitmap primitive: `allocate_first` / `mark_free` on one `[u8; PAGE_SIZE]` buffer. | Pure buffer manipulation; no cache or I/O. Composed into the multi-page tree by `freemap_tree.rs`. | | 4 | `freemap_tree.rs` | COW radix tree of FreeMap leaves; the full multi-page freemap. | All structural COW pages sourced out-of-band (never from the bitmap); session-COW dedup (one COW per node per commit). | | 4 | `data_page.rs` | Slotted page layout (R1): slot directory grows forward, packed value data grows backward. The directory is append-only — nothing is ever reclaimed within a page. | Slot indices are immutable for the page's lifetime; the handle table stores `(page_id, slot_index)` and relies on it. There is no intra-page compaction: reclamation is whole-page. | @@ -589,6 +589,14 @@ Spillway slots carry their own per-slot XXH3 checksum over `page_id ‖ page_byt The no-spill commit cost is **3 fsyncs**: pre-drain flush (I28) + main-pages flush + superblock. The pre-drain handles a subtle interaction in the commit protocol (see [Commit protocol](#commit-protocol) step 1). +### File permissions and the sidecar path + +Both files Chisel creates are created mode **0600**. This is a contract, not an implementation detail: a caller may rely on a database it creates not being readable by other users on the host. The mode applies only at creation, so reopening an existing database never alters permissions its owner deliberately set — with one exception: a *zero-length* file adopted by the create path is tightened to 0600, because an empty file is not yet a database and a permissive empty file at the database path is more likely a squatter than a deliberate choice. + +The sidecar needs more than a mode, because its path is fully derived from the database path and is therefore predictable, and because it is created lazily *mid-transaction* under cache pressure rather than at open. A local user who can create entries in the database's directory could otherwise plant `.spillway` and either (a) point it at a file the database owner can write, which the old `truncate(true)` open would have zeroed, or (b) leave a world-readable regular file that Chisel would adopt — writing uncommitted user values somewhere the planter can read, since `mode` does not apply to a file the open did not create. + +So the sidecar is created with `O_EXCL | O_NOFOLLOW`. A pre-existing entry is unlinked and retried **only** when it is a plain file, owned by this uid, with exactly one link — the crash-debris case the lifecycle was written for. A symlink, a foreign-owned file, or an extra hard link is treated as tampering and surfaces as a fatal `IoError`, which poisons the handle. Encryption does not substitute for either guard: the hazards are the truncate and the file's ownership, not the confidentiality of the bytes. + ### Slot packing and overflow Values up to `MAX_INLINE_VALUE` (~`PAGE_BODY_SIZE`) are stored inline in a data-page slot. Larger values get an overflow chain, and that path allocates **no data page and no slot at all**: the `HandleEntry` itself carries `HandleFlags::Overflow`, with `page_id` pointing directly at the first chain page and `slot_index = 0` (an unused placeholder, not a real slot). `HandleFlags` is a field of the handle-table entry, not of a data-page slot-directory entry — the slot directory has no flag but `SLOT_FLAG_LIVE`. diff --git a/README.md b/README.md index 0002f92..a1b0546 100644 --- a/README.md +++ b/README.md @@ -372,7 +372,7 @@ let options = Options::default() **Fatal errors** — storage integrity is in question. Drop the handle and reopen. -`IoError`, `ChecksumMismatch`, `CorruptSuperblock`, `FileSizeMismatch`, `LockFailed`, `UnsupportedFormatVersion`, `UnsupportedPageSize`, `CorruptPage`, `InvalidPageId`, `DecryptionFailed`. +`IoError`, `ChecksumMismatch`, `CorruptSuperblock`, `FileSizeMismatch`, `LockFailed`, `UnsupportedFormatVersion`, `UnsupportedPageSize`, `InvalidFreemapDepth`, `CorruptPage`, `InvalidPageId`, `DecryptionFailed`. `DecryptionFailed { page_id }` is fatal: an AEAD authentication failure while decrypting an already-read page means the ciphertext or session key can no longer be trusted, so it poisons the handle exactly like `ChecksumMismatch` (see the poison model below). It is distinct from the operational `InvalidEncryptionKey`, which fires at open time when the supplied key unwraps no key slot — before any data page is served. diff --git a/src/error.rs b/src/error.rs index 2151417..bde271b 100644 --- a/src/error.rs +++ b/src/error.rs @@ -159,6 +159,23 @@ pub enum ChiselError { stored: u32, compiled: u32, }, + // Raised at open time when the superblock's `freemap_depth` exceeds the + // depth the freemap radix can represent. Depth 5 already spans every u64 + // page id, so a larger value cannot have been written by any correct + // binary — it is a corrupt or forged field. + // + // Deliberately NOT `CorruptSuperblock`: that variant means "no readable + // superblock at all" and is documented as recoverable by reopening, + // because slot selection may pick a different slot next time. Here the + // superblock parsed and validated fine apart from this one field, and + // every sibling slot carries the same rejected depth — so a reopen + // returns the same error forever. It carries the offending value for the + // same reason `UnsupportedPageSize` does: a bare "corrupt" tells an + // operator nothing about which field to look at. + InvalidFreemapDepth { + stored: u32, + max: u32, + }, // Operational — the caller supplied the wrong key material or none, or // asked an unencrypted-only build to open an encrypted DB. The on-disk @@ -223,6 +240,7 @@ impl ChiselError { | ChiselError::CorruptPage { .. } | ChiselError::InvalidPageId { .. } | ChiselError::UnsupportedPageSize { .. } + | ChiselError::InvalidFreemapDepth { .. } | ChiselError::DecryptionFailed { .. } ) } @@ -313,6 +331,10 @@ impl fmt::Display for ChiselError { f, "page size mismatch: file was written with {stored}-byte pages, this build uses {compiled}-byte pages" ), + ChiselError::InvalidFreemapDepth { stored, max } => write!( + f, + "superblock declares freemap depth {stored}, which exceeds the maximum {max} (corrupt or forged superblock)" + ), ChiselError::NoEncryptionKey => write!( f, "database is encrypted but no encryption_key was supplied" @@ -571,6 +593,7 @@ mod tests { | ChiselError::CorruptPage { .. } | ChiselError::InvalidPageId { .. } | ChiselError::UnsupportedPageSize { .. } + | ChiselError::InvalidFreemapDepth { .. } | ChiselError::DecryptionFailed { .. } => true, } } @@ -616,6 +639,7 @@ mod tests { stored: 0, compiled: 0, }, + ChiselError::InvalidFreemapDepth { stored: 0, max: 0 }, ChiselError::NoEncryptionKey, ChiselError::InvalidEncryptionKey, ChiselError::EncryptionNotSupported, @@ -630,10 +654,13 @@ mod tests { "is_fatal() disagrees with the documented Fatal/Operational block for {e:?}" ); } - // Tripwire: exactly 10 variants are fatal today. If this count moves, the + // Tripwire: exactly 11 variants are fatal today. If this count moves, the // Fatal/Operational split changed — confirm that was intentional (it is a // breaking change for callers doing error-class matching, per the header). - assert_eq!(all.iter().filter(|e| e.is_fatal()).count(), 10); + // Last moved 10 -> 11 by InvalidFreemapDepth, which is fatal because a + // superblock field outside its representable range cannot be resolved by + // retrying: every sibling slot carries the same value. + assert_eq!(all.iter().filter(|e| e.is_fatal()).count(), 11); } // Phase 4: the three operational encryption errors are recoverable (the diff --git a/src/freemap_tree.rs b/src/freemap_tree.rs index a9bc845..673bf22 100644 --- a/src/freemap_tree.rs +++ b/src/freemap_tree.rs @@ -480,6 +480,13 @@ impl FreeMapTree { id: u64, extend: &mut dyn FnMut(&mut PageCache) -> Result, ) -> Result<()> { + // Depth-check FIRST. The `while` condition below evaluates `capacity()` + // before it evaluates `self.depth < MAX_DEPTH`, and `capacity()` loops + // `depth` times — so a forged depth of u32::MAX spins ~4.3e9 saturating + // multiplies (tens of seconds) before the guard inside `mark_free` ever + // runs. It does eventually fail closed, but "eventually" is not what the + // other entry points promise, and this is the manager-facing one. + self.check_depth()?; // Grow until capacity covers id. MAX_DEPTH covers all of u64, so the loop // terminates; grow() no-ops at MAX_DEPTH and capacity() saturates to // u64::MAX, breaking the loop. @@ -956,6 +963,19 @@ mod tests { t.allocate_first(&mut c, &mut hint, &mut extend).map(|_| ()), ); assert_rejected("mark_free", t.mark_free(&mut c, 7, &mut extend)); + // The manager-facing entry point, and the one the first version of this + // guard missed. Its `while` condition evaluates `capacity()` — which + // loops `depth` times — BEFORE it evaluates `depth < MAX_DEPTH`, so at + // u32::MAX it spun ~4.3e9 saturating multiplies before the guard inside + // mark_free fired. It reached the same verdict, but only after tens of + // seconds; a bounded-but-absurd delay is not what the other entry points + // promise. `u32::MAX` rather than `MAX_DEPTH + 1` is deliberate: at the + // small over-bound the missing check is invisible. + let mut t = FreeMapTree::from_roots(phantom_root, u32::MAX); + assert_rejected( + "mark_free_growing", + t.mark_free_growing(&mut c, 7, &mut extend), + ); // A depth AT the bound is legal and must still be accepted — the guard // is `>`, not `>=`. It fails on the phantom page instead, which proves diff --git a/src/handle.rs b/src/handle.rs index 381d98d..a83300f 100644 --- a/src/handle.rs +++ b/src/handle.rs @@ -19,15 +19,15 @@ use std::num::NonZeroU32; /// A stable, opaque chunk handle. Newtype over the engine's `u64` id. /// -/// `#[repr(transparent)]` is load-bearing, not decoration: the bench adapter -/// reinterprets a `&[u64]` slice as `&[Handle]` without copying, which is sound -/// only because `Handle` has identical layout to `u64`. +/// `#[repr(transparent)]` is kept deliberately so the type stays layout- +/// identical to `u64`, which is what lets a future FFI or zero-copy adapter +/// depend on it. Nothing does today — see the tripwire below for exactly what +/// the const assertions can and cannot enforce. #[repr(transparent)] #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct Handle(u64); -// Layout tripwire for `#[repr(transparent)]` above, which the FFI surfaces -// depend on. +// Layout tripwire for `#[repr(transparent)]` above. // // Be precise about what these catch, because the previous comment was not: // they fire if a field is ADDED (size changes) or alignment shifts. They do @@ -43,11 +43,11 @@ pub struct Handle(u64); const _: () = { assert!( core::mem::size_of::() == core::mem::size_of::(), - "Handle must stay layout-identical to u64 (bench transmute relies on repr(transparent))" + "Handle must stay layout-identical to u64 (repr(transparent) is the contract)" ); assert!( core::mem::align_of::() == core::mem::align_of::(), - "Handle must stay layout-identical to u64 (bench transmute relies on repr(transparent))" + "Handle must stay layout-identical to u64 (repr(transparent) is the contract)" ); }; diff --git a/src/overflow.rs b/src/overflow.rs index a6f2add..b2a50c9 100644 --- a/src/overflow.rs +++ b/src/overflow.rs @@ -293,6 +293,23 @@ impl Overflow { page_id: first_page, }); } + // The SAME disk-controlled `total_length` that `read` bounds, reached + // from `Chisel::delete` and `Chisel::update` instead of `Chisel::read`. + // It needs the identical ceiling and for the identical reason: it is the + // sole source of `max_pages`, which is this loop's only termination + // bound. A forged u64::MAX yields max_pages ~2.26e15, so a chain whose + // `next_page` points at itself pushes into `freed` until the allocator + // aborts — the process-killing outcome the poison model cannot intercept. + // + // See `read` for why the ceiling is `next_page_id` (the allocator + // high-water mark) rather than the file length: it must not reject a + // read-your-own-writes of a large value inside the writing transaction. + let ceiling = (cache.next_page_id() as usize).saturating_mul(OVERFLOW_PAYLOAD); + if total_length > ceiling { + return Err(ChiselError::CorruptPage { + page_id: first_page, + }); + } let max_pages = total_length.div_ceil(OVERFLOW_PAYLOAD); let mut freed = Vec::new(); diff --git a/src/page_io.rs b/src/page_io.rs index 95142aa..c207c11 100644 --- a/src/page_io.rs +++ b/src/page_io.rs @@ -157,7 +157,30 @@ impl PageIo { // ones. #[cfg(unix)] std::os::unix::fs::OpenOptionsExt::mode(&mut opts, 0o600); - opts.open(path)? + let f = opts.open(path)?; + // `mode` above applies only when the open CREATES the file, which + // leaves one gap in the contract this function documents. Since a + // zero-length file is a legitimate create target, another local user + // can pre-plant an empty world-readable file at the database path; + // Chisel would then adopt it and write every value into a file the + // planter can read, with the promised 0600 never applied. + // + // Close it by tightening any zero-length file we adopt. A file with + // no bytes is not yet a database, so there are no permissions its + // owner has deliberately set on a database to preserve. If the file + // belongs to someone else the fchmod fails EPERM and the open fails + // — the correct outcome for "something is squatting on my path". + // + // Non-empty files are untouched, exactly as documented: reopening an + // existing database never alters its permissions. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + if f.metadata()?.len() == 0 { + f.set_permissions(std::fs::Permissions::from_mode(0o600))?; + } + } + f }; Self::try_lock(&file)?; // I51: seed the page-count cache from the current file length. diff --git a/src/recovery_tests.rs b/src/recovery_tests.rs index e608316..e554656 100644 --- a/src/recovery_tests.rs +++ b/src/recovery_tests.rs @@ -1828,9 +1828,16 @@ fn forged_freemap_depth_is_rejected_at_open() { } match Chisel::open(&path, Default::default()) { - Err(ChiselError::CorruptSuperblock { .. }) => {} + // Not CorruptSuperblock: that variant means "no readable superblock at + // all" and is documented as reopen-recoverable via slot selection. This + // superblock parsed fine apart from one field, and every sibling slot + // carries the same rejected depth, so the error must name the field. + Err(ChiselError::InvalidFreemapDepth { stored, max }) => { + assert_eq!(stored, 500_000); + assert_eq!(max, crate::freemap_tree::MAX_DEPTH); + } Ok(_) => panic!("a freemap_depth of 500_000 must not open"), - Err(other) => panic!("expected CorruptSuperblock, got {other:?}"), + Err(other) => panic!("expected InvalidFreemapDepth, got {other:?}"), } } @@ -1852,10 +1859,15 @@ fn a_new_database_file_is_created_private_to_its_owner() { db.commit().unwrap(); } + // Assert the security property, not the exact bits. The kernel applies + // `mode & ~umask`, so a developer running with umask 0277 gets 0400 here — + // still private, but not equal to 0600. Testing `mode == 0o600` would fail + // for them on a file that is if anything MORE restrictive than required. let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777; assert_eq!( - mode, 0o600, - "a freshly created database must not be group- or world-readable (got {mode:o})" + mode & 0o077, + 0, + "a freshly created database must not be group- or world-accessible (got {mode:o})" ); } @@ -1912,3 +1924,139 @@ fn a_symlinked_spillway_path_is_refused_rather_than_followed() { "the symlink target must not have been truncated" ); } + +#[test] +#[cfg(unix)] +fn a_planted_regular_file_at_the_spillway_path_is_not_adopted() { + // The other half of SECURITY-SWEEP-4, missed by the first fix. O_NOFOLLOW + // closes the truncate direction (a planted SYMLINK) but not the disclosure + // direction: a planted REGULAR file is not a symlink, so the open succeeds, + // and `mode(0600)` applies only when the open creates the file. The planter + // keeps ownership and their permissive mode, and the engine then writes + // spilled pages — uncommitted user values — into a file they can read. + // + // The fix creates the sidecar exclusively and only unlinks a pre-existing + // entry that is a plain file we own. A foreign-owned plant cannot be + // distinguished from ours in a portable test (both are owned by the test + // user), so this asserts the property that IS observable and that the bug + // violated: whatever the engine ends up writing to must not carry the + // planted permissive mode. + use std::os::unix::fs::PermissionsExt; + + let dir = TempDir::new().unwrap(); + let db_path = dir.path().join("plant.db"); + let sidecar = dir.path().join("plant.db.spillway"); + + fs::write(&sidecar, b"planted").unwrap(); + fs::set_permissions(&sidecar, fs::Permissions::from_mode(0o666)).unwrap(); + + let opts = Options::default() + .cache_max_bytes(16 * PAGE_SIZE as u64) + .spillway_max_bytes(64 * PAGE_SIZE as u64); + let mut db = Chisel::open(&db_path, opts).unwrap(); + db.begin().unwrap(); + for _ in 0..400 { + if db.allocate(&vec![0x5A; PAGE_SIZE + 64]).is_err() { + break; + } + } + let _ = db.rollback(); + drop(db); + + let mode = fs::metadata(&sidecar).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode & 0o077, + 0, + "the spillway must not inherit a planted file's group/world bits (got {mode:o})" + ); + // And the planted bytes must be gone — the sidecar in use is a new file, + // not the planted one reused. + assert_ne!( + fs::read(&sidecar).unwrap(), + b"planted", + "the planted file must have been replaced, not adopted" + ); +} + +#[test] +#[cfg(unix)] +fn a_database_created_over_a_planted_empty_file_is_still_private() { + // `mode` on OpenOptions applies only when the open CREATES the file, and a + // zero-length file is a legitimate create target (that boundary is pinned + // by the PR #127 regression test). So a local user could pre-plant an empty + // world-readable file at the database path and Chisel would adopt it, + // writing every stored value into a file the planter can read while the + // promised 0600 was never applied. + use std::os::unix::fs::PermissionsExt; + + let dir = TempDir::new().unwrap(); + let path = dir.path().join("planted.db"); + fs::write(&path, b"").unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o666)).unwrap(); + + { + let mut db = Chisel::open(&path, Default::default()).unwrap(); + db.begin().unwrap(); + db.allocate(b"private").unwrap(); + db.commit().unwrap(); + } + + let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode & 0o077, + 0, + "adopting an empty file must not leave the database group/world-accessible (got {mode:o})" + ); +} + +#[test] +fn a_forged_overflow_length_cannot_run_the_delete_path_out_of_memory() { + // SECURITY-SWEEP-2 reached through `delete`/`update` instead of `read`. + // `Overflow::collect_chain_pages` reads the same disk-controlled + // total_length at bytes 16..24 and derives `max_pages` from it. The first + // fix bounded the value in `read` only, so a forged u64::MAX still yielded + // max_pages ~2.26e15 here — and with a self-referential next_page link the + // loop pushes into an unbounded Vec until the allocator aborts the process, + // which is exactly the poison-model bypass the fix set out to close. + let file = NamedTempFile::new().unwrap(); + let path = file.path().to_owned(); + + let handle; + { + let mut db = Chisel::open(&path, Default::default()).unwrap(); + db.begin().unwrap(); + handle = db.allocate(&vec![0xC3; 20_000]).unwrap(); + db.commit().unwrap(); + } + + let overflow_page = find_page_of_type(&path, PageType::Overflow); + { + let mut f = fs::OpenOptions::new() + .read(true) + .write(true) + .open(&path) + .unwrap(); + let mut buf = [0u8; PAGE_SIZE]; + f.seek(SeekFrom::Start(overflow_page * PAGE_SIZE as u64)) + .unwrap(); + f.read_exact(&mut buf).unwrap(); + // Forge the length AND point next_page at this same page: the + // self-reference is what turns an unbounded max_pages into an unbounded + // walk rather than a chain that simply ends. + buf[16..24].copy_from_slice(&u64::MAX.to_le_bytes()); + buf[24..32].copy_from_slice(&overflow_page.to_le_bytes()); + page::stamp_checksum(&mut buf); + f.seek(SeekFrom::Start(overflow_page * PAGE_SIZE as u64)) + .unwrap(); + f.write_all(&buf).unwrap(); + f.sync_all().unwrap(); + } + + let mut db = Chisel::open(&path, Default::default()).unwrap(); + db.begin().unwrap(); + match db.delete(handle) { + Err(ChiselError::CorruptPage { .. }) => {} + Ok(()) => panic!("a forged overflow length must not delete cleanly"), + Err(other) => panic!("expected CorruptPage, got {other:?}"), + } +} diff --git a/src/spillway.rs b/src/spillway.rs index 3e02c20..a022305 100644 --- a/src/spillway.rs +++ b/src/spillway.rs @@ -6,9 +6,13 @@ // allocation would push it past its strict cap. // // Lifecycle (spec 2026-05-03-chisel-spillway-design.md, "Lifecycle"): -// open file is created (or reused) and truncated to zero. Any -// pre-existing content is garbage from a crashed prior -// process and unconditionally discarded. +// open file is created fresh (O_EXCL | O_NOFOLLOW, mode 0600). Any +// pre-existing content is garbage from a crashed prior process +// and is discarded — but only after the entry is confirmed to be +// a plain file this user owns. The path is derived from the +// database path and is therefore predictable, so a symlink or a +// foreign-owned file there is a plant, not debris, and is refused +// (see `reclaim_stale_sidecar`). // spill page_id allocates a slot (or overwrites its existing // one), bytes + per-slot checksum are written. // Writes are deliberately NOT fsynced: spillway content never @@ -104,11 +108,56 @@ pub struct Spillway { payload_size: usize, } +/// Remove a pre-existing sidecar entry, but ONLY when it is plausibly debris +/// from a crashed run of this same user — a plain file, owned by us, with +/// exactly one link. Anything else (a symlink, a file owned by someone else, a +/// hard link into a directory we do not control) is a plant at a predictable +/// path, and is refused rather than cleaned up. +/// +/// The distinction matters because the two cases want opposite handling. Debris +/// is expected and must not break an open; a plant is an active attempt to +/// redirect or read uncommitted user data and must surface. `IoError` is fatal, +/// so a plant poisons the handle. +#[cfg(unix)] +fn reclaim_stale_sidecar(path: &Path) -> Result<()> { + use std::os::unix::fs::MetadataExt; + // symlink_metadata, not metadata: the whole point is to inspect the entry + // itself rather than whatever it may point at. + let md = std::fs::symlink_metadata(path).map_err(ChiselError::IoError)?; + // SAFETY: geteuid() is a pure read of process credentials. It cannot fail + // and touches no memory we own. + let ours = + md.file_type().is_file() && md.nlink() == 1 && md.uid() == unsafe { libc::geteuid() }; + if !ours { + return Err(ChiselError::IoError(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "spillway sidecar path is occupied by an entry this process did not create \ + (symlink, foreign owner, or extra hard link) — refusing to use it", + ))); + } + std::fs::remove_file(path).map_err(ChiselError::IoError) +} + +/// Non-unix fallback. Without `st_uid`/`st_nlink` there is nothing to +/// discriminate on, so keep the historical behaviour: treat a pre-existing +/// entry as crash debris and discard it. +#[cfg(not(unix))] +fn reclaim_stale_sidecar(path: &Path) -> Result<()> { + std::fs::remove_file(path).map_err(ChiselError::IoError) +} + impl Spillway { - /// Open (or create + truncate) a file-backed spillway alongside the - /// main database. The path is `.spillway`. Any pre-existing - /// content is discarded — no superblock can possibly point at - /// spillway bytes, so this is always safe. + /// Open a fresh file-backed spillway alongside the main database. The path + /// is `.spillway`. + /// + /// Any pre-existing entry at that path is REMOVED, not adopted: the sidecar + /// is created exclusively (`O_EXCL | O_NOFOLLOW`, mode 0600). No superblock + /// can point at spillway bytes, so discarding old content is always safe — + /// but reusing the old file is not, because the path is predictable and + /// another local user may have planted something there. A losing race for + /// the create (someone re-planted between the unlink and the open) is a hard + /// error that poisons the handle, which is the correct outcome for "someone + /// is tampering with my sidecar path". /// /// `payload_size` is `PAGE_SIZE` for plaintext DBs and `ENC_PAGE_SIZE` /// for encrypted DBs. It determines the slot size and capacity accounting. @@ -130,36 +179,62 @@ impl Spillway { // in-flight cache state, not a "fix your path and retry" condition, so it // must poison rather than mislead the caller into continuing (review // 2026-06-22). - let mut opts = OpenOptions::new(); - opts.read(true).write(true).create(true).truncate(true); - // The sidecar needs BOTH guards, and for a sharper reason than the main - // database file does. + // The sidecar needs sharper handling than the main database file. // // Its path is fully derived from the database path, so it is predictable // to anyone who can see the database. It is created lazily, mid // transaction, whenever cache pressure forces a spill — not at open, - // where a caller might notice something wrong. And it is opened with - // `truncate(true)`, on the documented assumption that any pre-existing - // content is garbage from a crashed prior process. + // where a caller might notice something wrong. And it used to be opened + // `create(true).truncate(true)`, on the documented assumption that any + // pre-existing content is garbage from a crashed prior process. // - // That assumption does not hold for a pre-existing SYMLINK. A local user - // who can create entries in the database's directory can plant - // `.spillway` pointing at any file the database owner can write; the - // next spill would then follow it and truncate the victim's file to - // zero. O_NOFOLLOW makes that open fail (ELOOP) instead — a poisoned - // handle, which is the correct outcome for "someone is tampering with my - // sidecar path". Encryption does not help here: the hazard is the - // truncate, not the contents. + // That assumption fails against a local user who can create entries in + // the database's directory, in TWO directions: // - // 0600 for the same reason as the main file, and it matters more here: - // spilled pages are uncommitted user data written without the caller - // ever asking for a second file to exist. + // * a planted SYMLINK pointing at any file the database owner can + // write would be followed and the victim's file truncated to zero; + // * a planted REGULAR file, owned by the attacker and mode 0666, would + // simply be adopted — `truncate` empties it but does not change its + // owner or mode, and `mode(0600)` applies only when the open itself + // CREATES the file. Chisel would then write spilled pages, which are + // uncommitted user values, into a file the attacker can read. + // + // Closing only the first direction leaves a data-disclosure hole behind + // a fix labelled as closing the hazard. So: unlink whatever is there, + // then create EXCLUSIVELY. `create_new` sets O_EXCL, which makes the + // open fail rather than adopt anything that reappears between the two + // syscalls, so the attacker cannot win the race by re-planting — the + // worst they achieve is a denial of service, which a local user who can + // write to this directory already has by other means. + // + // Encryption does not help with either direction: the hazards are the + // truncate and the file's ownership, not the contents. + // + // Crash debris is still discarded, as the lifecycle doc promises — but + // only after `reclaim_stale_sidecar` confirms it really is debris this + // user could have left, rather than another user's plant. That keeps the + // documented behaviour for the case it was written for (a prior run of + // ours died) and fails closed for the case it was not. + let mut opts = OpenOptions::new(); + opts.read(true).write(true).create_new(true); #[cfg(unix)] { use std::os::unix::fs::OpenOptionsExt; opts.mode(0o600).custom_flags(libc::O_NOFOLLOW); } - let file = opts.open(&path).map_err(ChiselError::IoError)?; + let file = match opts.open(&path) { + Ok(f) => f, + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + reclaim_stale_sidecar(Path::new(&path))?; + // Exclusive again on the retry: if the planter re-created the + // entry between the unlink and this open, we must fail rather + // than adopt it. A local user who can win that race can already + // deny service by other means, so a hard error is the correct + // trade — it never adopts a file we did not create. + opts.open(&path).map_err(ChiselError::IoError)? + } + Err(e) => return Err(ChiselError::IoError(e)), + }; Ok(Spillway { backing: Backing::File { file }, slots: HashMap::new(), diff --git a/src/transaction/recovery.rs b/src/transaction/recovery.rs index 1e03106..d380be0 100644 --- a/src/transaction/recovery.rs +++ b/src/transaction/recovery.rs @@ -434,8 +434,9 @@ impl TransactionManager { // claimed `capacity()` saturation "fails closed (rejects the descent)", // but saturation only prevents arithmetic overflow — it rejects nothing. if sb.freemap_depth > crate::freemap_tree::MAX_DEPTH { - return Err(ChiselError::CorruptSuperblock { - defects: Vec::new(), + return Err(ChiselError::InvalidFreemapDepth { + stored: sb.freemap_depth, + max: crate::freemap_tree::MAX_DEPTH, }); }