graphite2: UBSan load of value which is not valid for type 'const graphite2::vm::opcode' [@graphite2::vm::Machine::Code::decoder::fetch_opcode]

RESOLVED FIXED

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: tsmith, Unassigned)

Tracking

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

unspecified
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)

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

Description

3 years ago
Created attachment 8725772 [details]
test_case.ttf

This was found while fuzzing graphite2 latest revision (bc5409c573aa9ecccacd18cf713021272998cd35)

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

This is likely not a sec issue however I am hiding this bug because of the large number of bugs that have been found and I would like to avoid any unwanted attention until things calm down.

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

/home/user/code/graphite/src/Code.cpp:277:26: runtime error: load of value 153, which is not a valid value for type 'const graphite2::vm::opcode'
    #0 0x7f9856ab4657 in graphite2::vm::Machine::Code::decoder::fetch_opcode(unsigned char const*) /home/user/code/graphite/src/Code.cpp:277:26
    #1 0x7f9856ab1cb3 in graphite2::vm::Machine::Code::decoder::load(unsigned char const*, unsigned char const*) /home/user/code/graphite/src/Code.cpp:256:28
    #2 0x7f9856aae608 in graphite2::vm::Machine::Code::Code(bool, unsigned char const*, unsigned char const*, unsigned char, unsigned short, graphite2::Silf const&, graphite2::Face const&, graphite2::passtype, unsigned char**) /home/user/code/graphite/src/Code.cpp:196:9
    #3 0x7f9856be1e3c in graphite2::Pass::readRules(unsigned char const*, unsigned long, unsigned char const*, unsigned short const*, unsigned short const*, unsigned char const*, unsigned short const*, unsigned char const*, graphite2::Face&, graphite2::passtype, graphite2::Error&) /home/user/code/graphite/src/Pass.cpp:260:45
    #4 0x7f9856bdaf7b in graphite2::Pass::readPass(unsigned char const*, unsigned long, unsigned long, graphite2::Face&, graphite2::passtype, unsigned int, graphite2::Error&) /home/user/code/graphite/src/Pass.cpp:202:14
    #5 0x7f9856c50b33 in graphite2::Silf::readGraphite(unsigned char const*, unsigned long, graphite2::Face&, unsigned int) /home/user/code/graphite/src/Silf.cpp:216:14
    #6 0x7f9856b52831 in graphite2::Face::readGraphite(graphite2::Face::Table const&) /home/user/code/graphite/src/Face.cpp:149:14
    #7 0x7f9856a68bdb in (anonymous namespace)::load_face(graphite2::Face&, unsigned int) /home/user/code/graphite/src/gr_face.cpp:59:42
    #8 0x7f9856a68339 in gr_make_face_with_ops /home/user/code/graphite/src/gr_face.cpp:89:16
    #9 0x7f9856a6bfc3 in gr_make_file_face /home/user/code/graphite/src/gr_face.cpp:242:23
    #10 0x4f5f9d in Parameters::testFileFont() const /home/user/code/graphite/gr2fonttest/gr2FontTest.cpp:633:20
    #11 0x4fd33b in main /home/user/code/graphite/gr2fonttest/gr2FontTest.cpp:787:9
    #12 0x7f98565c1ec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287
    #13 0x41b985 in _start (/home/user/Desktop/graphite/gr2fonttest+0x41b985)

Comment 1

3 years ago
fixed? in or before 3d80c6b69fd647fdb134987e2e87ea830e93e3a4. Probably the same as some other bug.

Comment 2

3 years ago
Sorry missed the UBSan aspect to this bug. This is a won't fix. The very next thing after assigning the value is we check it.
I think the point, though, is that the behavior of the assignment is unspecified, because the value of the byte is outside the enum's range. So by the time you check it, you're already into uncharted territory.

The fix is to use a `byte` variable to fetch and validate the opcode, and only assign it to a typed `opcode` once you know it is within range.
Whiteboard: [gfx-noted]
I just proposed https://github.com/silnrsi/graphite/pull/2, which I believe should fix this.
Flags: needinfo?(martin_hosken)

Comment 5

3 years ago
PR merged (with slight modification)
Flags: needinfo?(martin_hosken)
(Reporter)

Comment 6

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

Updated

3 years ago
Depends on: 1262846
(Reporter)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 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.