Closed Bug 1253790 Opened 8 years ago Closed 8 years ago

graphite2: UBSan signed integer overflow in [@graphite2::vm::Machine::run]

Categories

(Core :: Graphics: Text, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox45 --- disabled
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed
firefox-esr38 46+ disabled
firefox-esr45 46+ disabled

People

(Reporter: tsmith, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-intoverflow, sec-audit, testcase, Whiteboard: [gfx-noted])

Attachments

(1 file)

1.64 KB, application/x-font-ttf
Details
Attached file test_case.ttf
This was found while fuzzing graphite2 latest revision (520d76818052772d614e581dacea69499b912be6)

This issue was uncovered using Undefined Behavior Sanitizer (UBSan). More information can be found here: http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html.

To reproduce:
Build with UBSan enabled.
run: ./gr2fonttest test_case.ttf -auto -demand

/home/user/code/graphite/src/inc/opcodes.h:128:5: runtime error: signed integer overflow: -2089 * 100672259 cannot be represented in type 'int'
    #0 0x7f5f5ca8af93 in (anonymous namespace)::mul(unsigned char const*&, int*&, int*, regbank&) /home/user/code/graphite/src/inc/opcodes.h:128:5
    #1 0x7f5f5ca87bca in graphite2::vm::Machine::run(void* const*, unsigned char const*, graphite2::Slot**&) /home/user/code/graphite/src/call_machine.cpp:121:12
    #2 0x7f5f5cb25423 in graphite2::vm::Machine::Code::run(graphite2::vm::Machine&, graphite2::Slot**&) const /home/user/code/graphite/src/Code.cpp:737:13
    #3 0x7f5f5cc5930a in graphite2::Pass::doAction(graphite2::vm::Machine::Code const*, graphite2::Slot*&, graphite2::vm::Machine&) const /home/user/code/graphite/src/Pass.cpp:675:17
    #4 0x7f5f5cc4b00f in graphite2::Pass::findNDoRule(graphite2::Slot*&, graphite2::vm::Machine&, graphite2::FiniteStateMachine&) const /home/user/code/graphite/src/Pass.cpp:544:33
    #5 0x7f5f5cc453e2 in graphite2::Pass::runGraphite(graphite2::vm::Machine&, graphite2::FiniteStateMachine&, bool) const /home/user/code/graphite/src/Pass.cpp:413:13
    #6 0x7f5f5ccb7026 in graphite2::Silf::runGraphite(graphite2::Segment*, unsigned char, unsigned char, int) const /home/user/code/graphite/src/Silf.cpp:423:21
    #7 0x7f5f5cbac33c in graphite2::Face::runGraphite(graphite2::Segment*, graphite2::Silf const*) const /home/user/code/graphite/src/Face.cpp:187:20
    #8 0x7f5f5cae29ac in graphite2::Segment::runGraphite() /home/user/code/graphite/src/inc/Segment.h:97:45
    #9 0x7f5f5cadb9a3 in (anonymous namespace)::makeAndInitialize(graphite2::Font const*, graphite2::Face const*, unsigned int, graphite2::FeatureVal const*, gr_encform, void const*, unsigned long, int) /home/user/code/graphite/src/gr_segment.cpp:46:67
    #10 0x7f5f5cadb575 in gr_make_seg /home/user/code/graphite/src/gr_segment.cpp:105:24
    #11 0x4f91fc in Parameters::testFileFont() const /home/user/code/graphite/gr2fonttest/gr2FontTest.cpp:684:20
    #12 0x4fcc89 in main /home/user/code/graphite/gr2fonttest/gr2FontTest.cpp:787:9
    #13 0x7f5f5c61cec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287
    #14 0x41b985 in _start (/home/user/Desktop/graphite/gr2fonttest+0x41b985)
Will not fix. This is defined behaviour. Overflowing in these opcodes is allowed.
Whiteboard: [gfx-noted]
(In reply to martin_hosken from comment #1)
> Will not fix. This is defined behaviour. Overflowing in these opcodes is
> allowed.

Integer overflow is explicitly defined to be undefined behavior. Please reconsider.
Flags: needinfo?(martin_hosken)
We don't want to slow the engine down by inserting a check and are quite happy if the multiplication overflows/wraps the 32-bit value. If you have a suggestion on how to code that in C++ satisfactorily, I'm open to it.
Flags: needinfo?(martin_hosken)
You can use an unsigned integer instead. Overflowing is defined behavior of unsigned integers.
(In reply to Tyson Smith [:tsmith] from comment #4)
> You can use an unsigned integer instead. Overflowing is defined behavior of
> unsigned integers.

That should be OK for the +, - and * binops: if you cast to uint32_t, perform the operation, and cast the result back to int32_t, I think (please double-check me!) you'll end up with the same thing as the "obvious" result you'd expect from simple wrapping, without entering undefined territory.

That won't work for division, though, so you can't simply package that into the binop() macro and be done; you'll need to keep division working with signed values, but fortunately that won't run into overflow so it's OK.
Alternatively, if you cast one of the operands to int64_t, the operation can't overflow; then truncate the result back to 32 bits to store it.
Fixed? in 56671221b974024dd96cc9c6f592678ee6d24841
Verified with graphite revision 56671221b974024dd96cc9c6f592678ee6d24841
Keywords: sec-audit
Depends on: 1262846
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Group: gfx-core-security → core-security-release
Graphite2 has been updated to 1.3.8 on all the relevant branches including ESRs
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.