Closed
Bug 805760
Opened 12 years ago
Closed 12 years ago
Graphite2 crash [@graphite2::sparse::sparse<::glat_iterator>::glat_iterator]
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
Tracking | Status | |
---|---|---|
firefox17 | --- | disabled |
firefox18 | --- | disabled |
firefox19 | + | fixed |
firefox-esr10 | --- | unaffected |
firefox-esr17 | --- | unaffected |
People
(Reporter: posidron, Assigned: jfkthame)
References
(Blocks 1 open bug)
Details
(Keywords: crash, sec-critical, testcase, Whiteboard: [adv-main19-])
Attachments
(4 files, 1 obsolete file)
9.78 KB,
text/plain
|
Details | |
22.94 KB,
application/zip
|
Details | |
27.20 KB,
patch
|
jtd
:
review+
|
Details | Diff | Splinter Review |
21.58 KB,
patch
|
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
READ 1 overflow gfx/graphite2/src/GlyphCache.cpp:52 [...] value_type operator * () const { if (_n==0) { _v.first = *_p++; _n = *_p++; } _v.second = be::peek<uint16>(_p); return _v; } [...] gfx/graphite2/src/inc/Sparse.h:104 [...] for (I i = attr; i != last; ++i, ++n_values) { const key_type k = (*i).first / SIZEOF_CHUNK; if (k >= m_nchunks) m_nchunks = k+1; } [...] gfx/graphite2/src/GlyphCache.cpp:326 [...] new (&glyph) GlyphFace(bbox, advance, glat_iterator(m_pGlat + glocs), glat_iterator(m_pGlat + gloce)); [...] Tested against m-central with changeset: 111360:58c8080a1a7c
Reporter | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
This patch applies on top of the patch in bug 803498, and pulls in the latest commits from upstream; see changelog at http://hg.palaso.org/graphitedev. For reference, I've also added a README that records the current upstream changeset, and a simple script used to semi-automate the import process.
Attachment #676550 -
Flags: review?(jdaggett)
Comment 3•12 years ago
|
||
Looks fine but we should have a crashtest for this.
Reporter | ||
Comment 4•12 years ago
|
||
The crash-test of the test-case.
Assignee | ||
Updated•12 years ago
|
Attachment #677348 -
Attachment is patch: true
Attachment #677348 -
Flags: review?(jdaggett)
Updated•12 years ago
|
Assignee: nobody → jfkthame
Updated•12 years ago
|
Attachment #676550 -
Flags: review?(jdaggett) → review+
Updated•12 years ago
|
Attachment #677348 -
Flags: review?(jdaggett) → review+
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #2) > This patch applies on top of the patch in bug 803498, John, do you intend to review 803498 as well, or should I just fold all the gr2 library updates into this bug and leapfrog the earlier update?
Assignee | ||
Comment 6•12 years ago
|
||
This folds in the intermediate patch (from bug 803498) so as to update our tree directly to the latest upstream commit. (Carrying over r=jdaggett.)
Assignee | ||
Comment 7•12 years ago
|
||
Comment on attachment 678671 [details] [diff] [review] update graphite2 lib to commit 51e72e74b9a6. [Security approval request comment] How easily can the security issue be deduced from the patch? The patch is a new upstream library version including a number of changes, so it does not directly point to the specific issue here. But an examination of the upstream changelog would indicate the potential "weak spots" that have been hardened. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The crashtest font file (not included in this patch, could be landed separately later) provides an example of how to reach the flawed code. Which older supported branches are affected by this flaw? All except esr10, probably. Older branches have older versions of the graphite2 library, which are presumably affected by this, but also by other known issues. However, as the feature is preffed-off by default, this is not a serious exposure. If not all supported branches, which bug introduced the flaw? 631479 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Backporting would mean taking a complete graphite2 update on the affected branches. Not very difficult or risky, but a lot of code churn for something that's disabled by default. How likely is this patch to cause regressions; how much testing does it need? Minimal risk of regressions; the fixes are basically more careful bounds-checking to safely read corrupted/malicious font tables, not significant functional changes.
Attachment #678671 -
Flags: sec-approval?
Comment 8•12 years ago
|
||
Comment on attachment 678671 [details] [diff] [review] update graphite2 lib to commit 51e72e74b9a6. sec-approval+. Go for it. Better now than later.
Attachment #678671 -
Flags: sec-approval? → sec-approval+
Updated•12 years ago
|
status-firefox-esr10:
--- → unaffected
status-firefox17:
--- → disabled
status-firefox18:
--- → disabled
status-firefox19:
--- → affected
tracking-firefox19:
--- → +
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1688a1c6fce
Assignee | ||
Updated•12 years ago
|
Attachment #676550 -
Attachment is obsolete: true
Comment 10•12 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #5) > (In reply to Jonathan Kew (:jfkthame) from comment #2) > > This patch applies on top of the patch in bug 803498, > > John, do you intend to review 803498 as well, or should I just fold all the > gr2 library updates into this bug and leapfrog the earlier update? Just to confirm, do we need bug 803498 open any longer? The changes there supersede these, right?
Comment 11•12 years ago
|
||
Argh, the changes *here* supersede those is what I meant to say...
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a1688a1c6fce
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to John Daggett (:jtd) from comment #11) > Argh, the changes *here* supersede those is what I meant to say... Yes (see bug 803498 comment 8).
Updated•12 years ago
|
status-firefox-esr17:
--- → unaffected
Updated•11 years ago
|
Whiteboard: [adv-main19-]
Reporter | ||
Updated•11 years ago
|
Blocks: fuzzing-fonts
Updated•10 years ago
|
Group: core-security
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/43a7c0e8d91f
Updated•8 years ago
|
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•