Skip to content

fix: bound txn_counter at the trust boundary (#108) - #151

Merged
Xof merged 1 commit into
mainfrom
fix/108-txn-counter-bound
Aug 5, 2026
Merged

fix: bound txn_counter at the trust boundary (#108)#151
Xof merged 1 commit into
mainfrom
fix/108-txn-counter-bound

Conversation

@Xof

@Xof Xof commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Closes #108. Stacked on #150.

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("txn_counter overflowed u64 (2^64 commits) — unreachable")

The comment justifying that expect over a typed error argued overflow is "structurally unreachable". That 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 regression test demonstrates precisely this — with the guard removed it panics at commit.rs:133 (the expect itself), not at the assertion.

Why the bound lives in validate()

Not 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 win outright. Returning a SuperblockDefect instead 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 rather than reporting a bare "no valid superblock".

Both behaviours are pinned by tests: all-slots-forged → CorruptSuperblock carrying BadTxnCounter; one-slot-forged → opens from the sibling and still commits.

MAX_TXN_COUNTER reserves 2³² off the top, so the expect now rests on a guarded invariant rather than an argument about counting: a file that opened has 4.3 billion increments of headroom, and a reopen re-validates. The rationale comments at both bump sites (commit.rs, keys.rs) are corrected — they are the sentences a future maintainer would otherwise trust.

Also: CI was not running on any of this

The workflow filtered pull_request: branches: [main], and that filter matches the PR's base. A stacked PR — based on its predecessor so each stays 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, so the work only met CI after being retargeted to main at merge time, i.e. after review had already happened against an unverified diff. #149 got CI only because it happens to target main directly.

On the other #108 finding

SUPERBLOCK-RECOVERY-4 (sub-page files silently overwritten) already landed on main as #127 — verified against the current tree, no further work needed.

Verification

714 tests pass, clippy and fmt clean. Both new tests verified non-vacuous by removing the guard.

…cked PRs

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).
@Xof
Xof changed the base branch from docs/publish-review-adr-and-retire-issues to main August 5, 2026 03:41
@Xof
Xof merged commit adb04d8 into main Aug 5, 2026
9 checks passed
Xof added a commit that referenced this pull request Aug 5, 2026
fix: bound txn_counter on the write side too (review follow-up to #151)
@Xof
Xof deleted the fix/108-txn-counter-bound branch August 5, 2026 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Recovery and open path: unvalidated on-disk values reach structure-dependent code

1 participant