1355174 - (CVE-2017-7774) Graphite2: out of bounds read [@ graphite2::Silf::readGraphite]

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Tyson Smith [:tsmith]]

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

Comment 1

[martin_hosken]

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.

Comment 2

[martin_hosken]

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?

Comment 4

[Tyson Smith [:tsmith]]

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.

Comment 5

[martin_hosken]

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?

Comment 6

[martin_hosken]

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.

Comment 7

[martin_hosken]

I have a workaround. Where should I commit it?

Comment 8

[Tyson Smith [:tsmith]]

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!

Comment 9

[martin_hosken]

Attachment: fix.patch

Fixes the bug

Comment 10

[Tyson Smith [:tsmith]]

Verified fixed in graphite-security commit bb24c61

Comment 11

[Jonathan Kew [:jfkthame]]

Should be fixed by the patch landed in bug 1349310.

Comment 12

[Frederik Braun [:freddy]]

(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.