Fix unbounded ReadGlyph allocation (issue #191)#192
Open
parasol-aser wants to merge 1 commit intogoogle:masterfrom
Open
Fix unbounded ReadGlyph allocation (issue #191)#192parasol-aser wants to merge 1 commit intogoogle:masterfrom
parasol-aser wants to merge 1 commit intogoogle:masterfrom
Conversation
In ReadGlyph's simple-glyph branch, endPtsOfContours is treated as a uint16_t without enforcing monotonicity. A crafted sfnt with point_index < last_point_index wraps the subtraction and drives std::vector<Point>::resize to ~65535 entries per contour, producing multi-GB allocations and OOM DoS in ConvertTTFToWOFF2. Enforce the spec's monotonic-non-decreasing requirement on endPtsOfContours, and bound num_points by the remaining glyph buffer so allocations stay proportional to input size. Add a libFuzzer harness (convert_ttf2woff2_fuzzer) mirroring the reporter's setup, unit tests for ReadGlyph's per-contour guards, and an end-to-end test over fontTools-generated TTF fixtures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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
Fixes #191 — Unbounded
ReadGlyphallocation viauint16_twraparound inConvertTTFToWOFF2(DoS, High).ReadGlyph's simple-glyph branch (src/glyph.cc) never enforces the sfnt spec requirement thatendPtsOfContoursbe monotonically non-decreasing. All operands areuint16_t, so a crafted later endpoint less thanlast_point_indexwraps the subtraction and makesnum_pointsland near 65535. Withmaxp.numGlyphsup to 32767,std::vector<Glyph::Point>::resizeis driven to multi-GB allocations on a ~100 KB input — libFuzzer OOMs at 2.1–2.4 GB RSS inside a singleConvertTTFToWOFF2invocation. The decoder side (ReconstructGlyf) already has equivalent guards; this brings the encoder to parity.Fix
src/glyph.cc— two guards inside the per-contour loop:point_index < last_point_indexfori > 0(option (a) from the issue).num_points > len - buffer.offset()— each point consumes at least one flag byte downstream, so this is a trivially sound upper bound against future variants where the arithmetic is well-defined but the aggregate allocation is still disproportionate (option (b) from the issue).Both return
FONT_COMPRESSION_FAILURE(), which already propagates cleanly throughWriteNormalizedLoca → NormalizeGlyphs → NormalizeFont → NormalizeFontCollection → ConvertTTFToWOFF2.No public API change. No decoder-side change (already guarded).
Changes
src/glyph.cc— add monotonicity and per-contour size guards inReadGlyph.src/convert_ttf2woff2_fuzzer.cc(new) — libFuzzer harness overConvertTTFToWOFF2using the existingWOFF2Paramsoverload, matching the reporter's setup.CMakeLists.txt— registerconvert_ttf2woff2_fuzzernext toconvert_woff2ttf_fuzzer, and add an opt-in test suite (gated onBUILD_TESTING, auto-detects a Python withfontToolsfor fixture generation; silently skips with a status message otherwise).test/test_read_glyph.cc(new) — unit tests that driveReadGlyphdirectly with crafted glyf payloads (non-monotonic endpoints, num_points exceeding remaining buffer, valid monotonic cases).test/test_convert_ttf2woff2.cc(new) — end-to-end test overConvertTTFToWOFF2.test/generate_fixtures.py(new) — fontTools-based generator for a minimal malformed TTF (numGlyphs == 2, each glyph withendPtsOfContours == [5, 2]) plus a valid TTF baseline.Test plan
convert_ttf2woff2_fuzzeron a crafted ~100 KB sfnt matching the reporter's pattern reproducesout-of-memory (~2149 Mb; limit: 2048 Mb)with the stack from [Fuzzing] DoS: Unbounded ReadGlyph allocation via uint16_t wraparound in ConvertTTFToWOFF2 #191.test_read_glyphcovers: non-monotonic endpoints (rejected), equal endpoints (accepted — intentional, some real fonts rely on this tolerance),num_pointsexceeding remaining buffer (rejected), valid monotonic endpoints (accepted).test_convert_ttf2woff2runsConvertTTFToWOFF2over the generated fixtures and asserts the malformed input is rejected while the valid baseline round-trips.woff2_compressoutput on a known-good corpus (e.g. Google Fonts) must be identical pre- vs post-fix — the fix only rejects inputs that previously triggered undefined-by-spec wraparound.🤖 Generated with Claude Code