Fix fuzzer-found crashes and hangs in the demuxer#52
Open
claytercek wants to merge 6 commits into
Open
Conversation
Fuzzing the demuxer's open/parse path with libFuzzer found several null-pointer derefs, an integer-overflow-driven heap overflow, and multi-GB alloc/leak issues, all reachable from a few hundred bytes of crafted input: - sample_to_chunk() treated chunk_count==0 the same as the legitimate single-chunk case, returning chunk 0 into a chunk_offset array that was never allocated. - OD_DCD/OD_DSI, BOX_stts, BOX_stsz/stz2, BOX_stsc, BOX_stco/co64, and BOX_avcC all dereferenced the current track (tr) without checking it was non-NULL. g_fullbox[]'s use_track_flag check doesn't actually guard this: at depth 0 (a box appearing with no enclosing moov/trak) its ERROR() call just breaks out of the unrelated lookup loop instead of aborting the parse, so a lone top-level box of the wrong type reaches these cases with no track allocated. - BOX_stts's k+sc bound check used 32-bit arithmetic that could wrap past ts_count, skipping the resize while the write loop still walked `sc` entries. - Every parse-time malloc/realloc trusted a file-supplied count/size directly, so a few hundred bytes of input could request multi-GB allocations. Routed all of them through one bounded helper. - MALLOC() didn't free() the previous block, so a same-track box occurring twice in a malformed file would leak the first allocation. - MP4D_frame_offset dereferenced tr->timestamp/tr->duration even when a track has samples (stsz) but no stts box, leaving them NULL.
More fuzzing turned up: - BOX_stts/stsz/stz2/stsc/stco/co64/avcC/mdhd all dereferenced the current track unconditionally. g_fullbox[]'s use_track_flag check doesn't actually prevent this: at depth 0 (box with no enclosing moov/trak) its ERROR() call only breaks the unrelated lookup loop instead of aborting the parse, so a lone top-level box of the wrong type reaches these cases with tr still NULL. - sample_to_chunk() rescanned its chunk table from scratch on every call; since a sequential demux queries every sample index in increasing order, this made a full demux O(chunk_count * sample_count). Same for MP4D_frame_offset's own in-chunk byte-offset accumulation, which independently rescanned from a chunk's first sample every call (hits even when sample_to_chunk never rescans, e.g. one giant chunk). Both now resume from a per-track cache instead of restarting, since every caller here queries samples in non-decreasing order. A 2M-sample worst-case file went from a 15s+ hang to <200ms. - MP4D_frame_offset could read past tr->timestamp/tr->duration when a track's own stts declared fewer samples than its stsz did.
Reallocating to the exact new size on every stts entry meant a file whose entries each grew the running total by only a little forced a full realloc (and full copy of everything so far) per entry -- a fuzzer-found multi-second hang distinct from the single-oversized- allocation case already bounded by MINIMP4_MAX_ALLOC_BYTES. Doubling capacity instead amortizes the cost, same rationale as std::vector.
count*elemsize computed in native 32-bit arithmetic (entry_size, timestamp/duration, DSI, and tag-string buffers) can overflow/wrap before ever reaching minimp4_bounded_malloc's size check, allocating far fewer bytes than the subsequent loop then writes. Cast to uint64_t before the multiply/cast so the bound check sees the true size. sample_to_chunk/chunk_offset were already safe since sizeof() promotes those products to size_t.
sample_to_chunk()'s chunk_count==0 and ==1 fast paths, and its general scan, can all return a numerically valid chunk index while tr->chunk_offset or tr->entry_size stayed NULL: chunk_count and sample_count get set before their matching MALLOC in BOX_stco/co64 and BOX_stsz/stz2, so a malformed file that hits that MALLOC's out-of-memory path (e.g. a duplicate box declaring an oversized count the second time, after free()-ing the prior valid allocation) can leave a numerically-set count paired with a NULL array -- especially since, at depth 0, ERROR() doesn't actually abort the parse. Guard directly in MP4D_frame_offset before either array is indexed. Fuzzer found this as a null-pointer deref from a crafted file with duplicate stco boxes.
count is a file-declared value with nothing to bound it against (ctts doesn't allocate); once the box's real payload runs out, READ() just returns zero-padding forever instead of stopping, so a file could claim ~4 billion entries in a handful of bytes on disk. Fuzzer found this as a multi-second hang. Bail once payload_bytes is exhausted.
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
1 participant
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.
We're embedding minimp4 in a video playback extension and ran the demux path through libFuzzer with ASan/UBSan. It turned up a handful of ways a malformed file can crash or hang the parser, all reachable from a few hundred bytes of crafted input:
stts/stsz/stsc/stcoappearing outside any moov/trak get parsed withtrstill NULL. Theuse_track_flagcheck looks like it guards this, but at depth 0 itsERROR()just breaks out of theg_fullbox[]lookup loop instead of aborting the parse, so a lone top-level box of the wrong type walks straight into a null deref.count * elemsize) can wrap before the malloc, so the parse loop then writes past a too-small buffer.sample_to_chunk()andMP4D_frame_offset()rescan their tables from scratch on every call, which makes a sequential demux O(chunk_count * sample_count). Combined with realloc-to-exact-size on every stts entry, crafted files produce multi-second hangs.BOX_cttstrusts its file-declared entry count with nothing bounding it against the actual payload, so once the box runs outREAD()just returns zero-padding for what can be ~4 billion entries.Each commit fixes one of these, with full details in the commit messages.
One tradeoff worth flagging: parse-time allocations are now capped at 256MB each (
MINIMP4_MAX_ALLOC_BYTES), so a legitimately enormous file could get rejected where it previously parsed. It's a single enum in the header if that bound is wrong for your use case. The perf fixes shouldn't change behavior for well-formed files; the chunk-scan cache assumes samples are queried in increasing order and falls back to a full rescan when they aren't.I know the repo's been quiet for a while, so no worries if this never lands. Mostly leaving it here so other folks embedding minimp4 can find the fixes; they live on our fork either way.