1350047 - (CVE-2017-7771) Graphite2: out of bounds read [@ graphite2::Pass::readPass]

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect, P3)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.ttf

Found with a 32bit build running gr2FontTest.

Invalid read of size 1
   at 0x807EBDD: _peek<1> (Endian.h:77)
   by 0x807EBDD: read<unsigned char> (Endian.h:60)
   by 0x807EBDD: graphite2::Pass::readPass(unsigned char const*, unsigned int, unsigned int, graphite2::Face&, graphite2::passtype, unsigned int, graphite2::Error&) (Pass.cpp:102)
   by 0x80650B2: graphite2::Silf::readGraphite(unsigned char const*, unsigned int, graphite2::Face&, unsigned int) (Silf.cpp:216)
   by 0x80555B7: graphite2::Face::readGraphite(graphite2::Face::Table const&) (Face.cpp:149)
   by 0x804EF04: (anonymous namespace)::load_face(graphite2::Face&, unsigned int) (gr_face.cpp:59)
   by 0x804FCE8: gr_make_face_with_ops (gr_face.cpp:89)
   by 0x804FCE8: gr_make_file_face (gr_face.cpp:242)
   by 0x804C77A: Parameters::testFileFont() const (gr2FontTest.cpp:639)
   by 0x804D936: main (gr2FontTest.cpp:798)
 Address 0xe3406f0a is not stack'd, malloc'd or (recently) free'd


Process terminating with default action of signal 11 (SIGSEGV)
 Access not within mapped region at address 0xE3406F0A
   at 0x807EBDD: _peek<1> (Endian.h:77)
   by 0x807EBDD: read<unsigned char> (Endian.h:60)
   by 0x807EBDD: graphite2::Pass::readPass(unsigned char const*, unsigned int, unsigned int, graphite2::Face&, graphite2::passtype, unsigned int, graphite2::Error&) (Pass.cpp:102)
   by 0x80650B2: graphite2::Silf::readGraphite(unsigned char const*, unsigned int, graphite2::Face&, unsigned int) (Silf.cpp:216)
   by 0x80555B7: graphite2::Face::readGraphite(graphite2::Face::Table const&) (Face.cpp:149)
   by 0x804EF04: (anonymous namespace)::load_face(graphite2::Face&, unsigned int) (gr_face.cpp:59)
   by 0x804FCE8: gr_make_face_with_ops (gr_face.cpp:89)
   by 0x804FCE8: gr_make_file_face (gr_face.cpp:242)
   by 0x804C77A: Parameters::testFileFont() const (gr2FontTest.cpp:639)
   by 0x804D936: main (gr2FontTest.cpp:798)

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Martin, one more for you :)

Comment 2

[martin_hosken]

Not sure how this one triggered a bug. Running this rejects the font due to lack of any glyphs, before any passes are read. Can you remind me of the command line you used to run this?

Comment 3

[Tyson Smith [:tsmith]]

(In reply to martin_hosken from comment #2)
> Not sure how this one triggered a bug. Running this rejects the font due to
> lack of any glyphs, before any passes are read. Can you remind me of the
> command line you used to run this?

This was found on a 32bit build using the following command line:
valgrind -q ./gr2fonttest -auto -demand -noprint test_case.ttf

Comment 4

[martin_hosken]

And when I run that (albeit without valgrind), I get "Invalid font, failed to read or parse tables" which triggers before any loading of any passes and therefore the backtrace never fires. Are you running with the latest code (at least to before today)?

Comment 5

[Tyson Smith [:tsmith]]

(In reply to martin_hosken from comment #4)
> And when I run that (albeit without valgrind), I get "Invalid font, failed
> to read or parse tables" which triggers before any loading of any passes and
> therefore the backtrace never fires. Are you running with the latest code
> (at least to before today)?

Yes I was using the latest code and this *needs* to be run under valgrind to detect the issue.

Comment 6

[martin_hosken]

But I can't get it to get to that piece of code. I set a breakpoint on the first time the line valgrind complains about is ever hit and the program exits long before that because the font fails another test somewhere else (no glyphs in the font). Any ideas?

Comment 7

[Tyson Smith [:tsmith]]

Looks like it is compiler dependent. 

Building a 32bit build with Clang 3.8 I can reproduce the issue.
Building a 32bit build with GCC 5.4.0 I can NOT reproduce the issue.

Comment 8

[Jonathan Kew [:jfkthame]]

That's a bit worrying in itself; assuming valgrind doesn't warn about using an uninitialized value earlier (which could explain why the two builds take different paths), it sounds like we could be running into an undefined-behavior situation somewhere along the way.

If you change the optimization level, does that affect reproducibility (with either compiler)?

Comment 9

[Tyson Smith [:tsmith]]

FWIW I did try running this test case under a 32bit UBSan build but it didn't report any issues

Comment 10

[martin_hosken]

I'm busy most of today and installing clang 32-bit on my 64-bit Ubuntu 16.04 is not coming together. But I have committed a speculative fix. So if you get a chance to give it a try, then I might have hit lucky :)

Comment 11

[Daniel Veditz [:dveditz]]

ni? tyson to check out comment 10

Comment 12

[Tyson Smith [:tsmith]]

I verified this bug is no longer reproducible with the latest commit (8afc7d0) in the graphite repo. However this test case now triggers bug 1355148

Comment 13

[Tyson Smith [:tsmith]]

Actually this is still reproducible and I don't need Valgrind to detect it now.

STR:
Run in gdb with a 32-bit debug build.
./g2fonttest test_case.ttf -auto -demand -noprint

Program received signal SIGSEGV, Segmentation fault.
graphite2::Pass::readPass (this=<optimized out>, 
    pass_start=0x880abd48 <error: Cannot access memory at address 0x880abd48>, 
    pass_length=2147483795, subtable_base=<optimized out>, face=..., pt=<optimized out>, 
    version=131072, e=...) at src/Pass.cpp:102
102	    const byte flags = be::read<byte>(p);
(gdb) bt
#0  graphite2::Pass::readPass (this=<optimized out>, 
    pass_start=0x880abd48 <error: Cannot access memory at address 0x880abd48>, 
    pass_length=2147483795, subtable_base=<optimized out>, face=..., pt=<optimized out>, 
    version=131072, e=...) at src/Pass.cpp:102
#1  0x080646b0 in graphite2::Silf::readGraphite (this=<optimized out>, 
    silf_start=<optimized out>, lSilf=<optimized out>, face=..., version=<optimized out>)
    at src/Silf.cpp:216
#2  0x080551d8 in graphite2::Face::readGraphite (this=<optimized out>, silf=...)
    at src/Face.cpp:149
#3  0x0804f27e in (anonymous namespace)::load_face (face=..., options=<optimized out>)
    at src/gr_face.cpp:59
#4  gr_make_face_with_ops (appFaceHandle=<optimized out>, ops=<optimized out>, 
    faceOptions=<optimized out>) at src/gr_face.cpp:89
#5  0x080502c2 in gr_make_file_face (filename=<optimized out>, faceOptions=134811874)
    at src/gr_face.cpp:242
#6  0x0804c6eb in Parameters::testFileFont (this=0xffffcf08)
    at gr2fonttest/gr2FontTest.cpp:639
#7  0x0804d9ed in main (argc=<optimized out>, argv=<optimized out>)
    at gr2fonttest/gr2FontTest.cpp:798

Comment 14

[Tyson Smith [:tsmith]]

Attachment: test_case.ttf

Comment 15

[martin_hosken]

This seems now to be the same core issue being considered in bug 1355174 where passes_start is passing its range test when it shouldn't be.

Comment 16

[Stephanie Ouillon [:arroway] (needinfo me)]

bug 1355174 ad bug 1355148 have been fixed, does this bug still reproduce?

Comment 17

[Tyson Smith [:tsmith]]

Verified fixed in graphite commit 090076bf4