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.
43 lines
2.4 KiB
Markdown
43 lines
2.4 KiB
Markdown
# 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.
|