Bound sequence parser allocations to the input size - #14
Open
fordred wants to merge 1 commit into
Open
Conversation
The sequence sample-table parsers (parseStsz, parseOffsets, parseStsc, parseStts) sized slices directly from 32-bit counts read from the file, without checking that the input could back them. A tiny forged HEIF container (e.g. a 124-byte ftypmsf1 file with a pict track whose stsz box claims 2^29 samples) forced a ~4 GiB allocation inside DecodeConfig/Decode - an OOM vector on untrusted uploads. Allocations are now bounded by data actually present: - parseStsz: per-entry sizes must fit the box payload; a uniform table implies one byte of media data per sample, so its count is capped by the input size. - parseOffsets/parseStsc: entry counts capped by the box payload length. - parseStts: entry count capped by the payload; the expanded duration total capped by the input size. Adds same-package bounds tests for each parser plus a DecodeConfig regression test asserting the forged container allocates under 64 MiB. Legit sequences are unaffected (verified against the 17-frame anim.heic fixture).
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.
Summary
The HEIC sequence sample-table parsers (
parseStsz,parseOffsets,parseStsc,parseSttsinsequence.go) size their slices directly from 32-bit counts read from the file, without checking that the input can back them.Reproduction: a 124-byte
ftypmsf1file with apicttrack whosestszbox claimssample_count = 0x20000000forces a 4.0 GiB allocation insideheic.DecodeConfig(measured viaruntime.MemStats), before any decode or dimension check. A 10 MB body cap doesn't help — forged counts need no backing data.parseSttshas the same amplification via an unbounded inner loop over a per-entry count.Fix
Allocations are now bounded by data actually present in the input:
parseStsz— per-entry sizes must fit the box payload ((len(p)-12)/4); a uniform table implies one byte of media data per sample, so its count is capped by the input size.parseOffsets/parseStsc— entry counts capped by the box payload length.parseStts— entry count capped by the payload; the expanded duration total capped by the input size.parseTrakalready has the full input in hand (data), so the input-size bound is passed down with no signature churn to the public API. Worst-case allocation is now proportional to the input (~30x, from the tables themselves), instead of unbounded.Tests
TestDecodeConfigDoesNotAmplifyForgedSequenceCounts— the 124-byte forged container must fail fast allocating < 64 MiB (pre-fix: ~4 GiB).TestDecodeAll(17-frameanim.heic) passes; benches and thewasm2gobuild are clean.Found via KopiChange's review of a v0.7.1 consumer; the fix there guards at the boundary, this bounds the parser for every consumer.