Repository navigation
LibGfx: Bounds-check CBLC index subtable array and header - #27095
Chandan3456 wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
👀 pr-needs-review
|
any update? |
| auto index_subtables_slice = m_slice.slice(bitmap_size.index_subtable_array_offset, bitmap_size.index_tables_size); | ||
|
|
||
| Checked<size_t> size_used_by_index_subtables = sizeof(EBLC::IndexSubTableArray); | ||
| size_used_by_index_subtables *= static_cast<u32>(bitmap_size.number_of_index_subtables); |
There was a problem hiding this comment.
the cast should not be needed, BigEndian<u32> should auto decay
There was a problem hiding this comment.
yep, it decays fine here. dropped the cast.
| Checked<size_t> size_used_by_index_subtables = sizeof(EBLC::IndexSubTableArray); | ||
| size_used_by_index_subtables *= static_cast<u32>(bitmap_size.number_of_index_subtables); | ||
| if (size_used_by_index_subtables.has_overflow() || index_subtables_slice.size() < size_used_by_index_subtables) { | ||
| dbgln("CBLC number of index subtables goes out of bounds"); |
There was a problem hiding this comment.
Don't quite like this dbgln,
the number in that sentence seems to throw me off
Also you could check against bitmap_size.index_tables_size and move it next to the other bounds check
There was a problem hiding this comment.
moved it up next to the other bounds check and it compares against index_tables_size now. reworded the dbgln to "CBLC index subtable array is larger than the index tables size" as well.
the comparison needs .value() on the Checked, since its operator< deduces T from both sides and won't compile against a BigEndian.
CBLC::index_subtable_for_glyph_id built a span of IndexSubTableArray records from the font's numberOfIndexSubTables without checking it against indexTablesSize, so iterating the span ran past the end of the table on a crafted colour-bitmap font. The IndexSubHeader read further down likewise assumed the remaining slice held a full header. Validate the count with Checked<size_t>, matching from_slice, and require sizeof(IndexSubHeader) bytes before the read.
77cfb4a to
d28dd57
Compare
while reading the opentype colour-bitmap code i noticed CBLC::index_subtable_for_glyph_id builds a ReadonlySpan of IndexSubTableArray records using numberOfIndexSubTables straight from the font, but only indexTablesSize worth of bytes is ever bounds-checked, so the range-for over that span walks off the end of the table when a font declares the count higher than the array it actually stores. the IndexSubHeader returned just below is read through a bit_cast without ensuring the remaining slice holds a full header either. both are reachable from embedded_bitmap_data_for_glyph while shaping a glyph from an untrusted cblc/cbdt font. the fix validates the subtable count against the slice with Checked<size_t>, the same way from_slice already does for the bitmap sizes, and requires sizeof(IndexSubHeader) bytes before the final read. asan on the added TestFontHandling case reports a heap-buffer-overflow read at the range-for on master and is clean afterwards.