Closed Bug 1350047 (CVE-2017-7771) Opened 3 years ago Closed 2 years ago

Graphite2: out of bounds read [@ graphite2::Pass::readPass]

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox-esr52 54+ fixed
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: tsmith, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-bounds, sec-high, testcase, Whiteboard: [gfx-noted][post-critsmash-triage][adv-main54+][adv-esr52.2+])

Attachments

(1 file, 1 obsolete file)

1.58 KB, application/x-font-ttf
Details
Attached file test_case.ttf (obsolete) —
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)
Martin, one more for you :)
Flags: needinfo?(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?
Flags: needinfo?(martin_hosken) → needinfo?(twsmith)
(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
Flags: needinfo?(twsmith)
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)?
Flags: needinfo?(twsmith)
(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.
Flags: needinfo?(twsmith)
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?
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.
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)?
FWIW I did try running this test case under a 32bit UBSan build but it didn't report any issues
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 :)
Priority: -- → P3
Whiteboard: [gfx-noted]
ni? tyson to check out comment 10
Flags: needinfo?(twsmith)
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
Flags: needinfo?(twsmith)
See Also: → 1355148
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
Attached file test_case.ttf
Attachment #8850653 - Attachment is obsolete: true
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.
Depends on: CVE-2017-7774
Keywords: sec-high
bug 1355174 ad bug 1355148 have been fixed, does this bug still reproduce?
Flags: needinfo?(twsmith)
Verified fixed in graphite commit 090076bf4
Flags: needinfo?(twsmith)
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Assignee: nobody → jfkthame
Target Milestone: --- → mozilla55
Flags: qe-verify-
Whiteboard: [gfx-noted] → [gfx-noted][post-critsmash-triage]
Group: gfx-core-security → core-security-release
Whiteboard: [gfx-noted][post-critsmash-triage] → [gfx-noted][post-critsmash-triage][adv-main54+][adv-esr52.2+]
Alias: CVE-2017-7771
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.