Closed Bug 1263191 Opened 8 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: tsmith, Unassigned)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

1.62 KB, application/x-font-ttf
Details
Attached file test_case.ttf
This was found while fuzzing graphite2 revision 9c93113992a644e71fa52356d2d73be47b888c97 (>1.3.8)

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

To reproduce:
run: ./gr2fonttest test_case.ttf -auto -demand

/home/user/code/graphite/src/inc/opcodes.h:578:38: runtime error: signed integer overflow: 0 - -2147483648 cannot be represented in type 'int'
    #0 0x7ffac10aef33 in (anonymous namespace)::direct_run(bool, void* const*, unsigned char const*, int*, graphite2::Slot**&, unsigned char, graphite2::vm::Machine::status_t&, graphite2::SlotMap*) /home/user/code/graphite/src/inc/opcodes.h:578:38
    #1 0x7ffac10b278c in graphite2::vm::Machine::run(void* const*, unsigned char const*, graphite2::Slot**&) /home/user/code/graphite/src/direct_machine.cpp:116:17
    #2 0x7ffac10e23ef in graphite2::vm::Machine::Code::run(graphite2::vm::Machine&, graphite2::Slot**&) const /home/user/code/graphite/src/Code.cpp:746:13
    #3 0x7ffac1169d7f in graphite2::Pass::doAction(graphite2::vm::Machine::Code const*, graphite2::Slot*&, graphite2::vm::Machine&) const /home/user/code/graphite/src/Pass.cpp:682:17
    #4 0x7ffac115eec2 in graphite2::Pass::findNDoRule(graphite2::Slot*&, graphite2::vm::Machine&, graphite2::FiniteStateMachine&) const /home/user/code/graphite/src/Pass.cpp:548:33
    #5 0x7ffac115c6de in graphite2::Pass::runGraphite(graphite2::vm::Machine&, graphite2::FiniteStateMachine&, bool) const /home/user/code/graphite/src/Pass.cpp:417:13
    #6 0x7ffac1195c2e in graphite2::Silf::runGraphite(graphite2::Segment*, unsigned char, unsigned char, int) const /home/user/code/graphite/src/Silf.cpp:423:21
    #7 0x7ffac111cd58 in graphite2::Face::runGraphite(graphite2::Segment*, graphite2::Silf const*) const /home/user/code/graphite/src/Face.cpp:187:20
    #8 0x7ffac10c6837 in graphite2::Segment::runGraphite() /home/user/code/graphite/src/inc/Segment.h:97:45
    #9 0x7ffac10c3873 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 0x7ffac10c3873 in gr_make_seg /home/user/code/graphite/src/gr_segment.cpp:105
    #11 0x4ea48d in Parameters::testFileFont() const /home/user/code/graphite/gr2fonttest/gr2FontTest.cpp:690:20
    #12 0x4ec0e9 in main /home/user/code/graphite/gr2fonttest/gr2FontTest.cpp:797:9
    #13 0x7ffac0cadec4 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)
    #14 0x41acc5 in _start (/home/ubuntu/build/build/gr2fonttest+0x41acc5)
This seems to be a limitation of int given that it can store a larger negative number than it can a positive one. Given the junk action code being executed, there is no evidence that this problem is either caused by something problematic or will cause problems down the line. Crazy values on the action code stack, including wrapping, are not inherently a problem so long as they are not caused by something bad or cause bad things to happen. Neither of these cases are exposed by this bug report.

No action required.
Whiteboard: [gfx-noted]
Does this still need to be open?
Flags: needinfo?(jfkthame)
Hmm. It's an edge case that probably doesn't represent a real threat at present, but given that signed integer overflow is undefined behavior, there's a theoretical risk something bad could happen; we really should try to get the code hardened so that this can't be exposed.

Let's keep this open for now, with the intention to least re-test and examine the code again. Leaving ni? flag to remind myself of it.
Has Regression Range: --- → irrelevant
I've fixed this bug in commit 5ca0dfc in the mozilla branch of the graphite-security repo and checked it against the test case provided.
Thanks Tim, verified. Do you plan to include this fix the next graphite release?
Group: gfx-core-security
This is fixed upstream now, right?
Flags: needinfo?(jfkthame) → needinfo?(tim_eves)
Yes, this was upstreamed as commit b259c9c69145f2b0fcd682633b420a4855e85112 on Aug 10 and is in release graphite2 1.3.12.
Flags: needinfo?(tim_eves)
Thanks for the confirmation.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.