fix(btree): bind node kind to authenticated envelope and bound-check node bodies - #11
Merged
farhan-syah merged 5 commits intoJul 26, 2026
Conversation
# Conflicts: # Cargo.toml # README.md
An authenticated page's bytes are only guaranteed to be what a key holder wrote, not what a correct writer would write. Leaf and internal decoders used prefix_len, slot_count, and slot-directory offsets directly as slice indices, so a malformed-but-authenticated body could panic the library instead of surfacing as corruption. Add validate_node_body to structurally check the header, slot directory, and every leaf/internal record fit within the body before any accessor indexes into it, and route all node parse paths through it.
Switch the authenticated cold node-read benchmark from a per-call Arc<AsyncMutex<Db>> plus a locally built runtime to the shared block_on/with_rt helpers and an Rc<Db>, since the workload is single-threaded and read-only. Also move cache eviction and key construction into iter_with_setup so only descent plus authentication is timed.
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
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.
Summary
A B+ tree node declares its kind twice: once in the AEAD-authenticated page
envelope, once in the encrypted body header. Reads decoded the body's
declaration and ignored the authenticated one, so a page authenticated as
BTreeInternalcould be interpreted as a leaf, or the reverse.BTree::read_node_guardnow requires the two to agree and reportsCorruption(HeaderUnverifiable)when they don't.Reviewing that led to a second, worse problem in the same threat model: the
node header's
prefix_len,slot_count, and slot-directory entries are useddirectly as slice indices with no validation, so a malformed-but-authenticated
page panics the library instead of reporting corruption. This PR fixes that
class too — see Bounds validation below.
The original diagnosis, the envelope/body binding, and its two-direction
regression are @presempathy-awb's. The bounds validation, the benchmark
correction, and the merge with current
maincame out of review.Threat model
Worth stating precisely, because it scopes both fixes.
page_kindis part of the AAD (src/crypto/aad.rs), bound to the same AEAD tagas the body. An attacker without the key cannot produce either a kind
mismatch or a malformed body — the tag check rejects it first. Both regressions
in this PR have to write their poisoned pages through the pager's own
write_main_pageto construct the condition.What remains, and why both fixes are still worth having:
Follower/apply_incremental/restore_from, where pages authoredelsewhere by a holder of the same key are consumed.
So this is integrity hardening at a trust boundary, not a remote-attacker fix.
Envelope/body binding
read_node_guardreceives the guard and the authenticated envelope kind fromthe existing one-pass pager API, decodes the body header once, and rejects
disagreement. No extra read, decrypt, or cache lookup — the authenticated kind
already arrives with the page.
The non-node
PageKindarm is unreachable today (KindBinding::Nodealreadyrestricts to the two node kinds on both the warm and cold pager paths). It is
kept, and now commented as such, so the boundary stays total if the pager ever
admits another kind.
Bounds validation
validate_node_body(src/btree/node.rs) structurally validates a body once:the header fits, prefix and slot directory fit, and every slot's record extent
lies inside the body — for both the leaf and internal record layouts.
It runs in all four constructors that turn raw bytes into a node —
Leaf::decode,Internal::decode,LeafAccessor::new,InternalAccessor::new— so the unchecked indexing downstream is sound by construction rather than by
inspection. The zero-copy accessors matter as much as the decoders here: they
are what the hot read path actually uses, and they had the same unchecked
indexing. Covering all four also catches the paths in
maintenance.rsanddeep_walk.rsthat decode bodies without going throughread_node_guard.Validation is deliberately extent-only. It proves each record lies inside
the body, not that the records are semantically sensible. A page with
overlapping or nonsensical offsets still decodes to garbage — authenticated
garbage is the writer's problem — but it cannot read out of bounds. Tightening
further (ordered, non-overlapping, past-the-directory offsets) would start
rejecting layouts a future encoder might legitimately produce.
OVERFLOW_SENTINELmoved fromleaf.rstonode.rs, since the validator needsthe record format and that is where the layout is defined.
Tests
Each was confirmed to fail without its fix, by temporarily stubbing the check
out — not merely observed to pass:
Some([118])— the value read out of a mis-typed pageprefix_lenpast bodypanicked: range end index 60024 out of range for slice of length 4056Coverage added: four unit tests on the validator in
src/btree/node.rs, andfive malformed-page cases driven end-to-end through
BTree::getintests/btree_basic.rs, alongside the existing two-direction mismatch test.The pager test was renamed
read_main_node_discovers_kind_in_a_single_read.It passes with the envelope/body check removed, so its earlier name
overstated what it guards. It locks the single-read shape of the pager API —
worth keeping, because the agreement check is only free while the authenticated
kind arrives with the page — and now says so.
Benchmark
benches/authenticated_node_read.rsmeasures a cold authenticated descentthrough a multi-level tree.
The first draft called
evict_main_pagesinsideb.iter, charging cachebookkeeping, the
Dblock, and key construction to the read path. That is thesource of the wide spread in the original numbers (per-round deltas from
-8.6%to+12.1%, and-11%to+25%on the earlier diagnostic run) — theharness, not the host. Eviction and key setup now happen in an untimed
iter_with_setupphase:Not comparable to the
~604 nsin the original description: that figure andthis one measure different things, and the old one is no longer produced by any
code in this branch.
The bench also now uses the shared
benches/commonharness that landed with#10, rather than re-rolling the runtime thread-local and tracking allocator.
Runs on
MemVfs, so the figure is CPU + AEAD for an authenticated cold descent,not the cost of reaching real storage.
Verification
On the merged branch:
cargo fmt --all --checkclean,cargo clippy --all-targets --all-features -- -D warningsclean,cargo nextest run --all-features416 passed / 4 skipped,cargo bench --bench authenticated_node_readruns to completion.Compatibility
Valid pages follow the same read, decrypt, decode, and accessor paths as
before. Newly rejected: a page whose two kind declarations disagree, and a page
whose header or slot directory describes records outside the body. Both were
previously accepted — the first silently, the second as a panic. No format,
public API, feature flag, dependency, or VFS contract movement.