fix(lba): check the address arithmetic the image chooses (2 fuzz-found panics) - #6
Merged
Conversation
Both nightly fuzz targets crash the same way:
src/lib.rs:477:18 attempt to add with overflow (parse_state)
src/lib.rs:596:26 attempt to add with overflow (read_dir)
Both sites add a partition start to a logical block number, and both operands
are u32 fields read out of the image:
fsd_lba: partition_start + fsd // File Set Descriptor location
let fe_lba = partition_start + icb_lbn; // directory entry's File Entry
Nothing bounds either, so a descriptor pair summing past u32::MAX panics. The
release profile would wrap instead and address the wrong block -- a silently
wrong answer rather than a loud one, which for a forensic reader is worse.
The reproducers are the bytes libFuzzer actually found, taken from the failing
run's uploaded artifact rather than hand-authored, so they exercise the real
path rather than the one I would have guessed. libFuzzer could not minimise
them further: the size is what carries the offsets past the boundary.
They are kept as ordinary tests, not only as fuzz corpus, because a defect
found by fuzzing should not need fuzzing to be caught again -- the fuzz job
runs nightly, the suite runs on every push.
The read_dir harness steers addressing from the input's leading bytes rather
than parsing them from the image, so its test decodes the reproducer exactly as
the target does; feeding the whole blob to the wrong entry point passes while
exercising nothing, which it did on the first attempt.
Assertions are deliberately weak on what comes back: a malformed image may
legitimately yield None, an error, or a structure. The contract under test is
only that it returns rather than panicking.
Both overflow sites add a partition start to a logical block number, and both
operands are u32 fields from the image, so their sum can leave the address
space:
* the File Set Descriptor address. An overflow means the descriptor pair does
not describe a location, so there is nothing to report and the walk returns
`Ok(None)`.
* a directory entry's File Entry address, per entry. The entry is dropped and
the walk continues, because one unreadable FID says nothing about its
siblings.
Release builds wrap rather than panic, which is the worse half of this bug: a
wrapped LBA addresses a real but wrong block, so the reader would have reported
a confidently incorrect File Set Descriptor, or a file size read from an
unrelated sector. Loud is better, absent is better still.
The name-field bounds in the same block are header-derived too and now
saturate. Saturating rather than skipping is deliberate: an out-of-range span
clamps to the end of the buffer and the existing `id_end > id_start` test then
leaves the name empty, which is what an unreadable name should produce.
One trap worth recording. The first attempt used `continue` for the per-entry
case, which read naturally and hung the test suite: the loop advances `off`
*below* that block, so skipping the iteration left the cursor unmoved and spun
on the same descriptor forever. Dropping the entry with `if let` keeps the
advance on the normal path. Trading a panic for an infinite loop would have
been a worse bug than the one being fixed, and only running the test caught it.
Both reproducers now pass through their real fuzz targets, not just the unit
tests. Full workspace: 0 failures; fmt and clippy clean.
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
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.
Two fuzz-found panics, one defect
Both sites add a partition start to a logical block number, and both operands
are
u32fields read out of the image:Nothing bounded either, so a descriptor pair summing past
u32::MAXpanics.The half that matters more
Release builds don't panic — they wrap. A wrapped LBA addresses a real but
wrong block, so the reader would report a confidently incorrect File Set
Descriptor, or a file size read from an unrelated sector. For a forensic tool
that's worse than a crash: a crash is visible, a wrong answer is not.
Fuzz builds enable overflow checks, which is why fuzzing found this and the
normal suite never could.
The fix
location, so the walk returns
Ok(None).unreadable FID says nothing about its siblings.
saturate, so an out-of-range span clamps and the existing
id_end > id_starttest leaves the name empty.
A trap worth recording
The first attempt used
continuefor the per-entry case. It read naturally andhung the test suite: the loop advances
offbelow that block, so skippingthe iteration left the cursor unmoved and spun on the same descriptor forever.
if letkeeps the advance on the normal path.Trading a panic for an infinite loop would have been a worse bug than the one
being fixed — and only running the test caught it.
Reproducers
The fixtures are the bytes libFuzzer actually found, taken from the failing
run's uploaded artifact rather than hand-authored, so they exercise the real
path rather than the one I'd have guessed.
cargo fuzz tmincould not reducethem: the size is what carries the offsets past the boundary.
They're kept as ordinary tests, not only as fuzz corpus — a defect found by
fuzzing shouldn't need fuzzing to be caught again. The fuzz job runs nightly;
the suite runs on every push.
Verification
cargo test --workspace— 0 failurescargo fmt --all -- --check,cargo clippy --workspace --all-targets -- -D warnings— clean