1251869 - Graphite2 heap-based buffer overflow in GlyphCache::GlyphCache

Status: RESOLVED FIXED

Description

[Mateusz Jurczyk]

Created attachment 8724444 [details]
Reproducers.

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

Steps to reproduce:

After reading the stories at [1] and [2], and seeing the security bulletin at [3] and bug report at [4], I decided to give Graphite2 a shot myself. If this is not the right place to report bugs in the library, please let me know.

The following crash due to a heap-based buffer overflow can be observed in a slightly modified ASAN build of the standard Graphite2 gr2FontTest utility (git trunk), triggered with the following command:

$ ./gr2fonttest /path/to/file text

My change in gr2FontTest was to hardcode the tested text to include all characters in the 0x1..0xfff range, instead of having to specify them in command line. The patch is as follows:

--- cut ---
--- graphite_original/gr2fonttest/gr2FontTest.cpp	2016-02-27 19:35:16.308071127 +0100
+++ graphite/gr2fonttest/gr2FontTest.cpp	2016-02-26 13:57:13.389186376 +0100
@@ -437,7 +437,17 @@
     if (mainArgOffset < 1) argError = true;
     else if (mainArgOffset > 1)
     {
-        if (!useCodes && pText != NULL)
+        const unsigned int kCodeLimit = 0x1000;
+
+        charLength = kCodeLimit - 1;
+
+        pText32 = (unsigned int *)malloc(sizeof(unsigned int) * kCodeLimit);
+        for (unsigned int i = 1; i < kCodeLimit; ++i) {
+          pText32[i - 1] = i;
+        }
+        pText32[kCodeLimit - 1] = 0;
+
+        /*if (!useCodes && pText != NULL)
         {
             charLength = convertUtf<gr2::utf8>(pText, pText32);
             if (!pText32)
@@ -466,7 +476,7 @@
         {
             pText32[charLength] = 0;
             fprintf(log, "\n");
-        }
+        }*/
     }
     return (argError) ? false : true;
 }
--- cut ---

The resulting ASAN crash is as follows:

--- cut ---
==27575==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000efd0 at pc 0x00000055daad bp 0x7ffdfb0bfe90 sp 0x7ffdfb0bfe88
WRITE of size 8 at 0x60200000efd0 thread T0
    #0 0x55daac in graphite2::GlyphCache::GlyphCache(graphite2::Face const&, unsigned int) graphite/src/GlyphCache.cpp:133:20
    #1 0x549503 in graphite2::Face::readGlyphs(unsigned int) graphite/src/Face.cpp:98:29
    #2 0x56d3f4 in (anonymous namespace)::load_face(graphite2::Face&, unsigned int) graphite/src/gr_face.cpp:54:14
    #3 0x56cf04 in gr_make_face_with_ops graphite/src/gr_face.cpp:89:16
    #4 0x56f240 in gr_make_file_face graphite/src/gr_face.cpp:242:23
    #5 0x4ec193 in Parameters::testFileFont() const (graphite/gr2fonttest/gr2fonttest+0x4ec193)
    #6 0x4ef595 in main (graphite/gr2fonttest/gr2fonttest+0x4ef595)

0x60200000efd1 is located 0 bytes to the right of 1-byte region [0x60200000efd0,0x60200000efd1)
allocated by thread T0 here:
    #0 0x4b86dc in calloc llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:56
    #1 0x56ac8a in graphite2::GlyphFace const** graphite2::grzeroalloc<graphite2::GlyphFace const*>(unsigned long) graphite/src/./inc/Main.h:96:28
    #2 0x55cb26 in graphite2::GlyphCache::GlyphCache(graphite2::Face const&, unsigned int) graphite/src/GlyphCache.cpp:119:45
    #3 0x549503 in graphite2::Face::readGlyphs(unsigned int) graphite/src/Face.cpp:98:29
    #4 0x56d3f4 in (anonymous namespace)::load_face(graphite2::Face&, unsigned int) graphite/src/gr_face.cpp:54:14
    #5 0x56cf04 in gr_make_face_with_ops graphite/src/gr_face.cpp:89:16
    #6 0x56f240 in gr_make_file_face graphite/src/gr_face.cpp:242:23
    #7 0x4ec193 in Parameters::testFileFont() const (graphite/gr2fonttest/gr2fonttest+0x4ec193)
    #8 0x4ef595 in main (graphite/gr2fonttest/gr2fonttest+0x4ef595)

SUMMARY: AddressSanitizer: heap-buffer-overflow graphite/src/GlyphCache.cpp:133:20 in graphite2::GlyphCache::GlyphCache(graphite2::Face const&, unsigned int)
Shadow bytes around the buggy address:
  0x0c047fff9da0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9db0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9dc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9dd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9de0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c047fff9df0: fa fa 00 fa fa fa 01 fa fa fa[01]fa fa fa 00 04
  0x0c047fff9e00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9e10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9e20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9e30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9e40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==27575==ABORTING
--- cut ---

A cursory analysis shows that the direct reason of the crash is the wrong assumption made by GlyphCache::GlyphCache() that the _num_glyphs field is always greater than 0. A buffer is allocated in line 128:

--- cut ---
   128	        GlyphFace * const glyphs = new GlyphFace [_num_glyphs];
--- cut ---

And regardless of the _num_glyphs value, data is written to its first entry in line 133:

--- cut ---
   132	        // The 0 glyph is definately required.
   133	        _glyphs[0] = _glyph_loader->read_glyph(0, glyphs[0], &numsubs);
--- cut ---

While this could just end as an off-by-one error and a fixed ~8 byte overflow, it gets worse. The subsequent loop in lines 139-140 also assumes that _num_glyphs is non-zero, and additionally wrongly (in the context of the misassumption) uses the != operator instead of < in the loop end condition:

--- cut ---
   138	        const GlyphFace * loaded = _glyphs[0];
   139	        for (uint16 gid = 1; loaded && gid != _num_glyphs; ++gid)
   140	            _glyphs[gid] = loaded = _glyph_loader->read_glyph(gid, glyphs[gid], &numsubs);
--- cut ---

This essentially means that the size of the overflown area is fully controlled by an attacker; it must only be a multiple of the native word size: typically 4 or 8 bytes.

Attached are three font files which trigger the crash.

This bug is subject to a 90 day disclosure deadline. If 90 days elapse without a broadly available patch, then the bug report will automatically become visible to the public.

References:
[1] http://linux.slashdot.org/story/16/02/15/143206/vulnerability-in-font-processing-library-affects-linux-openoffice-firefox
[2] http://blog.talosintel.com/2016/02/vulnerability-spotlight-libgraphite.html
[3] https://www.mozilla.org/en-US/security/advisories/mfsa2016-14/
[4] https://bugzilla.mozilla.org/show_bug.cgi?id=1243832

Comment 1

[Tyson Smith [:tsmith]]

Reproduced with graphite revision fb32e2e39ccd0980672f55052f75fc10f8736fb2

Thanks for the report Mateusz! Do you have the original test cases that you were fuzzing?

Comment 2

[martin_hosken]

Fixed? in 92e0add6083b2e360e30854cbfceedecaac0d3ad

Comment 3

[Tyson Smith [:tsmith]]

Verified fixed in graphite revision 92e0add6083b2e360e30854cbfceedecaac0d3ad

Comment 4

[martin_hosken]

Added -auto to gr2fonttest in 97825c511bc0e9e8c241c5753da23b07635d36a3 to do the same thing so run gr2fonttest font.ttf -auto

Comment 5

[Mateusz Jurczyk]

Martin, thanks for the quick fix and the -auto option - I'll happily use it for any future testing of the tool.

Tyson, are you still interested in the original test cases? In general I used the samples available at http://scripts.sil.org/cms/scripts/page.php?site_id=projects&item_id=graphite_fonts as my initial corpus, but I can check which files were specifically used to create the inputs attached in this bug.

Comment 6

[martin_hosken]

fixed? before or in e7deaf90c9c8ca30116340419313af527fe90d78

Comment 7

[Tyson Smith [:tsmith]]

Verified with graphite revision 346fad7b12942e011b4db758d4df74a6a0d20105

Comment 8

[Liz Henry (:lizzard) (needinfo? me)]

Since we uplifted graphite2 1.3.7 in bug 1255158, should this be fixed in 46?

Comment 9

[Tyson Smith [:tsmith]]

Yes, 1.3.7 includes the fix this issue.

Comment 10

[Liz Henry (:lizzard) (needinfo? me)]

The other dependencies on 1255158 too, probably.

Comment 11

[Daniel Veditz [:dveditz]]

Graphite2 has been updated on all affected branches including ESRs.