Closed Bug 1295376 Opened 8 years ago Closed 7 years ago

freetype2: signed integer overflow in [@Ins_ADD]

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: tsmith, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: sec-low, testcase, Whiteboard: [gfx-noted][adv-main57-])

Attachments

(1 file)

2.59 KB, application/x-font-ttf
Details
Attached file test_case.ttf
Found while fuzzing freetype2 commit 248f5629d8889aa5b77ea5bfce0935140293d50d (>2.6.5)

I don't think this has any security implication but marking sec-sensitive for now. Please add comments if you feel this bug should be unhidden.

src/truetype/ttinterp.c:2818:13: runtime error: signed integer overflow: 6944550625405304832 + 6944550625405304832 cannot be represented in type 'long'
    #0 0x7f2effd83b46 in Ins_ADD src/truetype/ttinterp.c:2818:13
    #1 0x7f2effd83b46 in TT_RunIns src/truetype/ttinterp.c:8042
    #2 0x7f2effdbae32 in tt_size_run_prep src/truetype/ttobjs.c:894:15
    #3 0x7f2effda6e97 in tt_size_ready_bytecode src/truetype/ttobjs.c:1117:15
    #4 0x7f2effda6e97 in tt_loader_init src/truetype/ttgload.c:2288
    #5 0x7f2effd54aa6 in TT_Load_Glyph src/truetype/ttgload.c:2645:13
    #6 0x7f2effd54aa6 in tt_glyph_load src/truetype/ttdriver.c:424
    #7 0x7f2effca633e in FT_Load_Glyph src/base/ftobjs.c:742:15
    #8 0x4ea5aa in TestFace src/tools/ftrandom/../../../src/tools/ftrandom/ftrandom.c:105:12
    #9 0x4ea5aa in ExecuteTest src/tools/ftrandom/../../../src/tools/ftrandom/ftrandom.c:143
    #10 0x4ea5aa in main src/tools/ftrandom/../../../src/tools/ftrandom/ftrandom.c:166
    #11 0x7f2efed6d82f in __libc_start_main /build/glibc-GKVZIf/glibc-2.23/csu/../csu/libc-start.c:291
    #12 0x418a78 in _start (/home/user/workspace/freetype2/ftrandom+0x418a78)
This is completely harmless.  Arithmetic in TrueType instructions is, AFAIK, modulo 2^32.

We have similar reports from the (new) CFF engine.  With newer compilers it is possible to suppress this run-time error without adding C code, however, I haven't yet found to implement this.
(In reply to Werner Lemberg from comment #1)
> This is completely harmless.  Arithmetic in TrueType instructions is, AFAIK,
> modulo 2^32.

If so, the arithmetic should probably be done with unsigned variables in C, and then the final result cast to signed where necessary. The problem is that *signed* integer overflow is undefined behavior according to the spec; and being undefined, it is not even guaranteed to be harmless, let alone predictable.
OK, thanks for the information.  Do you know any system where problems exist?
No, as far as I'm aware it is currently a theoretical rather than practical problem. (I still think it'd be good to fix, though. Undefined behavior is never a good thing...)
Let me preface this with I am just the messenger :)

Undefined behavior (UB) in general can lead to unexpected issues that may seem unrelated to the code that contains the UB (not just incorrect rendering issues). The issue isn't that (for example) overflowing a signed int won't perform a certain way (it is not guaranteed to) it's that depending on the compiler, optimization level and output architecture the compiler can do completely different (and unexpected) things. For more info see[1] and see[2]. I am using UBSan[3] while fuzzing to help find these issues.

FreeType2 is a popular core library that many projects depend on so I think it is important to make sure that the code is of the highest quality. I know UB bugs aren't always the most exciting to fix, but fixing can also help identify other issues. This also helps fuzzing efforts by removing issues that are getting in the way.

Werner, thank you for your quick response to the issues I have logged so far I do have more in the pipe and I am happy to continue log them as they come up. From the perspective of Firefox I don't think many of the bugs will be high priority so please don't feel rushed.

[1] http://blog.regehr.org/archives/213 (there are 3 parts)
[2] http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html
[3] http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
Tyson, thanks for those reports!  However, it would be very helpful if you could file them in the standard FreeType bug tracker (including the ones which are still open, e.g., this one, and which aren't already covered by existing bug reports – a simple link would be fully sufficient).  Note that Savannah also supports a `private' flag for reports in case the problem might have security issues.
sec-low because we think the behaviors are relatively defined (for now) with the OS's and compilers we're using, but that might not always be the case (especially with heavily optimized code).
Keywords: sec-low
Group: gfx-core-security
Whiteboard: [gfx-noted]
Per the upstream issue, this fix shipped in FreeType 2.8.1, which we landed in Fx57.
Status: NEW → RESOLVED
Closed: 7 years ago
Depends on: 1400602
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Whiteboard: [gfx-noted] → [gfx-noted][adv-main57-]
You need to log in before you can comment on or make changes to this bug.