Replace compression-ratio heuristic with absolute uncompressed-size bound - #204
Draft
rootkiller6788 wants to merge 1 commit into
Draft
Replace compression-ratio heuristic with absolute uncompressed-size bound#204rootkiller6788 wants to merge 1 commit into
rootkiller6788 wants to merge 1 commit into
Conversation
…ound
The decoder rejected valid fonts whose claimed uncompressed size was more
than 100x their on-disk size ("Implausible compression ratio"). Legitimate
highly-compressible fonts, e.g. subset fonts with many repeated empty glyphs
used to cover the whole Unicode range (issue google#184), can exceed any fixed
ratio: a 65534-glyph font compresses from ~533KB to 277 bytes (~1924x).
The ratio check was introduced as a decompression-bomb guard, but it is
both bypassable and wrong. The compressed size is equally attacker-controlled
(an attacker can pad the file to make any ratio pass), so it never bounded
the real allocation anyway; and the quantity that actually needs bounding is
the intermediate buffer sized by hdr.uncompressed_size. Bound that against
kDefaultMaxSize, the same limit the reconstructed output is already subject
to: a font that can decode successfully never needs an uncompressed buffer
larger than its output, so this cannot reject a decodable font, while a
claim above the limit is rejected before any allocation.
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.
Fixes #184
Problem
woff2_decompress(and the decoder used by browsers) rejects valid fonts with:Issue #184 shows a real use case: a font that covers Unicode planes 0, 1 and 15 with a pool of N identical empty glyphs (cmap format 12). Such a font is valid per the spec, and its WOFF2 compresses extremely well because the glyph data is repeated. At N=65534 it compresses from ~533KB to 277 bytes (~1924x), and the ratio check rejects it.
I reproduced this locally: a 8000-glyph font compressing from 65457 to 248 bytes (~264x) was rejected with "Implausible compression ratio 202.0"; with the fix it round-trips byte-for-byte.
Why the ratio check is the wrong guard
The check (added in 2016 as a decompression-bomb guard for the
std::vector<uint8_t> uncompressed_buf(hdr.uncompressed_size)allocation) rejects whenuncompressed_size / file_size > 100. Two problems:file_sizeis just as attacker-controlled asuncompressed_size; an attacker can pad the file so the ratio stays under 100 while claiming gigabytes. It therefore never actually bounded the allocation.The fix
Bound the quantity that actually sizes the allocation directly: reject
hdr.uncompressed_size > kDefaultMaxSize(128MB), the same limit the reconstructed output is already subject to viaWOFF2StringOut/WOFF2MemoryOut.A font that can decode successfully reconstructs to at most
kDefaultMaxSize, and a valid font's transformed table data is never larger than its reconstructed output, so this bound cannot reject a decodable font. A claim above the limit is rejected before any allocation happens.Verification