Closed Bug 1270288 Opened 8 years ago Closed 7 years ago

freetype2: use of uninitialised value in [@cf2_glyphpath_lineTo]

Categories

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

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: tsmith, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uninitialized, sec-low, testcase, Whiteboard: [post-critsmash-triage][gfx-noted][adv-main52+])

Attachments

(2 files, 1 obsolete file)

Attached file test_case.ttf
Found while fuzzing freetype2 commit d9fb2175c43b5a1cd4ad63781cb21cba9506da34 (>2.6.3)

I'm not sure if this affects the browser or if we are protected by OTS. I'm also not sure how far this bug goes back.

==39418==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7f456ffc6314 in cf2_glyphpath_lineTo /home/user/code/freetype2/src/cff/cf2hints.c:1657:38
    #1 0x7f456ffc05a1 in cf2_glyphpath_closeOpenPath /home/user/code/freetype2/src/cff/cf2hints.c:1827:7
    #2 0x7f456ffc05a1 in cf2_interpT2CharString /home/user/code/freetype2/src/cff/cf2intrp.c:1284
    #3 0x7f456ff93546 in cf2_getGlyphOutline /home/user/code/freetype2/src/cff/cf2font.c:472:7
    #4 0x7f456ff93546 in cf2_decoder_parse_charstrings /home/user/code/freetype2/src/cff/cf2ft.c:395
    #5 0x7f456ff855e4 in cff_slot_load /home/user/code/freetype2/src/cff/cffgload.c:2971:17
    #6 0x7f456ff855e4 in cff_glyph_load /home/user/code/freetype2/src/cff/cffdrivr.c:177
    #7 0x7f456fe8a9bb in FT_Load_Glyph /home/user/code/freetype2/src/base/ftobjs.c:742:15
    #8 0x7f456fe86cb8 in TestFace /home/user/code/freetype2/src/tools/ftrandom/../../../src/tools/ftrandom/ftrandom.c:105:12
    #9 0x7f456fe86cb8 in ExecuteTest /home/user/code/freetype2/src/tools/ftrandom/../../../src/tools/ftrandom/ftrandom.c:143
    #10 0x7f456fe86cb8 in main /home/user/code/freetype2/src/tools/ftrandom/../../../src/tools/ftrandom/ftrandom.c:166
    #11 0x7f456eadaec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287
    #12 0x7f456fe27aee in _start (/home/user/Desktop/ft/ftrandom_msan+0x25aee)
Attached file valgrind_log.txt (obsolete) —
I'm not sure if we have anyone who is sufficiently familiar with the freetype code (this looks like it's deep inside the CFF hint processor) to evaluate this properly. Jeff? Karl?

On the surface, it seems like use of an uninitialized value at that point is unlikely to be really dangerous; most likely it just results in fractionally inferior rendering because a hint may be mis-applied. But a much better understanding of the rasterizer would be needed to figure out if this is just the tip of a more serious issue.

I think this really needs to be reported upstream (https://savannah.nongnu.org/bugs/?group=freetype) for the FT core developers to assess. We just take their releases.
The uninitialized values seem to be x/y coordinates, which won't be too harmful (just bad drawing): calling it sec-low. This rating should not be generalized to other uninitialized variables which can lead to much worse outcomes.
Keywords: sec-low
I can't reproduce this with current git.  Please provide an exact `test_case.ttf' file that triggers the bug.
Flags: needinfo?(twsmith)
Attached file valgrind_log.txt
I was able to reproduce the issue under Valgrind with the current test case with commit 4e659d7...e3e33.

I built with CFLAGS="-g -O2" with gcc 5.4.0.

I ran the following command:
> $ LD_LIBRARY_PATH=. valgrind -q --track-origins=yes ./ftrandom test_case.ttf

Last I am using the same ftrandom.c file as bug 1272173.

Hopefully that helps with reproduction :)
Attachment #8748900 - Attachment is obsolete: true
Flags: needinfo?(twsmith)
I can repeat it too, now, thanks.  This is a strange problem, and I'm not sure whether this isn't a gcc optimization (or valgrind) bug – can you reproduce the valgrind warning with -O0 also?

The issue are the values `glyphpath->currentCS.{x,y}', which appear to be uninitialized at the beginning of function `cf2_glyphpath_lineTo'.  However, the `glyphpath' structure has been completely zeroed at the beginning of function `cf2_glyphpath_init', which means that `currentCS' *is* initialized from the very beginning.  I've verified (using `printf') that the `glyphpath' objects in the two functions have the same address.

How can this be further analyzed?
I confirmed that this can be detected with a gcc -O0 build and a clang 3.8 build using Valgrind. I can also repro this with a memory sanitizer build so I don't think a Valgrind false positive is to blame.

In terms of further analysis, I would try initializing the memory to a known value and verify that value is present before it is accessed. Perhaps setting a breakpoint on that memory location in gdb and debugging the issue that way will be more helpful since you won't need to modify the code (assuming there are compiler optimization issues). We can also CC others from the freetype dev team on the bug or I could open it and move it to the freetype bug tracker to get ore eyeballs on the issue (but I won't do that unless you give me the go ahead).
Yes, please move it to the FreeType bug tracker so that the original author of the CFF code (Dave Arnold from Adobe) can have a closer look.  I've now tried a few hours to find the cause without success...
I think the problem is that cf2_interpT2CharString fails to initialize its storage[] array, and it's possible for code in a bad charstring to 'get' a value from there that has not previously been 'put'. Such a value can then find its way into the glyphpath's currentCS field, where it eventually upsets valgrind when it is used in a comparison.

Explicitly zeroing storage[], e.g. by adding

    FT_ZERO( &storage );

along with the other initializations at the beginning of cf2_interpT2CharString should prevent the error.
Werner, could you check/confirm the above, and take this upstream as appropriate? Thanks.
Flags: needinfo?(wl)
That's it, thanks!  Fixed now in git, please test.
Flags: needinfo?(wl)
Verified with freetype revision ebbb8b...fb9defa. Thanks!
Priority: -- → P3
Whiteboard: [gfx-noted]
Fixed in FreeType 2.7.0 by http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=a15133e6efc10f5342dedf5dfca9070c8bcc49ca
Status: NEW → RESOLVED
Closed: 7 years ago
Depends on: 1176531
Resolution: --- → FIXED
Group: gfx-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [gfx-noted] → [post-critsmash-triage][gfx-noted]
Whiteboard: [post-critsmash-triage][gfx-noted] → [post-critsmash-triage][gfx-noted][adv-main52+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.