If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
Graphics: Text
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: tsmith, Unassigned)

Tracking

(Blocks: 1 bug, {csectype-intoverflow, sec-audit, testcase})

unspecified
csectype-intoverflow, sec-audit, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 disabled, firefox46 fixed, firefox47 fixed, firefox48 fixed, firefox-esr3846+ disabled, firefox-esr4546+ disabled)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

1.64 KB, application/x-font-ttf
Details
(Reporter)

Description

2 years ago
Created attachment 8727017 [details]
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)

Comment 1

2 years ago
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)

Comment 3

2 years ago
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)
(Reporter)

Comment 4

2 years ago
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.

Comment 7

2 years ago
Fixed? in 56671221b974024dd96cc9c6f592678ee6d24841
(Reporter)

Comment 8

2 years ago
Verified with graphite revision 56671221b974024dd96cc9c6f592678ee6d24841
(Reporter)

Updated

2 years ago
Keywords: sec-audit
(Reporter)

Updated

2 years ago
Depends on: 1262846
(Reporter)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 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
status-firefox45: --- → wontfix
status-firefox46: --- → fixed
status-firefox47: --- → fixed
status-firefox48: --- → fixed
status-firefox-esr38: --- → fixed
status-firefox-esr45: --- → fixed
tracking-firefox-esr38: --- → 46+
tracking-firefox-esr45: --- → 46+
status-firefox45: wontfix → disabled
status-firefox-esr38: fixed → disabled
status-firefox-esr45: fixed → disabled
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.