fix: heap overflows in V1/V2 log decode (crafted counts_limit / word_size) - #146
Open
filipecosta90 wants to merge 2 commits into
Open
fix: heap overflows in V1/V2 log decode (crafted counts_limit / word_size)#146filipecosta90 wants to merge 2 commits into
filipecosta90 wants to merge 2 commits into
Conversation
hdr_decode_compressed_v1 derived counts_limit from the attacker-controlled payload_len field and passed it to apply_to_counts() without bounding it against h->counts_len. A crafted log (tiny histogram, large payload_len) makes apply_to_counts write past h->counts -> heap-buffer-overflow WRITE. Reachable from the public hdr_log_decode / hdr_log_read API. (V0 passes h->counts_len; V2 bounds internally -- only V1 trusted the payload length.) Reject counts_limit < 0 or > h->counts_len with HDR_ENCODED_INPUT_TOO_LONG before allocation/apply; this also prevents counts_array_len = counts_limit * word_size from overflowing int32_t. Verified with ASan: a crafted V1 blob triggers 'heap-buffer-overflow WRITE ... apply_to_counts_64' on the old code and returns HDR_ENCODED_INPUT_TOO_LONG with no error after the fix. Added a regression test with the crafted blob. Found-by: adversarial review during the ClusterFuzzLite fuzzing effort Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarial review of the initial V1 counts_limit fix surfaced two more attacker-reachable memory-safety bugs in the decode path: - hdr_decode_compressed_v1 with word_size == 1 routes counts to the zig-zag reader (apply_to_counts_zz), whose LEB128 lookahead reads up to 9 bytes past the counts buffer. V2 allocates +9 padding for this; V1 does not -> heap OOB READ. V1 only ever uses fixed-width 2/4/8; reject any other word size. - hdr_decode_compressed_v2 read counts_limit = be32toh(payload_len) (signed) with no check. A negative value makes hdr_calloc((size_t)counts_limit + 9) wrap to a tiny buffer while strm.avail_out = (uInt)counts_limit becomes ~4GB, so inflate writes gigabytes past it -> heap OOB WRITE. V2 is the default encoding, so this is the common path. Reject counts_limit < 0. Added regression tests (crafted blobs) for the V1 negative-payload_len, V1 word_size==1, and V2 negative-payload_len vectors; each reproduces a sanitizer crash on the old code and returns a clean error now. Full suite passes under ASan+UBSan. Found-by: adversarial review during the ClusterFuzzLite fuzzing effort Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
filipecosta90
changed the title
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
2 participants
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.
Security fix — heap buffer-overflow write, reachable from the public decode API
Surfaced by adversarial review during the new ClusterFuzzLite fuzzing effort (the random fuzzer didn't hit the specific crafted input in its budget; a reviewer found it by analysis).
Root cause
hdr_decode_compressed_v1computescounts_limit = be32toh(payload_len) / word_sizefrom the attacker-controlledpayload_lenfield, then callsapply_to_counts(h, word_size, counts_array, counts_limit), which loopsfor (i=0; i<counts_limit; i++) h->counts[i] = .... With no bound againsth->counts_len, a crafted log (tiny histogram + largepayload_len) writes past thehdr_calloc(counts_len, 8)buffer → heap-buffer-overflow WRITE. Reachable viahdr_log_decode/hdr_log_read. V0 (passesh->counts_len) and V2 (bounds internally) are unaffected — only V1 trusted the payload length.Fix
Reject
counts_limit < 0 || counts_limit > h->counts_lenwithHDR_ENCODED_INPUT_TOO_LONGright afterhdr_init, before allocation/apply. Also prevents an int32 overflow ofcounts_array_len = counts_limit * word_size.Verification (local, ASan)
AddressSanitizer: heap-buffer-overflow WRITE of size 8 in apply_to_counts_64 ← hdr_decode_compressed_v1:538 ← hdr_log_decode. After: returnsHDR_ENCODED_INPUT_TOO_LONG, no ASan error.test_v1_decode_rejects_oversized_counts) with the crafted blob; full suite passes under ASan+UBSan, valid encode/decode round-trips unaffected.🤖 Generated with Claude Code