Closed Bug 836225 Opened 7 years ago Closed 7 years ago

crash when shaping supplementary-plane text with graphite font

Categories

(Core :: Graphics: Text, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox19 --- disabled
firefox20 - fixed
firefox21 + fixed
firefox-esr17 - disabled
b2g18 --- disabled

People

(Reporter: martin_hosken, Assigned: jfkthame)

References

Details

(Keywords: crash, sec-critical, testcase, Whiteboard: [adv-main20+])

Attachments

(5 files, 1 obsolete file)

Attached file font and test html
The enclosed iswa font is graphite based and crashes firefox. There is a crash report with my name on it somewhere.
Martin's crash: http://crash-stats.mozilla.com/report/index/bp-46755649-b3bf-4ad7-8f9c-cfc802130130

Also crashes for me on OS X.

Provisionally marking security-sensitive, pending further investigation.
Group: core-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
In a debug build, this hits a bunch of assertions before crashing, starting with
###!!! ASSERTION: unexpected offset: 'offs >= j && offs < aLength', file /Users/jkew/mozdev/mc-clean/gfx/thebes/gfxGraphiteShaper.cpp, line 380

Either graphite is giving us strange output, or we're doing something wrong with the output it gives. Will investigate shortly.
Attached file callstack
This is already crashing earlier and shows an invalid read of 4 bytes in an ASan enabled build.
Attached file testcase-minimized
Minimized the testcase and replaced the glyphs with HTML entities:

&#1038339;
This is looking like a graphite engine bug - it seems to be returning a spurious value for the number of glyphs in the run (gr_seg_n_slots), which leads to us accessing lots of out-of-bounds stuff when trying to process glyphs that aren't actually there.
Blocks: 700023
Turns out the bug is in our integration (gfxGraphiteShaper) after all - we pass the number of utf16 code units to the gr_make_seg function, but what it actually wants is the number of Unicode characters. So this affects any non-BMP text. Patch coming up.
In addition to correcting the length parameter to gr_make_seg, we should convert character indices back to utf16 code point offsets immediately during the cluster-finding loop, so that all our char/glyph processing is consistently done in terms of utf16 codes. With this fix, the example here (using Plane 15 characters) works properly, AFAICS.
Attachment #708153 - Flags: review?(jdaggett)
(We should also have a crashtest for this, though we might want to hold off on that until the fix is in place.)
(In reply to Jonathan Kew (:jfkthame) from comment #7)
> Created attachment 708153 [details] [diff] [review]
> handle supplementary-plane chars properly in graphite shaper
> 
> In addition to correcting the length parameter to gr_make_seg, we should
> convert character indices back to utf16 code point offsets immediately
> during the cluster-finding loop, so that all our char/glyph processing is
> consistently done in terms of utf16 codes. With this fix, the example here
> (using Plane 15 characters) works properly, AFAICS.

Hmmm, shouldn't the checks in the asserts also be if (xxx) continue statements to be very sure we avoid out-of-bounds array references?
(In reply to John Daggett (:jtd) from comment #9)
> (In reply to Jonathan Kew (:jfkthame) from comment #7)
> > Created attachment 708153 [details] [diff] [review]
> > handle supplementary-plane chars properly in graphite shaper
> > 
> > In addition to correcting the length parameter to gr_make_seg, we should
> > convert character indices back to utf16 code point offsets immediately
> > during the cluster-finding loop, so that all our char/glyph processing is
> > consistently done in terms of utf16 codes. With this fix, the example here
> > (using Plane 15 characters) works properly, AFAICS.
> 
> Hmmm, shouldn't the checks in the asserts also be if (xxx) continue
> statements to be very sure we avoid out-of-bounds array references?

Possibly, though they'd then have an impact on release build performance. In general, we don't expect release builds to include all the same sanity-checks for internal consistency that we have in debug builds.
Summary: A very large font crashes firefox → crash when shaping supplementary-plane text with graphite font
Crashtest using a copy of the PigLatin graphite font with characters mapped into plane 15. With graphite enabled, this at least throws a bunch of assertions (may or may not actually crash the browser) without the patch here; loads cleanly once the patch is applied.
Attachment #708252 - Flags: review?(jdaggett)
And now with a test-pref() annotation, so that the test can run usefully even before we pref-on graphite by default.
Attachment #708269 - Flags: review?(jdaggett)
Attachment #708252 - Attachment is obsolete: true
Attachment #708252 - Flags: review?(jdaggett)
Assignee: nobody → jfkthame
OS: Linux → All
Version: 18 Branch → Trunk
Were you planning on enabling graphite in Firefox 21, or in a later cycle?
(In reply to Daniel Veditz [:dveditz] from comment #13)
> Were you planning on enabling graphite in Firefox 21, or in a later cycle?

I was hoping to try for FF21, and I think we're getting really close to being ready; but before moving forward with that, I want to get the tests in bug 700022 safely landed. Which may take a little while, as other priorities keep popping up...
Review ping... would be nice to get this into the tree soon.
Attachment #708153 - Flags: review?(jdaggett) → review+
Attachment #708269 - Flags: review?(jdaggett) → review+
Comment on attachment 708153 [details] [diff] [review]
handle supplementary-plane chars properly in graphite shaper

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It would be pretty easy to construct an example that hits the faulty codepath (for users with graphite preffed on), and is likely to crash.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The code change makes it pretty clear the problem related to the use of supplementary-plane characters with graphite.

Which older supported branches are affected by this flaw?
All current branches, but the feature is preffed off by default, so nobody is exposed unless they have deliberately preffed on the graphite font support in about:config.

If not all supported branches, which bug introduced the flaw?
All branches.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Backports would be pretty simple - the patch doesn't apply as-is due to changes in the context, but the actual code fix needed remains the same so the fixup would be straightforward.
Given that the feature is preffed off by default, it's not clear to me that it is worth backporting the fix, but if desired I will prepare the patches for older branches.

How likely is this patch to cause regressions; how much testing does it need?
Simple fix for incorrect use of the API, should be very little risk of regression.
Attachment #708153 - Flags: sec-approval?
Attachment #708153 - Flags: sec-approval? → sec-approval+
(In reply to Jonathan Kew (:jfkthame) from comment #16)
> Which older supported branches are affected by this flaw?
> All current branches, but the feature is preffed off by default, so nobody
> is exposed unless they have deliberately preffed on the graphite font
> support in about:config.

Given this, no need to take code change.
Comment on attachment 708153 [details] [diff] [review]
handle supplementary-plane chars properly in graphite shaper

Although this isn't critical to track for FF20 release, as it is preffed off by default, requesting approval to land the fix on Aurora so that it will work for those users who know they need it and specifically enable the feature.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): graphite font support

User impact if declined: users wanting to use graphite fonts and supplementary-plane text (there's a project actively developing fonts for exactly this scenario) will not be able to work with FF20

Testing completed (on m-c, etc.): tested locally with the sign-language font prototype, verified to prevent the assertions in debug builds as well as the actual crash

Risk to taking this patch (and alternatives if risky): the feature is preffed off by default, so no risk to users in general

String or UUID changes made by this patch: none
Attachment #708153 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/118c857ffee0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 708153 [details] [diff] [review]
handle supplementary-plane chars properly in graphite shaper

Approving since it's low risk (preffed off by default).
Attachment #708153 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/49a5773f3ed0

Updated status-firefox20 to "fixed", as the potential crash bug is now fixed for FF20, although the feature remains preffed off by default anyway.
Whiteboard: [adv-main20+]
Pushed the crashtest, as the fix here has long since shipped to all relevant channels.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7299b4f7f43
Group: core-security
You need to log in before you can comment on or make changes to this bug.