Closed Bug 1355182 (CVE-2017-7775) Opened 3 years ago Closed 3 years ago

Graphite2: Assertion 'size() > n' failed [@ graphite2::FeatureRef::applyValToFeature]

Categories

(Core :: Graphics: Text, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox-esr52 54+ fixed
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: tsmith, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main54+][adv-esr52.2+])

Attachments

(1 file)

115.00 KB, application/x-font-ttf
Details
Attached file test_case.ttf
Found with a 32bit build running gr2FontTest.

STR:
Run in gdb with a 32-bit debug build.
gdb --args ./g2fonttest test_case.ttf -auto -demand -noprint

gr2fonttest: src/inc/List.h:81: reference graphite2::Vector<unsigned int>::operator[](size_t) [T = unsigned int]: Assertion `size() > n' failed.

Program received signal SIGABRT, Aborted.
0xf7fd7c89 in __kernel_vsyscall ()
(gdb) bt
#0  0xf7fd7c89 in __kernel_vsyscall ()
#1  0xf7cafea9 in __GI_raise (sig=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#2  0xf7cb1407 in __GI_abort () at abort.c:89
#3  0xf7ca8d07 in __assert_fail_base (
    fmt=0xf7de3088 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
    assertion=0x808c3a8 "size() > n", 
    file=0x808b89b "src/inc/List.h", line=81, 
    function=0x808c3b3 "reference graphite2::Vector<unsigned int>::operator[](size_t) [T = unsigned int]") at assert.c:92
#4  0xf7ca8d8b in __GI___assert_fail (assertion=0x808c3a8 "size() > n", 
    file=0x808b89b "src/inc/List.h", line=81, 
    function=0x808c3b3 "reference graphite2::Vector<unsigned int>::operator[](size_t) [T = unsigned int]") at assert.c:101
#5  0x080572af in graphite2::Vector<unsigned int>::operator[] (n=<optimized out>, 
    this=<optimized out>) at src/inc/List.h:81
#6  graphite2::FeatureRef::applyValToFeature (this=<optimized out>, val=3888, pDest=...)
    at src/FeatureMap.cpp:279
#7  0x08056eea in graphite2::FeatureMap::readFeats (this=<optimized out>, face=...)
    at src/FeatureMap.cpp:178
#8  0x08057371 in graphite2::SillMap::readFace (this=0x80abd04, face=...)
    at src/FeatureMap.cpp:191
#9  0x08055330 in graphite2::Face::readFeatures (this=0x80abd00)
    at src/Face.cpp:161
#10 0x0804f251 in (anonymous namespace)::load_face (face=..., options=<optimized out>)
    at src/gr_face.cpp:59
#11 gr_make_face_with_ops (appFaceHandle=<optimized out>, ops=<optimized out>, 
    faceOptions=<optimized out>) at src/gr_face.cpp:89
#12 0x080502c2 in gr_make_file_face (filename=<optimized out>, faceOptions=0)
    at src/gr_face.cpp:242
#13 0x0804c6eb in Parameters::testFileFont (this=0xffffcf28)
    at gr2fonttest/gr2FontTest.cpp:639
#14 0x0804d9ed in main (argc=<optimized out>, argv=<optimized out>)
    at gr2fonttest/gr2FontTest.cpp:798
Fixed in b23d7b9. But there was no security issue here. The assert is debug only and if the assert was ignored, the read and write were still security safe (even if the results were uninitialised junk).
(In reply to martin_hosken from comment #1)
> Fixed in b23d7b9. But there was no security issue here. The assert is debug
> only and if the assert was ignored, the read and write were still security
> safe (even if the results were uninitialised junk).

Milan please help us find out if the mentioned fix-commit is included in our build and triage the bug appropriately?
Flags: needinfo?(milan)
(In reply to Frederik Braun [:freddyb] from comment #2)
> (In reply to martin_hosken from comment #1)
> > Fixed in b23d7b9. But there was no security issue here. The assert is debug
> > only and if the assert was ignored, the read and write were still security
> > safe (even if the results were uninitialised junk).
> 
> Milan please help us find out if the mentioned fix-commit is included in our
> build and triage the bug appropriately?

It's not; this only landed upstream last week, and we haven't pulled a new version since then.

However, Martin notes (comment 1) that this particular bug doesn't actually represent a security issue for gecko, and AFAICS that appears to be correct.

I haven't dug far enough to see whether the resulting uninitialized feature value might lead to subsequent problems if the value ends up being wildly outside what later code expects, but at least at the point where this assertion fires, it looks harmless (though it's still a bug, and the mentioned commit fixes it so that the value will be set properly).
Flags: needinfo?(milan)
The pDest.reserve() that comes before the indexing (which the fix changes to a .resize()) ensures that there is memory allocated to cover whatever value of m_index may be being used. This means that m_index cannot be set such that it accesses memory outside of the list (which is a reimplementation of std:vector). It doesn't matter what uninitialised junk may be in the uninitialised block created by .reserve() because we are oring in potentially random junk from the font too. Those values are only ever used for testing to control which GDL rules will fire and are bounds limited. So they are, in effect, untrusted values all the way since a naughty font could set the values to arbitrary nastiness, which can be no worse than random junk.

Having said all that, I recommend my making the patches available on graphite (as opposed to graphite-security) and releasing a new version of graphite with them in asap and your taking that.
Should be fixed by the patch landed in bug 1349310.
Depends on: CVE-2017-7778
Tyson, can you retest?
Flags: needinfo?(twsmith)
Verified fixed in graphite commit 090076bf4
Flags: needinfo?(twsmith)
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Group: gfx-core-security → core-security-release
Assignee: nobody → jfkthame
Target Milestone: --- → mozilla55
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54+][adv-esr52.2+]
Alias: CVE-2017-7775
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.