Closed Bug 692270 Opened 13 years ago Closed 13 years ago

Malformed Graphite2 font leads to crash [@graphite2::Pass::readPass]

Categories

(Core :: Graphics, defect)

x86_64
macOS
defect
Not set
critical

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)

Attached file 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;
Blocks: 681976
No longer depends on: 681976
Attached file testcase
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]
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.
(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().
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.
This is a regression in graphite2, sort of.
Depends on: 631479
Keywords: regression
This fix is now included in the latest graphite-import patch (in bug 631479).
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][sg:audit] → [sg:critical?][sg:audit][qa+]
Keywords: testcase
Whiteboard: [sg:critical?][sg:audit][qa+] → [sg:critical][qa+]
Verified on FF 11 Beta 6.
Verified in FF 10.0.2.
Verified in FF 12 Aurora and FF 13 Nightly.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Group: core-security
You need to log in before you can comment on or make changes to this bug.