webpmeta: bound ICC profile allocation by stream contents - #19
Open
ChrisJr404 wants to merge 1 commit into
Open
ChrisJr404 wants to merge 1 commit into
ChrisJr404 wants to merge 1 commit into
Conversation
readICCP pre-allocated a buffer sized directly from the ICCP chunk's untrusted 32-bit length field before reading any data, so a tiny WebP declaring a huge ICCP length forced an allocation of up to ~4GB (the surrounding recover only turns panics into errors and does not cover out-of-memory). Read the profile incrementally with io.CopyN into a bytes.Buffer so memory tracks the bytes actually present and a truncated stream fails without a large allocation. Signed-off-by: Chris (ChrisJr404) <11917633+ChrisJr404@users.noreply.github.com>
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.
When extracting metadata from a WebP file,
readICCPallocated a buffer sized directly from the ICCP chunk header's 32-bit length field before reading anything, so a truncated file that simply declares a large length forces a correspondingly large allocation.Failure mode: a ~40-byte WebP whose ICCP chunk claims a length of
0xFFFFFFFFmakesreadICCPdomake([]byte, ch.Length)(~4GB) up front. Therecover()inextractMetadataonly converts panics into errors; it does not cover the resulting out-of-memory.autometa.Loadreaches this path for any RIFF/WEBP input, so a single small crafted file can exhaust memory.Repro (declaring 512 MiB, provided no data) shows ~536 MB allocated for a ~40-byte input before the fix.
Fix: read the profile incrementally with
io.CopyNinto abytes.Bufferso memory tracks the bytes actually present in the stream. Valid files are unaffected (the declared length matches the data that follows); a truncated stream now fails without a large allocation.Test notes: added
TestExtractMetadataICCPAllocation, which declares a 512 MiB ICCP length with no following data and asserts the allocation stays bounded. It fails on the previous code (~536 MB allocated) and passes with this change.go test ./...andgo vet ./...are clean.