Omar Sobh
b08df7b628
Mission: 01a00c41-bac0-7eb3-a8c8-8b7044f3086d Phase: 01a00c41-bac2-71e3-a58b-c473421200ee Committed by the ClawMates delivery pipeline from the agents' working tree. Authored by agents, not by the named committer.
2.4 KiB
Research: Security — clawhdf5
Scope: opportunities not already covered by the shipped hardening (WAL CRC32
trailer / WAL_VERSION 2, MAX_WAL_FIELD_LEN field caps, Android JNI length
validation, chunked_read.rs/data_read.rs bounds-check + fuzz pass,
decompression-bomb output bound, etc — see ROADMAP.md).
Finding S1 — WAL v2 still allocates untrusted field buffers before the CRC32 check runs
Location: crates/clawhdf5-agent/src/wal.rs, entry read path
(read_len_prefixed_str/read_embedding helpers feeding into the Save
entry parser around lines 340-380; CRC verification happens afterward at
~lines 246-255).
Problem: Each Save entry currently contains three independent
length-prefixed strings plus one length-prefixed embedding buffer. Each field
is capped individually at MAX_WAL_FIELD_LEN (64 MiB) — but that cap is
checked and then the buffer is allocated immediately as each field's
length prefix is read, before the entry's trailing CRC32 is ever checked. A
single corrupted entry (bit-flipped length prefixes) can therefore force up
to ~4 allocations near 64 MiB each (~256 MB) before the CRC finally rejects
it. This is exactly what ROADMAP.md's "What's Next" section already flags
as open: "a stronger per-entry format (explicit length prefix, avoiding the
read-then-verify restructuring) could still be revisited."
Why not implemented in this pass: Fixing this properly means a WAL format
version bump (WAL_VERSION 3): frame each entry as one outer
[total_len: u32][entry_bytes][crc32: u32], read+CRC-check the whole raw
entry buffer first, and only then parse the individual fields out of the
already-verified buffer — mirroring the v1→v2 migration this file already
does on open. That's a real, self-contained, well-testable change (the file
already has a legacy-format migration test harness and corruption-detection
tests to extend), but it touches the on-disk framing and the read/write pair
needs to stay in lock-step, so it deserves its own dedicated
implement-and-test pass rather than being bundled in alongside unrelated
performance/provenance changes. Tracked as INT-04 below for follow-up.
Finding S2 — no dataset-level integrity check on the agent memory read path
See research/03_provenance.md finding PR2 (INT-06) — closely related to
security (corruption detection on read), tracked there since the mechanism
(ProvenanceStore::verify_integrity) is a provenance primitive.