If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Critical heap buffer overflow in graphite2, possible RCE

RESOLVED FIXED

Status

()

Core
Graphics: Text
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: laf.intel, Unassigned)

Tracking

(4 keywords)

47 Branch
crash, csectype-bounds, sec-critical, testcase
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox45 disabled, firefox46 fixed, firefox47 fixed, firefox48+ fixed, firefox-esr3846+ disabled, firefox-esr4546+ disabled)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8726832 [details]
testcase.zip

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.116 Safari/537.36

Steps to reproduce:

The graphite2 library in Firefox contains a heap buffer overflow during parsing of font files. 
Attached are two files, foo.html and font file (xxx.ttf) which trigger the overflow in Firefox 47 Debug asan.
Loading the HTML file in Firefox 45 crashes even without asan, Firefox 47 doesn't crash without asan.

All functions involved in provoking the bug are within gfx/graphite2/src/GlyphCache.cpp
Below is a code excerpt showing the conditions triggering the bug.

line 117: GlyphCache::GlyphCache(const Face & face, const uint32 face_options)
line 118: _glyph_loader(new Loader(face, bool(face_options & gr_face_dumbRendering)))
|
|-> line 228: GlyphCache::Loader::Loader(const Face & face, const bool dumb_font)
|   ...
|   line 302 and following:
|   else if (version >= 0x00030000)
|   {
|       unsigned int glatflags = be::read<uint32>(p);
|       has_boxes = glatflags & 1;
|       // delete this once the compiler is fixed
|       has_boxes = true;
|   }
|
|   Thus, if version >= 0x00030000 the variable has_boxes is true.
|
...
line 167:if (_glyphs && glyph(0) == 0)
|
|-> GlyphCache::glyph()
    line 217: if (g)  p = _glyph_loader->read_glyph(glyphid, *g, &numsubs);
    |
    |--> GlyphCache::Loader::read_glyph()
    |   line 393:
    |   if (glat_version == 0x00030000) {
    |       ...
    |   }
    |
    |   The code in the above if statement doesn't executed because we set the version > 0x00030000
    |   therefore numsubs calculated within the if block will be 0
    |
    ...
    line 223: if (_boxes)
    since _boxes is true we execute the code within the if block
    |
    line 225:  _boxes[glyphid] = (GlyphBox *)gralloc<char>(sizeof(GlyphBox) + 8 * numsubs * sizeof(float));
    numsubs is still 0 and the allocation allocates only sizeof(GlyphBox) bytes to _boxes[glyphid] 
    Therefore the dynamically sized _subs field of the GlyphBox is just one element large
    line 226: if (!_glyph_loader->read_box(glyphid, _boxes[glyphid], *_glyphs[glyphid]))
    |
    |--> GlyphCache::Loader::read_box()
        line 461: uint16 bmap = be::read<uint16>(p);
        ...
        line 471: 
        for (int i = 0; i < num * 2; ++i)
        {
            Rect box = readbox((i & 1) ? diamax : bbox, p[0], p[2], p[1], p[3]);
            curr->addSubBox(i >> 1, i & 1, &box);
            |
            |-> void addSubBox(int subindex, int boundary, Rect *val) { _subs[subindex * 2 + boundary] = *val; }
            // BOOM _subs is one element large and due to the controlled loop counter we override
            // the heap with arbitrary data 

credit to Frederic Besler @ laf.intel


Actual results:

Crash in Firefox 47 Debug asan
Crash in Firefox 45 Debug (without asan)


Expected results:

No heap overflow :)

Comment 1

2 years ago
Pinging Milan to get this on the gfx radar.
Group: firefox-core-security → core-security
Component: Untriaged → Graphics
Flags: needinfo?(milan)
Product: Firefox → Core
Created attachment 8726900 [details]
call_stack.txt

Updated

2 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, csectype-bounds, testcase

Updated

2 years ago
Component: Graphics → Graphics: Text
Un-pinging Milan, and flagging Martin instead.
Flags: needinfo?(milan) → needinfo?(martin_hosken)

Updated

2 years ago
Group: core-security → gfx-core-security
I was able to reproduce this on the latest revision of graphite 520d76818052772d614e581dacea69499b912be6

Comment 5

2 years ago
Fixed? upstream in 0531c66a35329985e31483236e2ea17a3068697d
Flags: needinfo?(martin_hosken)
Verified with graphite revision 4dc0e5bab12f0da9c9857dfec0612e57802af808
Whiteboard: [gfx-noted]
Flags: sec-bounty?
Keywords: sec-critical

Updated

2 years ago
Depends on: 1255158
status-firefox45: --- → wontfix
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox-esr38: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox48: --- → +

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Group: gfx-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Graphite2 has been updated on all affected branches including ESRs.
status-firefox46: affected → fixed
status-firefox47: affected → fixed
status-firefox48: affected → fixed
status-firefox-esr38: affected → fixed
status-firefox-esr45: affected → fixed
tracking-firefox-esr38: --- → 46+
tracking-firefox-esr45: --- → 46+
status-firefox45: wontfix → disabled
status-firefox-esr38: fixed → disabled
status-firefox-esr45: fixed → disabled
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.