Closed
Bug 692270
Opened 13 years ago
Closed 13 years ago
Malformed Graphite2 font leads to crash [@graphite2::Pass::readPass]
Categories
(Core :: Graphics, defect)
Tracking
()
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
firefox7 | - | unaffected |
firefox8 | - | unaffected |
firefox9 | - | unaffected |
firefox10 | + | verified |
firefox11 | --- | verified |
firefox12 | --- | verified |
firefox-esr10 | --- | unaffected |
status1.9.2 | --- | unaffected |
People
(Reporter: posidron, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: crash, regression, testcase, Whiteboard: [sg:critical][qa!])
Attachments
(3 files)
16.34 KB,
text/plain
|
Details | |
33.73 KB,
application/octet-stream
|
Details | |
1.35 KB,
patch
|
Details | Diff | Splinter Review |
If testcase does not work immediately, let it run for a few seconds. I further have marked this as a security issue because I believe there are certain read conditions in this code which _perhaps_ should be better sanitized in point of integer overflows. Example: /gfx/graphite2/src/Pass.cpp:111 numRanges = be::read<uint16>(p); ... if (p + numRanges * 6 - 4 > pass_end) return false; m_numGlyphs = be::swap<uint16>(*(uint16 *)(p + numRanges * 6 - 4)) + 1; // Caculate the start of vairous arrays. const uint16 * const ranges = reinterpret_cast<const uint16 *>(p); p += numRanges*sizeof(uint16)*3;
Reporter | ||
Updated•13 years ago
|
Reporter | ||
Comment 1•13 years ago
|
||
Comment 2•13 years ago
|
||
This seems to be two bugs for the price of one: a specific crash, and a need to look for similar bad code patterns. We might want to split them in two. Is there a target release for including the Graphite library? Fx10, or later? Is graphite included in nightly builds, or is it a special build option at the moment?
Whiteboard: [sg:critical?][sg:audit]
Updated•13 years ago
|
status-firefox7:
--- → unaffected
status-firefox8:
--- → unaffected
status-firefox9:
--- → unaffected
Reporter | ||
Comment 3•13 years ago
|
||
Graphite2(In reply to Daniel Veditz from comment #2) > Is graphite included in nightly builds, or is it a special build option at > the moment? Graphite2 can be implemented by applying the patches at https://bugzilla.mozilla.org/show_bug.cgi?id=631479 I used the graphics branch for that purpose.
Comment 4•13 years ago
|
||
(In reply to Christoph Diehl [:cdiehl] from comment #0) > Created attachment 565009 [details] > callstack > > If testcase does not work immediately, let it run for a few seconds. > > I further have marked this as a security issue because I believe there are > certain read conditions in this code which _perhaps_ should be better > sanitized in point of integer overflows. > > Example: /gfx/graphite2/src/Pass.cpp:111 > > numRanges = be::read<uint16>(p); > ... > if (p + numRanges * 6 - 4 > pass_end) return false; > m_numGlyphs = be::swap<uint16>(*(uint16 *)(p + numRanges * 6 - 4)) + 1; > // Caculate the start of vairous arrays. > const uint16 * const ranges = reinterpret_cast<const uint16 *>(p); > p += numRanges*sizeof(uint16)*3; It's not immediately obvious that there's an integer overflow problem right here - the value of numRanges comes from a 16-bit field in the table, so it cannot exceed 65535, but it's a size_t variable, and so all the arithmetic should be happening with plenty of bits to spare. I think we need to dig a little further to isolate exactly what's going wrong here. Looking at the callstack, I think the real problem is that the pass_length argument to readPass() is huge, and I don't think the font file is really that big - so then readPass() tries to read from beyond the end of the file. This needs to be validated in Silf::readGraphite() before calling readPass().
Comment 5•13 years ago
|
||
I believe this fixes the problem, by validating the next-pass offset so that we don't try to call readPass() with an insane length. Martin, please check this for correctness and take upstream if it's OK.
Comment 6•13 years ago
|
||
This is a regression in graphite2, sort of.
tracking-firefox10:
--- → +
tracking-firefox7:
--- → -
tracking-firefox8:
--- → -
tracking-firefox9:
--- → -
Depends on: 631479
Keywords: regression
Comment 7•13 years ago
|
||
This fix is now included in the latest graphite-import patch (in bug 631479).
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
status-firefox10:
--- → fixed
Updated•12 years ago
|
status1.9.2:
--- → unaffected
status-firefox11:
--- → fixed
status-firefox12:
--- → fixed
Keywords: testcase
Whiteboard: [sg:critical?][sg:audit][qa+] → [sg:critical][qa+]
Comment 8•12 years ago
|
||
Verified on FF 11 Beta 6.
Comment 9•12 years ago
|
||
Verified in FF 10.0.2.
Comment 10•12 years ago
|
||
Verified in FF 12 Aurora and FF 13 Nightly.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Updated•12 years ago
|
Group: core-security
status-firefox-esr10:
--- → unaffected
Reporter | ||
Updated•12 years ago
|
Blocks: fuzzing-fonts
You need to log in
before you can comment on or make changes to this bug.
Description
•