From bdac2cc9db48c7c79fa3a335f767e800f32e729f Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Tue, 4 Aug 2026 16:23:38 -0700 Subject: [PATCH] fix: bound txn_counter at the trust boundary and stop CI skipping stacked PRs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SECURITY-SWEEP-6. `open_existing` adopted `txn_counter` verbatim from the superblock and `deserialize` read it with no bound, while commit.rs did txn_counter.checked_add(1).expect("...(2^64 commits) — unreachable") The comment justifying that `expect` over a typed error argued overflow is "structurally unreachable", which assumed the counter could only ever be produced by this binary's own increments. It cannot: it is read from a file the threat model says the attacker controls. A superblock forged with txn_counter = u64::MAX, valid magic, an in-range superblock_count and a recomputed XXH3 checksum opens fine, and the first commit after it panics — bypassing the documented poison-and-reopen contract, and surfacing in the PyO3 binding as a PanicException rather than a mapped error class. The new test reproduces exactly that: with the guard removed it panics at commit.rs's `expect`, not at the assertion. The bound goes in `Superblock::validate` — the shared torn-slot rule — rather than beside the page_size / freemap_depth gates in open_existing, because the counter is PER-SLOT. Selection is "highest counter wins", so a forged slot would otherwise win outright; returning a defect makes it lose to a healthy sibling exactly as a bad checksum does, which is what shadow paging is for. Only when every slot is bad does this surface, and then `diagnose` names the offending field instead of reporting a bare "no valid superblock". MAX_TXN_COUNTER reserves 2^32 off the top. That makes the `expect` rest on a guarded invariant rather than an argument about counting: a file that opened at all has 4.3 billion increments of headroom, and a reopen re-validates. The rationale comments at both bump sites are corrected to say so — they are the sentences a future maintainer would otherwise trust. Also fixes CI, which was not running on any of this. The workflow filtered `pull_request: branches: [main]`, and that filter matches the PR's BASE — so a stacked PR, based on its predecessor to stay individually reviewable, got no checks at all. The failure was silent: GitHub shows no checks, which reads as "nothing to run" rather than "never triggered", and the work only met CI after being retargeted to main at merge time — after review had happened against an unverified diff. Note that SUPERBLOCK-RECOVERY-4, the other finding on #108, already landed on main as PR #127 (sub-page files are refused rather than created over). --- .github/workflows/ci.yml | 8 ++- src/recovery_tests.rs | 119 ++++++++++++++++++++++++++++++++++++++ src/superblock/mod.rs | 35 +++++++++++ src/transaction/commit.rs | 21 +++++-- src/transaction/keys.rs | 5 ++ 5 files changed, 183 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index eb5f638..51bd706 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,8 +3,14 @@ name: CI on: push: branches: [main] + # Deliberately NOT filtered to `branches: [main]`. That filter matches the + # PR's BASE, so a stacked PR — one based on its predecessor rather than on + # main, which is how a chain of dependent fixes stays individually + # reviewable — got no CI at all. The gap was silent: the PR simply showed no + # checks, which reads like "nothing to run" rather than "never triggered", + # and the work only met CI after being retargeted to main at merge time, + # i.e. after review had already happened against an unverified diff. pull_request: - branches: [main] env: CARGO_TERM_COLOR: always diff --git a/src/recovery_tests.rs b/src/recovery_tests.rs index e554656..8d4b311 100644 --- a/src/recovery_tests.rs +++ b/src/recovery_tests.rs @@ -2060,3 +2060,122 @@ fn a_forged_overflow_length_cannot_run_the_delete_path_out_of_memory() { Err(other) => panic!("expected CorruptPage, got {other:?}"), } } + +// Helper for the two txn_counter tests below. Patches bytes 8..16 of one +// superblock slot and re-stamps the XXH3 checksum, which is exactly what an +// attacker with byte-level file access does — the checksum is non-cryptographic +// and publicly recomputable. Going through `Superblock::serialize` would not +// work here: `deserialize` now rejects the forged value, so the round-trip +// cannot produce the bytes under test. +#[cfg(test)] +fn forge_txn_counter(path: &std::path::Path, slot: u64, value: u64) { + let mut f = fs::OpenOptions::new() + .read(true) + .write(true) + .open(path) + .unwrap(); + let mut buf = [0u8; PAGE_SIZE]; + f.seek(SeekFrom::Start(slot * PAGE_SIZE as u64)).unwrap(); + f.read_exact(&mut buf).unwrap(); + buf[8..16].copy_from_slice(&value.to_le_bytes()); + page::stamp_checksum(&mut buf); + f.seek(SeekFrom::Start(slot * PAGE_SIZE as u64)).unwrap(); + f.write_all(&buf).unwrap(); + f.sync_all().unwrap(); +} + +#[test] +fn a_forged_txn_counter_in_every_slot_is_refused_at_open() { + // SECURITY-SWEEP-6. `open_existing` adopted `txn_counter` verbatim from the + // superblock, and `deserialize` read it with no bound. commit.rs then did + // `txn_counter.checked_add(1).expect("...unreachable")` on the strength of + // a comment arguing overflow needs 2^64 commits — an argument that only + // holds if the counter can ONLY come from this binary's own increments. + // It cannot: it comes off disk. A forged u64::MAX panicked on the first + // commit after open, bypassing the poison-and-reopen contract entirely and + // reaching Python as a PanicException rather than a mapped error class. + 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(); + } + + for slot in 0..2u64 { + forge_txn_counter(&path, slot, u64::MAX); + } + + // Every slot is bad, so selection has nothing to fall back to and the open + // fails with a per-slot diagnosis rather than reaching a commit at all. + match Chisel::open(&path, Default::default()) { + Err(ChiselError::CorruptSuperblock { defects }) => { + assert!( + defects + .iter() + .any(|d| matches!(d.defect, crate::superblock::SuperblockDefect::BadTxnCounter(n) if n == u64::MAX)), + "the diagnosis must name the offending field, got {defects:?}" + ); + } + Ok(_) => panic!("a txn_counter of u64::MAX in every slot must not open"), + Err(other) => panic!("expected CorruptSuperblock, got {other:?}"), + } +} + +#[test] +fn a_forged_txn_counter_in_one_slot_loses_to_its_healthy_sibling() { + // The reason this bound lives in `validate` (the shared torn-slot rule) + // rather than at the open-time gate next to page_size/freemap_depth: the + // counter is PER-SLOT. Shadow paging's whole point is that a damaged slot + // loses to a good one, so a forged counter must be discarded the same way a + // bad checksum is — not fail the entire open. Without that, the forged slot + // would win selection outright, since selection is "highest counter wins". + let file = NamedTempFile::new().unwrap(); + let path = file.path().to_owned(); + let first; + { + let mut db = Chisel::open(&path, Default::default()).unwrap(); + db.begin().unwrap(); + first = db.allocate(b"from the first commit").unwrap(); + db.commit().unwrap(); + // A second commit so BOTH slots hold a real state; the newest one is + // the forgery target below. + db.begin().unwrap(); + db.allocate(b"from the second commit").unwrap(); + db.commit().unwrap(); + } + + // Forge whichever slot currently wins selection. u64::MAX would outrank + // every sibling, so without the bound this slot is guaranteed to be chosen + // and its adopted counter would then panic the next commit. + let newest = { + let mut f = fs::File::open(&path).unwrap(); + let mut best = (0u64, 0u64); + for slot in 0..2u64 { + let mut buf = [0u8; PAGE_SIZE]; + f.seek(SeekFrom::Start(slot * PAGE_SIZE as u64)).unwrap(); + f.read_exact(&mut buf).unwrap(); + let sb = Superblock::deserialize(&buf).expect("slot must be valid"); + if sb.txn_counter >= best.1 { + best = (slot, sb.txn_counter); + } + } + best.0 + }; + forge_txn_counter(&path, newest, u64::MAX); + + let mut db = Chisel::open(&path, Default::default()) + .expect("a healthy sibling slot must still open the database"); + // Discarding the newest slot rewinds to the previous commit — that is the + // correct and unavoidable consequence of "a damaged slot loses to a good + // one", since the good one is by definition older. What matters is that the + // open SUCCEEDS from the sibling instead of failing outright or adopting + // the forged counter. + assert_eq!(db.read(first).unwrap(), b"from the first commit"); + // And the engine is fully usable: the counter it adopted came from the + // sibling, so the commit path has its full headroom and does not panic. + db.begin().unwrap(); + db.allocate(b"and it still commits").unwrap(); + db.commit().unwrap(); +} diff --git a/src/superblock/mod.rs b/src/superblock/mod.rs index 1e448be..8bc90a4 100644 --- a/src/superblock/mod.rs +++ b/src/superblock/mod.rs @@ -55,6 +55,27 @@ pub const MIN_SUPERBLOCKS: u32 = 2; pub const MAX_SUPERBLOCKS: u32 = 16; pub const DEFAULT_SUPERBLOCK_COUNT: u32 = 2; +// Headroom the commit path is guaranteed at open, reserved off the top of +// `txn_counter`'s range. `validate` rejects any slot claiming a counter above +// `MAX_TXN_COUNTER`, so a file that opens is always at least this far from +// overflowing. +// +// This exists because `commit` and `rewrite_crypto_header` both do +// `txn_counter.checked_add(1).expect(...)`, justified by a comment calling +// overflow "structurally unreachable". That argument assumed the counter could +// only ever be produced by this binary's own increments. It is in fact read +// from a file the threat model says an attacker controls, so a forged +// `txn_counter = u64::MAX` made the "unreachable" panic reachable from the +// first commit after open — bypassing the poison-and-reopen contract and, in +// the PyO3 binding, surfacing as a PanicException rather than a mapped error. +// +// 2^32 is chosen to be unreachable from the other direction too: exhausting it +// requires 4.3 billion commits within a SINGLE session (a reopen re-validates), +// at three fsyncs each. A database that legitimately climbed past +// MAX_TXN_COUNTER would be refused, but reaching it needs ~1.8e19 commits. +pub const TXN_COUNTER_RESERVE: u64 = 1 << 32; +pub const MAX_TXN_COUNTER: u64 = u64::MAX - TXN_COUNTER_RESERVE; + // Byte offset of the superblock_count field within the serialized // superblock page. Placed AFTER the named-root table (which ends at // NAMED_ROOTS_END = 308). Deserialization rejects any value outside @@ -140,6 +161,13 @@ pub enum SuperblockDefect { BadChecksum, BadMagic, BadCount(u32), // the out-of-range superblock_count value + // The slot's txn_counter leaves less than TXN_COUNTER_RESERVE of headroom + // before u64 overflow. Treated as a torn-slot rule rather than an open-time + // gate precisely because the counter is PER-SLOT: a forged slot should lose + // to a healthy sibling under the normal "highest counter wins" selection, + // which is exactly what returning a defect here achieves. Only if EVERY + // slot is bad does this surface, and then `diagnose` names the slot. + BadTxnCounter(u64), // The file is too short to contain this slot at all. Distinct from // BadMagic: there are no bytes to have magic, so the slot was never // read. Raised by `Chisel::open` for a file that has content but is @@ -155,6 +183,9 @@ impl fmt::Display for SuperblockDefect { SuperblockDefect::BadChecksum => write!(f, "bad checksum"), SuperblockDefect::BadMagic => write!(f, "bad magic"), SuperblockDefect::BadCount(n) => write!(f, "bad superblock_count {n}"), + SuperblockDefect::BadTxnCounter(n) => { + write!(f, "txn_counter {n} is too close to u64 overflow") + } SuperblockDefect::TooShort => write!(f, "file too short to contain this slot"), } } @@ -259,6 +290,10 @@ fn validate(buf: &[u8; PAGE_SIZE]) -> Result<(), SuperblockDefect> { if !(MIN_SUPERBLOCKS..=MAX_SUPERBLOCKS).contains(&count) { return Err(SuperblockDefect::BadCount(count)); } + let txn_counter = u64::from_le_bytes(buf[8..16].try_into().unwrap()); + if txn_counter > MAX_TXN_COUNTER { + return Err(SuperblockDefect::BadTxnCounter(txn_counter)); + } Ok(()) } diff --git a/src/transaction/commit.rs b/src/transaction/commit.rs index 7c151ad..9034bf3 100644 --- a/src/transaction/commit.rs +++ b/src/transaction/commit.rs @@ -110,10 +110,23 @@ pub(super) fn run_commit(ctx: &mut CommitCtx<'_>) -> Result<()> { // // I119 (ISSUES.md, 2026-06-21): checked, not `+= 1`. A wrapped counter // would corrupt `Superblock::select`'s "highest counter wins" (release - // wrap to 0) — far worse than the loud, controlled panic here. Overflow - // needs 2^64 commits, so it is structurally unreachable; a dedicated - // fatal error variant for an impossible event would be speculative - // public surface, so the `expect` on the invariant is proportionate. + // wrap to 0) — far worse than the loud, controlled panic here. + // + // The original rationale for the `expect` was "overflow needs 2^64 commits, + // so it is structurally unreachable". That was WRONG, and worth spelling out + // because it is the sentence a future maintainer would trust: the counter is + // not only produced by this binary's increments, it is also READ FROM THE + // FILE at open (`recovery.rs`, `txn_counter: sb.txn_counter`). A superblock + // forged with `txn_counter = u64::MAX` and a recomputed XXH3 checksum made + // the panic fire on the first commit after open. + // + // What makes it unreachable now is an actual invariant rather than an + // argument about counting: `Superblock::validate` rejects any slot whose + // counter exceeds MAX_TXN_COUNTER, so a file that opened at all has at least + // TXN_COUNTER_RESERVE (2^32) increments of headroom. Exhausting that takes + // 4.3 billion commits in one session, since a reopen re-validates. + // The `expect` therefore stays: it now documents a guarded invariant rather + // than asserting an unguarded hope. *ctx.txn_counter = ctx .txn_counter .checked_add(1) diff --git a/src/transaction/keys.rs b/src/transaction/keys.rs index 908e0d5..2b3b790 100644 --- a/src/transaction/keys.rs +++ b/src/transaction/keys.rs @@ -63,6 +63,11 @@ impl TransactionManager { // I119: use checked_add, not `+= 1`. A wrapped counter corrupts // Superblock::select's "highest counter wins" rule on recovery. + // + // Reachable from a forged superblock until `Superblock::validate` grew + // its MAX_TXN_COUNTER bound; see the long note at the matching site in + // commit.rs for why the old "structurally unreachable" claim was wrong + // and what actually guarantees the headroom now. self.txn_counter = self .txn_counter .checked_add(1)