Graphite2 crash [@graphite2::sparse::sparse<::glat_iterator>::glat_iterator]

RESOLVED FIXED in Firefox 19

Status

()

Core
Graphics
--
critical
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: posidron, Assigned: jfkthame)

Tracking

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

Trunk
mozilla19
x86_64
Mac OS X
crash, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17 disabled, firefox18 disabled, firefox19+ fixed, firefox-esr10 unaffected, firefox-esr17 unaffected)

Details

(Whiteboard: [adv-main19-])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 675498 [details]
callstack

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

6 years ago
Created attachment 675499 [details]
testcase
(Assignee)

Comment 2

6 years ago
Created attachment 676550 [details] [diff] [review]
update graphite2 lib to commit 51e72e74b9a6.

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

6 years ago
Looks fine but we should have a crashtest for this.
(Reporter)

Comment 4

6 years ago
Created attachment 677348 [details] [diff] [review]
testcase as crashtest

The crash-test of the test-case.
(Assignee)

Updated

6 years ago
Attachment #677348 - Attachment is patch: true
Attachment #677348 - Flags: review?(jdaggett)
Assignee: nobody → jfkthame

Updated

6 years ago
Attachment #676550 - Flags: review?(jdaggett) → review+

Updated

6 years ago
Attachment #677348 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 5

6 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

6 years ago
Created attachment 678671 [details] [diff] [review]
update graphite2 lib to commit 51e72e74b9a6.

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

6 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 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+
status-firefox-esr10: --- → unaffected
status-firefox17: --- → disabled
status-firefox18: --- → disabled
status-firefox19: --- → affected
tracking-firefox19: --- → +
(Assignee)

Updated

6 years ago
Attachment #676550 - Attachment is obsolete: true

Comment 10

6 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

6 years ago
Argh, the changes *here* supersede those is what I meant to say...
https://hg.mozilla.org/mozilla-central/rev/a1688a1c6fce
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox19: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Assignee)

Comment 13

6 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).
Blocks: 803498
status-firefox-esr17: --- → unaffected
Whiteboard: [adv-main19-]
(Reporter)

Updated

5 years ago
Blocks: 750695
Group: core-security

Updated

3 years ago
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.