font/cff: bound INDEX count/offSize to prevent huge allocation - #267
Conversation
parseIndexContent did make([][]byte, count) using count and offSize taken directly from the font. The CFF2 INDEX header parser (indexStart.mustParse) does not validate offSize the way the CFF1 path does, so a malformed font presenting offSize 0 zeroes offsetArraySize and defeats the length check, and a 32-bit count near 0xFFFFFFFF wraps in count+1. Either drives a multi-GB allocation before the loop errors out — e.g. macOS SFIndia.ttc makes NewFont allocate ~58GB, OOM-killing the process. Reject offSize outside 1..4 and bound count by the bytes available for the offset array before allocating. Adds TestParseIndexContentBounds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
whereswaldon
left a comment
whereswaldon
left a comment
There was a problem hiding this comment.
Fixing this potential huge allocation is awesome, thank you very much for tracking this problem down! I do wonder whether we should fix this in the parser though. The font parsers are @benoitkugler's area of expertise, not mine, so I'm happy to defer to his judgement about which layer should handle this issue.
| } | ||
| oSize := int(header.offSize) | ||
| offsetArraySize := int(header.count+1) * oSize | ||
| // offSize must be 1..4 (CFF spec, 5176 §5). The CFF2 INDEX header parser |
There was a problem hiding this comment.
Did you evaluate whether we could instead validate this in the CFF2 INDEX header parser? Or is there a limitation there that prevents validating at parse-time?
There was a problem hiding this comment.
I have not considered the CFF2 INDEX header parser. Good question. I'll check.
There was a problem hiding this comment.
The CFF2 header parser is the generated indexStart.mustParse (cff_gen.go, DO NOT EDIT) — no error return, so it can't reject anything without changing the codegen. The offSize check could go in the hand-written parseIndex2, but the count bound has to stay in parseIndexContent: only CFF2 puts a full uint32 in count (CFF1 reads a uint16, so count+1 can't wrap), and by the time both paths reach parseIndexContent you can't tell them apart — it's also where count+1 and the make actually run. One check at that shared allocation site covers CFF1, CFF2, and any future caller. Can also add the offSize check to parseIndex2 for parse-time symmetry if you'd prefer.
| if oSize < 1 || oSize > 4 { | ||
| return nil, 0, fmt.Errorf("reading INDEX: invalid offSize %d", oSize) | ||
| } | ||
| if int(header.count) > len(src)/oSize { |
There was a problem hiding this comment.
Could you explain why this check (l 295) is required, given the following lines ?
There was a problem hiding this comment.
It's an overflow guard for 32-bit platforms.
There was a problem hiding this comment.
Thank you.
Do you think we can drop the next check then ? It seems to be logically equivalent.
There was a problem hiding this comment.
Not quite — they are not equivalent, and dropping the length check turns an error into a panic.
The count check floors, so it admits count == len(src)/oSize while (count+1)*oSize still exceeds len(src). Concretely with len(src)=3, oSize=1, count=3: 3 > 3 is false so the count check passes, offsetArraySize is 4, and without the length check src[4:] panics on a 3-byte slice.
Your instinct that one check should do the job is right, though, and the pair I wrote was also still soft on 32-bit — the exact case line 295 was meant to cover. There int is 32 bits, so int(0xFFFFFFFF) is -1: the count check passes, offsetArraySize goes negative, L < offsetArraySize passes, and make([][]byte, header.count) panics with "len out of range".
So I have replaced both with a single check in uint64:
size := (uint64(header.count) + 1) * uint64(oSize)
if uint64(len(src)) < size {
return nil, 0, fmt.Errorf("reading INDEX offsets: EOF: expected length: %d, got %d", size, len(src))
}
offsetArraySize := int(size)Exact rather than floored, cannot wrap on either word size, and passing it implies size <= len(src) so the conversion to int is safe and count is bounded for the make. One error instead of two.
The offSize 1..4 check has to stay separate: offSize == 0 makes size zero, which satisfies any length check while make([][]byte, count) still allocates unbounded — that is the original SFIndia.ttc path.
Pushed as 4b2141b, with tests added for the floor-slack case and for offSize > 4.
benoitkugler
left a comment
benoitkugler
left a comment
There was a problem hiding this comment.
Thank you very much for the detailed report and the fix.
I have a remaining question, but the overall design looks great.
|
Knudge 😄 |
Replace the count check plus the offset-array length check with a single uint64 computation of (count+1)*offSize. The two checks were not equivalent -- the count check floors, so count == len(src)/offSize passed it while (count+1)*offSize still exceeded len(src) -- and the uint64 arithmetic removes the 32-bit wrap the int conversion left open. The offSize 1..4 check stays: offSize 0 zeroes the size and would let make([][]byte, count) allocate unbounded.
benoitkugler
left a comment
benoitkugler
left a comment
There was a problem hiding this comment.
This looks great, thank you !
|
This is great work. Thanks so much for the detailed explanations that accompany the fix! |
|
Could we release a new version following this fix? Also ran into this problem and would prefer not to target main directly |
* font/cff: resolve CFF2 local subrs relative to the Private DICT The Subrs operand in a Private DICT is an offset "relative to the beginning of the Private DICT data" (5176.CFF.pdf section 15, CFF2 section 10). ParseCFF2 was using it as an offset from the start of the table, so any CFF2 font declaring local subroutines had its Local Subrs INDEX read from an arbitrary position. The CFF1 parser already gets this right, in parsePrivateDict. What the wrong position yields is a matter of luck. On the repository's own toys/CFF2-VF.otf it is a byte giving offSize 140, so the table is rejected. Of the three CFF2 fonts shipped with macOS that declare local subroutines, two present counts of 28 and 58 million and fail the length check, and ZitherIndia.otf presents offSize 0 -- which before #267 fell through to bigEndian(src[0:0]) and panicked with "unreachable", and now errors. None of that surfaced, because NewFont discards the error from loadCff2: an affected font loads with no CFF2 outlines rather than failing. It also had no test coverage. The only other CFF2 fixture, common/NotoSansCJKjp-VF.otf, has 18 font DICTs and no local subroutines, so it never reaches this code, and the existing user of CFF2-VF.otf (TestVar) exercises fvar normalization only and never asks for a glyph. With the offset corrected, all four parse and every glyph interprets: toys/CFF2-VF.otf 5 charstrings, 3 local subrs ZitherIndia.otf 5683 charstrings, 17582 local subrs ZitherMalayalam.otf 198 charstrings, 485 local subrs ZitherTamil.otf 647 charstrings, 770 local subrs TestCFF2LocalSubrs pins it on CFF2-VF.otf, asserting the font declares subroutines and that every glyph loads -- the latter being what actually exercises them, since a charstring reaching callsubr with a wrongly located INDEX fails there. It fails without this change with "reading INDEX: invalid offSize 140". parseIndex2 also gains a guard against a negative offset, which its length check passes trivially and which would then panic on src[offset:]. That is newly reachable now that the offset is a sum of two values read from the font. * font/cff: add a synthetic CFF2 pinning the local subrs offset TestCFF2LocalSubrs covers the fix on toys/CFF2-VF.otf, but only because that font happens to be affected. This adds a case that pins the rule itself rather than relying on a fixture to embody it. buildCFF2LocalSubrs assembles a 63-byte CFF2 table whose Private DICT declares local subroutines, laid out so that resolving the Subrs operand from the start of the table lands inside the Top DICT, on a byte that is not a legal offSize. Real fonts differ only in what the wrong address happens to contain -- an offSize of 0, or a count of tens of millions -- and in all of those cases the visible effect is the same, because NewFont discards the error from loadCff2 and the font loads with no CFF2 outlines. Being built rather than read also means the case does not depend on a font file that can be redistributed, which the fonts that first showed this cannot be: they ship with macOS but are commercial and licensed, so neither the files nor their CFF2 tables can go in a repository.
5 participants
parseIndexContentcallsmake([][]byte, count)withcountandoffSizeread directly from the font. The CFF2 INDEX header parser (indexStart.mustParse) doesn't validateoffSizethe way the CFF1 path (parseIndexHeader) does. So a malformed font can present:offSize == 0→offsetArraySizebecomes 0, defeating thelen(src) < offsetArraySizelength check, orcountnear0xFFFFFFFF→count+1wraps to 0 inuint32, same effect.Either drives a multi-gigabyte allocation before the loop errors out. A concrete real-world trigger: macOS
SFIndia.ttcmakesfont.NewFontallocate ~58 GB, OOM-killing the process (found while shipping a pure-Go terminal that renders arbitrary Unicode).Fix: reject
offSizeoutside 1–4 and boundcountby the bytes available for the offset array before allocating. AddsTestParseIndexContentBounds. Existingfont/cfftests pass.