Skip to content

fix(parser): prevent integer overflow in Parser::next bounds check - #9

Open
dingledropper wants to merge 1 commit into
BlockchainCommons:masterfrom
dingledropper:fix/parser-next-overflow
Open

fix(parser): prevent integer overflow in Parser::next bounds check#9
dingledropper wants to merge 1 commit into
BlockchainCommons:masterfrom
dingledropper:fix/parser-next-overflow

Conversation

@dingledropper

Copy link
Copy Markdown

Summary

Fixes #8.

Parser::next (src/parser/parser_impl.rs) computed self.offset + n > self.buffer.len() with unchecked usize addition. A CompactSize length prefix of 0xff followed by 8 bytes of 0xff decodes to n = usize::MAX, wrapping the addition modulo 2^64 and bypassing the bounds check. The subsequent slice access then panics the thread (or panic_const_add_overflow fires one line earlier under -Cdebug-assertions).

Found via cargo-fuzz within seconds of running fresh harnesses against DBKey::parse_data and the full BDBDump → ZcashdDump → ZcashdParser::parse_dump pipeline.

Fix

Use checked_add to detect the overflow and return Error::BufferUnderflow cleanly. Audited siblings (peek line 270, rest line 275, std::io::Read impl line 296) — each saturates n to min(n, self.remaining()) before slice access, so they cannot wrap and need no change.

What's in this PR

  • src/parser/parser_impl.rs — replace the unchecked addition in Parser::next with checked_add + an inline comment explaining the fuzz finding.
  • fuzz/ — two cargo-fuzz harnesses (fuzz_dbkey_parse_data, fuzz_zcashd_full_parse) that surfaced the panic in the first few thousand iterations. Both reproducers are committed under fuzz/corpus/ so the regression class is caught in CI if you'd like to adopt the harness.

Test plan

  • cargo test passes.
  • PoC ff ff ff ff ff ff ff ff ff 04 (10 bytes) returns Err(BufferUnderflow) instead of panicking.
  • PoC 47-byte input through ZcashdParser::parse_dump returns Err cleanly.
  • cargo +nightly fuzz run fuzz_dbkey_parse_data ran for ~10 hours post-patch, 0 sibling panics.
  • cargo +nightly fuzz run fuzz_zcashd_full_parse ran for ~10 hours post-patch, 0 sibling panics.

Notes

The fuzz/Cargo.toml includes [patch.crates-io] zewif = { path = "../../zewif" } because the published zewif-zcashd Cargo.toml currently requires zewif = "0.1.0" while only 0.0.0 exists on crates.io. Happy to drop the patch line once zewif is bumped.

AI disclosure

This finding was identified, the fix was drafted, and the fuzz harnesses were built with assistance from Claude (Anthropic). I reviewed each step and am the responsible author.

`Parser::next(n)` previously computed `self.offset + n` to check whether
the read would exceed the buffer. A peer-controlled CompactSize length
prefix can produce an `n` of `usize::MAX` (the `0xff` prefix is followed
by an 8-byte little-endian u64). The unchecked addition wraps, the
bounds check returns `false`, and the function panics — either at the
overflow site under `-Cdebug-assertions` (libfuzzer build) or at the
subsequent slice access in a release build.

Reproducer: `[0xff; 9].chain([0x04])` (10 bytes) fed through
`DBKey::parse_data`, found in seconds by libfuzzer.

Switch to `checked_add` so out-of-range lengths return
`Error::BufferUnderflow` cleanly. CWE-190 (Integer Overflow), CWE-754
(Improper Check for Unusual Conditions).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integer overflow in Parser::next bounds check causes panic on crafted input

1 participant