Closed Bug 805760 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Graphics, defect)

x86_64
macOS
defect
Not set
critical

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)

Attached file 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
Attached file testcase
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)
Looks fine but we should have a crashtest for this.
The crash-test of the test-case.
Attachment #677348 - Attachment is patch: true
Attachment #677348 - Flags: review?(jdaggett)
Assignee: nobody → jfkthame
Attachment #676550 - Flags: review?(jdaggett) → review+
Attachment #677348 - Flags: review?(jdaggett) → review+
(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?
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.)
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+
Attachment #676550 - Attachment is obsolete: true
(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?
Argh, the changes *here* supersede those is what I meant to say...
https://hg.mozilla.org/mozilla-central/rev/a1688a1c6fce
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(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
Whiteboard: [adv-main19-]
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.