fix: trust-boundary hardening gaps from the cross-cutting security sweep (#102) - #149
Merged
Conversation
…weep 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.
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.
🚦 Bench results: PR vs main
Per-scenario detail (4 metrics × cells)document-store
mutation-log
ycsb-a
ycsb-b
|
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #102.
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 #128; this closes the other four, plus the gaps an adversarial review found in those fixes.
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 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.What is fixed
Overflow::readsized aVecfrom a disk-controlledu64→handle_alloc_error, process abort, from a plainChisel::readtotal_lengthagainstnext_page_id * OVERFLOW_PAYLOADfreemap_depthcopied from the superblock unvalidated →scan_nodestack overflow (SIGSEGV) andcow_descendO(depth²) file growth on the ordinary commit pathopen_existing; depth check at the tree's own entry pointsO_EXCL | O_NOFOLLOW&[Identifier]→&[Handle]transmute guarded by const assertions that cannot detect removal ofrepr(transparent)Second commit: what the adversarial review caught
The review (Fable 5, against the stated threat model) confirmed all five original tests are non-vacuous, then found two fixes incomplete in ways that left the original hazard reachable through a different door:
Overflow::collect_chain_pagesreads the sametotal_lengthand derivesmax_pagesfrom it with no ceiling — reached fromdelete/updaterather thanread. Forgedu64::MAX+ a self-referentialnext_pagegrows an unboundedVecuntil the allocator aborts. Verified by removing the guard and watching the new test run past 90s instead of failing.O_NOFOLLOWstops a planted symlink, but a planted regular file is not a symlink: the open succeeds, andmode(0600)applies only when the open creates the file. The planter keeps ownership and their 0666, and Chisel writes spilled pages — uncommitted user values — into a file they can read. Measured at 0666 before the fix. The same class of hole existed on the main database file, where a planted empty file is a legitimate create target under the fix: refuse to create a database over an existing sub-page file #127 rules.Plus:
mark_free_growingevaluatedcapacity()(which loopsdepthtimes) before its depth check, so a forgedu32::MAXspun ~4.3e9 multiplies before failing closed; the over-deep depth was reported asCorruptSuperblockwith an empty defect list, which is both undiagnosable and the wrong recoverability class; andhandle.rsstill documented the transmute the first commit deleted.Verification
mode 666, the overflow test runs unbounded past 90s.ARCHITECTURE.mdgains a "File permissions and the sidecar path" section — these were user-visible contract changes shipped with no documentation.