diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ec2f47..75de94f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,29 @@ # Changelog +## Unreleased + +### Security +- `clawhdf5-format`: **a crafted file could crash any reader through B-tree v2 + traversal.** Recursion was bounded only by the depth the file claimed (a + `u16`), and child addresses were never checked for sharing. A node listing + itself as its own child under a header claiming 65 535 levels — under 100 + bytes — overflowed the stack and **aborted the process** (SIGABRT, not a + catchable error). Levels whose children all point at one shared node below + reached it fan-out^depth times: 29.5 million records from ~5 KB, and one + more level would exhaust memory. Both are now errors, returned in under a + millisecond: depth is capped at 64 (as the fractal heap already was), and + traversal stops once it has produced more records than the file has bytes + to hold. Every B-tree v2 user goes through this path — dense attributes, + v2 groups, shared messages and chunk indexes. Valid files are unaffected, + including a depth-2 HDF5 2.0 chunk index with 40 000 records, now covered by + an interop test. + +### Documentation +- `clawhdf5-agent`: `BM25Index::search` claimed to use Block-Max WAND for early + termination. It never did; it scores every match exhaustively. It now says + so, and why no pruning would help the store: `hybrid_search` uses `scores()`, + since fusion normalises over every match. + ## v2.6.0 (2026-09-20) ### Upgrade Notes diff --git a/crates/clawhdf5-agent/src/bm25.rs b/crates/clawhdf5-agent/src/bm25.rs index 621a9f7..ba23cb0 100644 --- a/crates/clawhdf5-agent/src/bm25.rs +++ b/crates/clawhdf5-agent/src/bm25.rs @@ -88,8 +88,11 @@ impl BM25Index { /// Search the index for a query, returning the top `k` results /// as `(doc_id, score)` pairs sorted by score descending. /// - /// Uses Block-Max WAND for early termination when remaining documents - /// cannot beat the current top-k threshold. + /// Scores every matching document exhaustively, then keeps the top `k`. + /// There is no early termination (WAND, MaxScore): the store's hot path + /// is [`scores`](Self::scores), because score fusion normalises over the + /// whole matching set and so needs every score, which no pruning scheme + /// can skip. This method is for BM25-only callers. pub fn search(&self, query: &str, k: usize) -> Vec<(usize, f32)> { if k == 0 { return Vec::new(); @@ -561,8 +564,9 @@ mod tests { } #[test] - fn wand_returns_same_results_as_exhaustive() { - // WAND-style search should produce same scores as exhaustive + fn top_k_search_matches_ranking_every_score() { + // `search` must agree with ranking the full `scores` set — the + // bounded heap is an optimisation over sorting, not an approximation. let docs: Vec = (0..100) .map(|i| { if i % 3 == 0 { diff --git a/crates/clawhdf5-format/src/btree_v2.rs b/crates/clawhdf5-format/src/btree_v2.rs index ad58aa0..43fd7b0 100644 --- a/crates/clawhdf5-format/src/btree_v2.rs +++ b/crates/clawhdf5-format/src/btree_v2.rs @@ -172,6 +172,17 @@ fn max_records_leaf(node_size: u32, record_size: u16) -> u64 { ((node_size - overhead) / record_size as u32) as u64 } +/// Deepest B-tree v2 accepted. See [`collect_btree_v2_records`]. +const MAX_DEPTH: u16 = 64; + +/// Take `n` records from the traversal's budget, or refuse the tree. +fn spend(budget: &mut usize, n: usize) -> Result<(), FormatError> { + *budget = budget + .checked_sub(n) + .ok_or(FormatError::NestingDepthExceeded)?; + Ok(()) +} + /// Collect all records from a B-tree v2 by traversing from the root. pub fn collect_btree_v2_records( file_data: &[u8], @@ -182,6 +193,22 @@ pub fn collect_btree_v2_records( if header.total_records == 0 || header.num_records_in_root == 0 { return Ok(Vec::new()); } + // Recursion is one frame per level, and the depth is read from the file: + // a crafted header claiming 65 535 levels over a node that is its own + // child overflowed the stack. 64 matches the fractal heap's guard, and no + // real tree comes close — even at the minimum fan-out of two it would + // hold more than 2^64 records. + if header.depth > MAX_DEPTH { + return Err(FormatError::NestingDepthExceeded); + } + // A valid tree stores each record once, in its own bytes, so it cannot + // hold more records than the file has room for. Children are addresses, + // though, and nothing makes them distinct: levels whose children all + // point at one shared node below reach it fan-out^depth times, which is + // millions of records from a few kilobytes. Counting against what the + // file could physically contain bounds that without trusting the + // header's own `total_records`. + let mut budget = file_data.len() / usize::from(header.record_size.max(1)); let max_leaf_nrec = max_records_leaf(header.node_size, header.record_size); @@ -206,6 +233,7 @@ pub fn collect_btree_v2_records( offset_size, length_size, max_leaf_nrec, + &mut budget, &mut records, )?; Ok(records) @@ -273,6 +301,7 @@ fn collect_internal_records( offset_size: u8, length_size: u8, max_leaf_nrec: u64, + budget: &mut usize, out: &mut Vec, ) -> Result<(), FormatError> { // signature(4) + version(1) + type(1) = 6 @@ -350,6 +379,8 @@ fn collect_internal_records( // We collect child[0] records, then record[0], then child[1], etc. for (i, &(child_addr, child_nrec)) in children.iter().enumerate() { if child_depth == 0 { + // Before parsing, so a refused tree is not also a large allocation. + spend(budget, usize::from(child_nrec))?; let leaf_recs = parse_leaf_records(file_data, child_addr as usize, child_nrec, record_size)?; out.extend(leaf_recs); @@ -364,6 +395,7 @@ fn collect_internal_records( offset_size, length_size, max_leaf_nrec, + budget, out, )?; } @@ -393,6 +425,7 @@ fn collect_internal_records( available: file_data.len(), }); } + spend(budget, 1)?; out.push(BTreeV2Record { data: file_data[rec_start..rec_end].to_vec(), }); @@ -466,6 +499,124 @@ mod tests { buf } + /// An internal node laid out exactly as `collect_internal_records` will + /// read it at `depth`: `records` zeroed records, then `children` pointers, + /// all to `child_addr` claiming `child_nrec` records. + fn internal_node( + depth: u16, + node_size: u32, + record_size: u16, + records: usize, + children: usize, + child_addr: u64, + child_nrec: u64, + ) -> Vec { + let max_leaf = max_records_leaf(node_size, record_size); + let nrec_width = bytes_for_max_records(if depth == 1 { max_leaf } else { max_leaf * 2 }); + let total_width = if depth > 1 { + bytes_for_max_records(header_max_total_records(max_leaf, depth - 1)) + } else { + 0 + }; + let mut buf = b"BTIN".to_vec(); + buf.extend_from_slice(&[0, 5]); + buf.resize(buf.len() + records * record_size as usize, 0); + for _ in 0..children { + buf.extend_from_slice(&child_addr.to_le_bytes()); + buf.extend_from_slice(&child_nrec.to_le_bytes()[..nrec_width]); + buf.resize(buf.len() + total_width, 0); + } + buf + } + + fn header(depth: u16, root: u64, root_nrec: u16, total: u64) -> BTreeV2Header { + BTreeV2Header { + tree_type: 5, + node_size: 512, + record_size: 8, + depth, + root_node_address: root, + num_records_in_root: root_nrec, + total_records: total, + } + } + + #[test] + fn a_node_that_is_its_own_child_is_rejected_not_recursed() { + // One internal node whose two children are itself, under a header + // claiming the deepest tree a u16 allows. The layout stops depending + // on depth once the subtree-total width saturates, so every level + // parses cleanly and recursion runs ~65 000 frames deep: before the + // cap this overflowed the stack and aborted the process, from a file + // of under 100 bytes. + let mut data = internal_node(u16::MAX, 512, 8, 1, 2, 0, 1); + data.resize(4096, 0); + let result = collect_btree_v2_records(&data, &header(u16::MAX, 0, 1, 1), 8, 8); + assert!(result.is_err(), "{result:?}"); + } + + #[test] + fn a_shared_subtree_cannot_multiply_the_work() { + // A chain of distinct levels, each node's children all pointing at the + // single node below, ending in a real leaf. Every node parses and + // nothing is cyclic, yet the leaf is reached fan-out^depth times: 62 + // children over 4 levels is ~15 million leaf visits from a few + // kilobytes. A valid tree cannot hold more records than the file has + // room for, so that bounds the traversal instead. + let (node_size, record_size) = (512u32, 8u16); + let fanout = 62usize; + let depth = 4u16; + let leaf = build_leaf_node(5, &[&[0u8; 8][..]]); + + // Lay out root first, then each lower level, then the leaf. + let mut nodes: Vec> = Vec::new(); + let mut addrs = Vec::new(); + let mut at = 0u64; + let mut sizes = Vec::new(); + for d in (1..=depth).rev() { + let n = internal_node(d, node_size, record_size, fanout - 1, fanout, 0, 0); + sizes.push(n.len()); + } + for size in &sizes { + addrs.push(at); + at += *size as u64; + } + let leaf_addr = at; + for (i, d) in (1..=depth).rev().enumerate() { + let (child, child_nrec) = if d == 1 { + (leaf_addr, 1) + } else { + (addrs[i + 1], fanout as u64 - 1) + }; + nodes.push(internal_node( + d, + node_size, + record_size, + fanout - 1, + fanout, + child, + child_nrec, + )); + } + let mut data: Vec = nodes.concat(); + data.extend_from_slice(&leaf); + data.resize(data.len() + 64, 0); + + let started = std::time::Instant::now(); + let result = + collect_btree_v2_records(&data, &header(depth, 0, fanout as u16 - 1, u64::MAX), 8, 8); + assert!( + result.is_err(), + "expected a refusal, got {} records", + result.map_or(0, |r| r.len()) + ); + assert!( + started.elapsed() < std::time::Duration::from_secs(2), + "took {:?}", + started.elapsed() + ); + } + #[test] fn parse_header() { let data = build_btree_v2_header(5, 512, 11, 0, 0x1000, 3, 3, 8, 8); diff --git a/crates/clawhdf5/tests/h5py_interop_tests.rs b/crates/clawhdf5/tests/h5py_interop_tests.rs index c7ab11c..ede336b 100644 --- a/crates/clawhdf5/tests/h5py_interop_tests.rs +++ b/crates/clawhdf5/tests/h5py_interop_tests.rs @@ -1047,3 +1047,47 @@ with h5py.File("{path_str}", "r") as f: data[start..start + cols as usize] ); } + +#[test] +fn h5py_deep_btree_v2_chunk_index_clawhdf5_reads() { + // Two unlimited dimensions give a B-tree v2 chunk index, and 2x2 chunks + // over 400x400 give 40 000 index records — enough for HDF5 to build a + // tree of depth 2. Small h5py files only ever produce depth-0 trees, so + // this is the one fixture that walks internal nodes: the path where the + // traversal's record budget (the guard against crafted shared-subtree + // trees) is spent, which must never refuse a real file. + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("deep_btree.h5"); + let path_str = path.display().to_string(); + + let script = format!( + r#" +import h5py, numpy as np +with h5py.File("{path_str}", "w", libver="latest") as f: + d = f.create_dataset("x", shape=(400, 400), maxshape=(None, None), + chunks=(2, 2), dtype="i4") + d[...] = np.arange(160000, dtype="i4").reshape(400, 400) +"# + ); + run_python(&script); + + // The fixture is only meaningful if HDF5 really built internal nodes. + let bytes = std::fs::read(&path).unwrap(); + let at = bytes + .windows(4) + .position(|w| w == b"BTHD") + .expect("expected a B-tree v2 chunk index"); + let depth = u16::from_le_bytes([bytes[at + 12], bytes[at + 13]]); + assert!( + depth >= 1, + "fixture tree has depth {depth}; it tests nothing" + ); + + let file = File::open(&path).unwrap(); + let values = file.dataset("x").unwrap().read_i32().unwrap(); + assert_eq!(values.len(), 160_000); + for (i, &v) in values.iter().enumerate() { + assert_eq!(v, i as i32, "element {i}"); + } +} diff --git a/docs/known-issues.md b/docs/known-issues.md index ad06ab0..d81eef1 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -169,3 +169,27 @@ python3 -m venv .venv && .venv/bin/pip install h5py numpy netCDF4 Set `CLAWHDF5_REQUIRE_INTEROP=1` in any automated runner so a missing interpreter is a failure rather than a skip. + + +--- + +## Crafted B-tree v2 structures crash or exhaust the reader + +**Status:** fixed on `main` (2026-09-20), after v2.6.0. **Every release up to +and including v2.6.0 is affected.** + +B-tree v2 traversal (`clawhdf5-format`, `btree_v2::collect_btree_v2_records`) +recursed one frame per level with the depth taken from the file, and followed +child addresses without checking whether they were shared. Two consequences +for anyone reading untrusted files: + +- A node that is its own child, under a header claiming 65 535 levels, overflows + the stack and aborts the process. The file is under 100 bytes. +- Levels whose children all point at one node below make the traversal visit it + fan-out^depth times: ~30 million records from ~5 KB, and memory exhaustion one + level deeper. + +B-tree v2 backs dense attribute storage, v2 groups, shared object header +messages and chunk indexes, so opening an object that uses any of them is +enough. Both are now errors: depth is capped at 64, and traversal stops once it +has produced more records than the file could physically hold.