1253664 - Critical heap buffer overflow in graphite2, possible RCE

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[laf.intel]

Attachment: 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

[:Gijs (he/him)]

Pinging Milan to get this on the gfx radar.

Comment 2

[Tyson Smith [:tsmith]]

Attachment: call_stack.txt

Comment 3

[Jonathan Kew [:jfkthame]]

Un-pinging Milan, and flagging Martin instead.

Comment 4

[Tyson Smith [:tsmith]]

I was able to reproduce this on the latest revision of graphite 520d76818052772d614e581dacea69499b912be6

Comment 5

[martin_hosken]

Fixed? upstream in 0531c66a35329985e31483236e2ea17a3068697d

Comment 6

[Tyson Smith [:tsmith]]

Verified with graphite revision 4dc0e5bab12f0da9c9857dfec0612e57802af808

Comment 8

[Daniel Veditz [:dveditz]]

Graphite2 has been updated on all affected branches including ESRs.