1875367 - Read beyond bounds in OpenTypeGDEF::ParseLigCaretListTable()

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[mozillabugs]

OpenTypeGDEF::ParseLigCaretListTable() (gfx/ots/src/gdef.cc) can read from an incorrect area of the font file if the file contains a specially-crafted value of offset_device. While this read probably is harmless in itself, it means that any code that relies on ParseLigCaretListTable() to reject a defective table might be open to attack.

The bug is in lines 175-180, which don't check the value of offset_device before adding it to other quantities, then assign the result to the uint16_t absolute_offset, potentially truncating a sum > 0xffff and falsely allowing the if condition on line 181 to evaluate as true. (E.g., if offset_device == 0xffff, any nonzero value of the other quantities on line 179 causes truncation). The bogus absolute_offset is then used to validate the "device table" via a call to ots::ParseDeviceTable() (gfx/ots/src/layout.cc), which could declare the table valid if it can read 3 16-bit quantities from it, and the 3rd quantity == 0x8000.

Lines are from the tag FIREFOX_121_0_RELEASE.

     97: bool OpenTypeGDEF::ParseLigCaretListTable(const uint8_t *data, size_t length) {
     98:   ots::Buffer subtable(data, length);
    ...
    134: 
    135:   // Parse ligature glyph table
    136:   for (unsigned i = 0; i < lig_glyphs.size(); ++i) {
    ...
    158:     // Parse caret values table
    159:     for (unsigned j = 0; j < caret_count; ++j) {
    160:       subtable.set_offset(lig_glyphs[i] + caret_value_offsets[j]);
    161:       uint16_t caret_format = 0;
    162:       if (!subtable.ReadU16(&caret_format)) {
    163:         return Error("Can't read caret values table %d in glyph %d", j, i);
    164:       }
    165:       if (caret_format == 0 || caret_format > kMaxCaretValueFormat) {
    166:         return Error("bad caret value format: %u", caret_format);
    167:       }
    168:       // CaretValueFormats contain a 2-byte field which could be
    169:       // arbitrary value.
    170:       if (!subtable.Skip(2)) {
    171:         return Error("Bad caret value table structure %d in glyph %d", j, i);
    172:       }
    173:       if (caret_format == 3) {
    174:         uint16_t offset_device = 0;
    175:         if (!subtable.ReadU16(&offset_device)) {
    176:           return Error("Can't read device offset for caret value %d "
    177:                        "in glyph %d", j, i);
    178:         }
    179:         uint16_t absolute_offset = lig_glyphs[i] + caret_value_offsets[j]
    180:                                    + offset_device;
    181:         if (offset_device == 0 || absolute_offset >= length) {
    182:           return Error("Bad device offset for caret value %d in glyph %d: %d",
    183:                        j, i, offset_device);
    184:         }
    185:         if (!ots::ParseDeviceTable(GetFont(), data + absolute_offset,
    186:                                    length - absolute_offset)) {
    187:           return Error("Bad device table for caret value %d in glyph %d",
    188:                        j, i, offset_device);
    189:         }
    190:       }
    191:     }
    192:   }
    ...

I am working on a POC. It appears that the table parsed by this function is rarely deployed in web fonts.

Comment 1

[mozillabugs]

Attached is a POC that demonstrates the truncation and shows ParseLigCaretListTable() reading from an incorrect location in the font file. Because of https://bugzilla.mozilla.org/show_bug.cgi?id=1331737 , the POC uses a nondefault preference. Use it thusly:

1. Build FF debug.
2. Copy ffbug_2220.htm and ffbug_2220.ttf to a folder on a webserver.
3. Edit ffbug_2220.htm to point to wherever you put ffbug_2220.ttf.
4. Run FF and attach a debugger to it.
5. Set the preference gfx.downloadable_fonts.otl_validation to true.
6. Set a BP on line 175, below.
7. Load ffbug_2220.htm in FF.
8. When the BP fires, step line 175. Notice that the value read is 0xfffe .
9. Step through line 180 and notice that absolute_offset has been truncated to 0x000c.
10. Step into ParseDeviceTable() and notice that it reads from an incorrect offset in the font's GDEF table.

Comment 2

[mozillabugs]

Attachment: ffbug_2220.htm

Comment 3

[mozillabugs]

Attachment: ffbug_2220.ttf

Comment 4

[mozillabugs]

Reported to Google at https://bugs.chromium.org/p/chromium/issues/detail?id=1520968 .

Comment 5

[Daniel Veditz [:dveditz]]

What's the potential impact? We might base validator decisions on bogus data, and "pass" a font we should have rejected? Apparently we aren't too worried about what this validator is checking for if we've disabled it on release.

Comment 6

[Daniel Veditz [:dveditz]]

Ron: what severity did the chrome team assign to their version of the bug?

Comment 7

[mozillabugs]

(In reply to Daniel Veditz [:dveditz] from comment #6)

Ron: what severity did the chrome team assign to their version of the bug?

They haven't rated the bug yet.

Comment 8

[mozillabugs]

This bug appears to be latent in Chrome, because Chrome appears to hardcode bypassing OTS checks for the GDEF table (see BlinkOTSContext::GetTableAction in third_party\blink\renderer\platform\fonts\web_font_decoder.cc), unlike FF where it's under a preference.

Comment 9

[Daniel Veditz [:dveditz]]

This seems pretty benign though. Assuming we read within bounds so we don't crash this leads to us rejecting a font that we wouldn't have, even though it's trying to trick us into reading out of bounds?

Comment 10

[Jonathan Kew [:jfkthame]]

Presumably it could also lead to us accepting and using a font that we should have rejected, which means the underlying platform font code is exposed to a faulty table; the reason we validate font resources is because historically we couldn't trust that the (closed-source, in the case of Windows or macOS) platform code would handle invalid resources safely.

So while we don't know of any specific vulnerability this would expose, I would assume it has the potential to let a malicious (or just broken) font bypass one layer of our defences. (The idea of crafting a bad font table that both exploits the bug here to sneak past OTS checks and then exploits some unrelated flaw in the platform font code to do Bad Things seems a bit improbable to me. But I'm repeatedly amazed at the weird things people manage to come up with, so I don't want to dismiss the possibility.)

As noted, we disable this on release anyhow, to match Chrome behavior; given that we don't rely on closed-source platform libs to actually do the font shaping that would be the main client of GDEF (we use harfbuzz for that, across all platforms), the exposure of untrusted code is quite limited, and the compat pain of rejecting fonts with minor GDEF (or GSUB or GPOS) flaws was exceeding the perceived low risk of letting them through. But we can't guarantee that Core Text or DirectWrite won't look at the GDEF in any way, or what they'd do if it's invalid.

In short, we should definitely fix this, but my sense is that the actual risk it involves is pretty low.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:lsalzman, could you have a look please?

For more information, please visit BugBot documentation.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:jfkthame, could you have a look please?

For more information, please visit BugBot documentation.

Comment 13

[Jonathan Kew [:jfkthame]]

It looks like this can be trivially fixed by making the absolute_offset variable a size_t (to match length). It's computed by adding together three 16-bit values from the table, so the sum could overflow a uint16_t but won't overflow size_t for any architecture we care about.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1875367 - Make absolute_offset a size_t variable. r=#gfx-reviewers

Comment 15

[Jonathan Kew [:jfkthame]]

If we take this fix locally, I'll also open a PR to land it upstream in OTS (though I believe Chromium bypasses GDEF validation anyway, so it won't directly affect them).

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35bd4ace2aab
Make absolute_offset a size_t variable. r=gfx-reviewers,lsalzman

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/35bd4ace2aab

Comment 18

[Donal Meehan [:dmeehan]]

[Tracking Requested - why for this release]:

Comment 19

[Jonathan Kew [:jfkthame]]

I'm not sure how important it is to track (or potentially uplift) this? Note that this code is preffed-off for Beta & Release builds (see also comment 2), so it's only enabled by default on the Nightly channel.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jfkthame, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox125 to wontfix.

For more information, please visit BugBot documentation.

Comment 21

[Jonathan Kew [:jfkthame]]

Just FTR, https://github.com/khaledhosny/ots/pull/278 is the PR to apply this upstream.