Closed Bug 1355174 (CVE-2017-7774) Opened 3 years ago Closed 3 years ago

Graphite2: out of bounds read [@ graphite2::Silf::readGraphite]

Categories

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

defect
Not set
critical

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: [post-critsmash-triage][adv-main54+][adv-esr52.2+])

Attachments

(2 files)

Attached file test_case.ttf
Found with a 32bit build running gr2FontTest.

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

Program received signal SIGSEGV, Segmentation fault.
be::_peek<4> (p=<optimized out>) at src/inc/Endian.h:50
50          return _peek<S/2>(p) << (S/2)*8 | _peek<S/2>(p+S/2);
(gdb) bt
#0  be::_peek<4> (p=<optimized out>) at src/inc/Endian.h:50
#1  be::read<unsigned int> (p=<optimized out>)
    at src/inc/Endian.h:60
#2  graphite2::Silf::readGraphite (this=<optimized out>, silf_start=<optimized out>, 
    lSilf=<optimized out>, face=..., version=<optimized out>)
    at src/Silf.cpp:188
#3  0x080551d8 in graphite2::Face::readGraphite (this=<optimized out>, silf=...)
    at src/Face.cpp:149
#4  0x0804f27e in (anonymous namespace)::load_face (face=..., options=<optimized out>)
    at src/gr_face.cpp:59
#5  gr_make_face_with_ops (appFaceHandle=<optimized out>, ops=<optimized out>, 
    faceOptions=<optimized out>) at src/gr_face.cpp:89
#6  0x080502c2 in gr_make_file_face (filename=<optimized out>, faceOptions=4294967292)
    at src/gr_face.cpp:242
#7  0x0804c6eb in Parameters::testFileFont (this=0xffffcf08)
    at gr2fonttest/gr2FontTest.cpp:639
#8  0x0804d9ed in main (argc=<optimized out>, argv=<optimized out>)
    at gr2fonttest/gr2FontTest.cpp:798
Keywords: sec-high
Haven't managed to replicate the segfault. Looking at what might have caused a segfault at the line in the stack trace given, I see that passes_start is a huge number way beyond silf_end, and in my running of the code, this is caught at line 167 and a premature return happens and the font load fails gracefully.

So, not sure why in your situation, the test did not fail at line 167 and that the reading ploughed on towards a segfault.
I notice in your debug build that there are a number of values optimized out. That doesn't feel very debug build to me.

Example values of passes_start places it very high (as in 0xed753426) vs a lower silf_end (0x80d43178). Now perhaps on my machine because they are both above 0x8000000 they compare OK. But I would expect a sensible compiler, even on a 32-bit architecture to compare (via >=) two const * byte const pointers unsigned? Am I missing something here?
Duplicate of this bug: 1355148
I can reproduce this with a 32-bit Debug or Release build, built with clang3.8 (Ubuntu 16.04 default clang). Be sure to build with -O3 and run under Valgrind to consistently catch the issue. I verified I can still repro this with commit 0646e4ee. Let me know if you still have issues.
I have managed to replicate the error, but I don't know what to do about it. I know it's highly suspect to blame the compiler, but I think in this case it is to blame. Here is the offending assembly code after a -O3 compilation:

in <graphite2::Silf::readGraphite(unsigned char const*, unsigned int, graphite2::Face&, unsigned int)

<+1052>:        mov    %eax,0x44(%esp)
<+1056>:	cmp    0x88(%esp),%eax
<+1063>:	mov    $0x12,%eax
<+1068>:	cmovge %eax,%ecx
<+1071>:	mov    $0x12,%edi
<+1076>:	mov    %ecx,0x68(%esp)

Notice the cmovge which is doing a conditional move of the error code if the cmp is greater than or equal, which is a signed comparison. An unsigned comparison would be cmovbe or something similar. This is the kind of test that is made in non -O3 code. And since the flags at that point are SF|IF, the move is not made and so the test passes when it should fail. Just to walk through that again. %eax contains passes_start and 0x88(%esp) contains silf_end. The cmp does passes_start - silf_end and records the flags. Since passes_start > silf_end, the flags come out with CF=0 (but as it happens with SF=1). The cmovge tests the SF against the OF and since they are the same does not do the move (if it were cmovbe then with CF=0 the move would happen).

Now compare this with gcc -O3:

In graphite2::Silf::readGraphite(unsigned char const*, unsigned long, graphite2::Face&, unsigned int)

<+1663>:	cmp    %rsi,0x8(%rsp)
<+1668>:	mov    %rsi,0x10(%rsp)
<+1673>:	setbe  %cl

Here %rsi contains passes_start and 0x8(%rsp) contains silf_end. So the cmp does silf_end - passes_start which should set the CF flag since passes_start > silf_end. And after the cmp we get CF|PF|SF|IF. The setbe set %cl to true to indicate the error if CF is set and so the error is triggered and the font bails.

So from what I can see, clang -O3 is generating the wrong code and is doing a signed instead of unsigned pointer comparison, which means a test that should fail (and does on other compilers) doesn't.

Any suggestions as to a work around that isn't completely ugly?
On deeper digging, it turns out that clang is doing comparison of offsets and not the actual pointers at that point. This is a perfectly acceptable optimisation. But it still should be doing the comparison unsigned.
I have a workaround. Where should I commit it?
If you could attach a patch to the bug that would be great. I could then test and fuzz the patch. When we are happy with the patch we can then find a good time to land it. Thanks Martin!
Attached patch fix.patchSplinter Review
Fixes the bug
Verified fixed in graphite-security commit bb24c61
Should be fixed by the patch landed in bug 1349310.
Depends on: CVE-2017-7778
(In reply to Tyson Smith [:tsmith] from comment #10)
> Verified fixed in graphite-security commit bb24c61

(In reply to Jonathan Kew (:jfkthame) from comment #11)
> Should be fixed by the patch landed in bug 1349310.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Assignee: nobody → jfkthame
Target Milestone: --- → mozilla55
Group: gfx-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54+][adv-esr52.2+]
Alias: CVE-2017-7774
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.