950590 - canvas rendering context does not respect dynamic changes to the user font set

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: testcase showing <canvas> use of a no-longer-defined user font

See attached testcase.

The canvas context resolves the font family name to an actual platform font or user font at the time setFont is called; subsequent measureText or fillText calls will use that font.

However, according to [1], "if the font style source node does not have that font in scope at the time the font is to be used, then it must be treated as if it was an unknown font, falling back to another". In other words, if setFont resolved to a user font, but that user font is subsequently removed (the stylesheet that defined it is deactivated, for example), then fillText etc should NOT continue to use it.

The left-hand canvas in the testcase demonstrates the issue: the user font "test", which is defined as a local script or serif face, should be removed from the font set *before* the middle line "expect monospace?" is drawn, and so that line should be rendered with the fallback monospace font.

FWIW, Chrome also appears to get this wrong.

Filing this as security-sensitive, because the canvas context failure to notice user font set updates can lead to use-after-free errors and crashes, as in bug 950000. The use of src:local() in the testcase here avoids that particular issue, but the example still points towards a potential vulnerability, IMO.

If we fix this, I think it will resolve the crash in bug 950000 as well (whereas the reverse is not true: we could fix that crash without addressing the incorrect behavior here).

[1] http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas/#text-styles

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - use nsRefPtr instead of manual addref/release calls for gfxFontGroup's reference to the user font set

Some preliminary cleanup. I don't see any reason why gfxFontGroup needs to use a raw gfxUserFontSet* pointer and then manually addref/release it; we can just make it an nsRefPtr and let that take care of the refcounting.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - support updating a gfxFontGroup's user font set on the fly; make canvas rendering context update the font set before drawing/measuring text.

This fixes the behavior of the testcase here, such that both canvases render the same. It also prevents the crash in bug 950000 that occurs due to using a stale mUserFontSet pointer from the font group in the canvas context's state.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - reftest for canvas use of a user font after it is no longer defined.

Reftest based on the testcase above, to check that we don't draw with a font that has been undefined. Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=386960cf8efc

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8348681 [details] [diff] [review]
part 1 - use nsRefPtr instead of manual addref/release calls for gfxFontGroup's reference to the user font set

Review of attachment 8348681 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that

::: gfx/thebes/gfxFont.cpp
@@ +4064,5 @@
>      , mHyphenWidth(-1)
>      , mTextPerf(nullptr)
>  {
> +    mUserFontSet = aUserFontSet;
> +    mCurrGeneration = GetGeneration();

Why not keep calling SetUserFontSet? 1 line instead of 2 :-)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8348883 [details] [diff] [review]
part 3 - reftest for canvas use of a user font after it is no longer defined.

Review of attachment 8348883 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/test/reftest/stale-userfont-1-ref.html
@@ +30,5 @@
> +  c2.fillText("expect test font (script/serif)", 10, 50);
> +
> +  // now we remove the stylesheet that defined "test",
> +  // so it should become unknown and text should fall back
> +  document.getElementById("s").remove();

Can't we simplify the reference here so that we don't have to add and remove stylesheets?

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)

> ::: gfx/thebes/gfxFont.cpp
> @@ +4064,5 @@
> >      , mHyphenWidth(-1)
> >      , mTextPerf(nullptr)
> >  {
> > +    mUserFontSet = aUserFontSet;
> > +    mCurrGeneration = GetGeneration();
> 
> Why not keep calling SetUserFontSet? 1 line instead of 2 :-)

Ah, yes, we can do that here. This made more sense before I split the patch into stages: when the behavior of SetUserFontSet changes (to possibly call UpdateFontList), it's no longer suitable for use from the gfxFontGroup constructor; we just want BuildFontList there.

I'll tidy up the gfxFontGroup constructor a bit more and refresh the patches.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - use nsRefPtr instead of manual addref/release calls for gfxFontGroup's reference to the user font set.

Now with a cleaner constructor. Carrying forward r=roc (but speak up if you see something wrong with it).

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - support updating a gfxFontGroup's user font set on the fly; make canvas rendering context update the font set before drawing/measuring text.

And the corresponding update to part 2. Carrying forward r=roc.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - reftest for canvas use of a user font after it is no longer defined.

OK, here's a simplified version of the reftest.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 8349247 [details] [diff] [review]
part 1 - use nsRefPtr instead of manual addref/release calls for gfxFontGroup's reference to the user font set.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

The patch implies we found an issue with user fonts (@font-face), dynamic stylesheet changes, and HTML <canvas>. It's not a big stretch to go from there to constructing an example that can hit a use-after-free error and potentially a crash, such as in bug 950000. Whether this can be leveraged to a more serious exploit is unclear to me, but I can't guarantee it *isn't* possible.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Not a bulls-eye (the testcase in bug 950000 comes closer to that), but they do hint in the general direction of the flaw.

Which older supported branches are affected by this flaw?

All current branches.

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

Longstanding issue.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Straightforward to backport; there are some unrelated changes in the context, so a manual merge rather than just hg transplant will be needed, but no conflicts that affect the actual code changes here.

How likely is this patch to cause regressions; how much testing does it need?

Little risk of new regressions, IMO; this is fixing a corner case that is currently broken and leads to use-after-free or other undefined behavior. We have unit tests that cover the more typical uses of @font-face fonts.


[NB: the above comment applies to the combination of the two code patches here. The first patch does not change behavior or fix any bugs in itself, it is just preparatory cleanup before the actual changes in part 2.]

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Calling this a sec-critical

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8349248 [details] [diff] [review]
part 2 - support updating a gfxFontGroup's user font set on the fly; make canvas rendering context update the font set before drawing/measuring text.

sec-approval+ for trunk. We'll want branch patches (including ESR24) made and nominated as well.

Comment 13

[Jonathan Kew [:jfkthame]]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d46a72c9085d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b712295eed24

I'll upload the rebased patches for branches once we see this sticks.

Not landing the testcase for now, as it provides the most direct indication of how to actually hit the problem codepath from a web page.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d46a72c9085d
https://hg.mozilla.org/mozilla-central/rev/b712295eed24

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: use nsRefPtr for gfxFontGroup's reference to the user font set, and support changing it from canvas context.

Parts 1 and 2 combined into single patch for mozilla-beta.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: use nsRefPtr for gfxFontGroup's reference to the user font set, and support changing it from canvas context.

Parts 1 and 2 combined into single patch for mozilla-esr24.

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 8350543 [details] [diff] [review]
use nsRefPtr for gfxFontGroup's reference to the user font set, and support changing it from canvas context.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Longstanding bug (combination of canvas2d + @font-face support)

User impact if declined: 
Potential for web page to crash the browser due to using a stale font set (as per bug 950000); unclear whether this could be further exploited

Testing completed (on m-c, etc.): 
Verified locally to prevent the crash with bug 950000 testcase, as well as fixing the incorrect behavior demonstrated in testcase here. Currently on mozilla-central.

Risk to taking this patch (and alternatives if risky): 
Low-risk patch to just ensure the font set is up to date when performing canvas text operations. This is less intrusive and risky than the alternative crash-fix suggested in bug 950000 that only avoided the use-after-free, but didn't fix the incorrect behavior.

String or IDL/UUID changes made by this patch:
none

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8350544 [details] [diff] [review]
use nsRefPtr for gfxFontGroup's reference to the user font set, and support changing it from canvas context.

[Approval Request Comment]
as above

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 8350543 [details] [diff] [review]
use nsRefPtr for gfxFontGroup's reference to the user font set, and support changing it from canvas context.

Argh, sorry, I set the wrong a? flags on these patches. The two "combined" patches are for mozilla-beta and mozilla-esr24 respectively; for aurora, the original m-c patches apply unchanged.

Moving flags to the proper patches... apologies for the extra bugspam.

Comment 20

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b603966034c
https://hg.mozilla.org/releases/mozilla-aurora/rev/af28fe58e263

Comment 21

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-beta/rev/072a8ecc163b

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/6143d1bc4f00
https://hg.mozilla.org/releases/mozilla-esr24/rev/c1b000119743

Please post a patch for b2g18 when you get a chance as well.

Comment 23

[Jonathan Kew [:jfkthame]]

Attachment: use nsRefPtr for gfxFontGroup's reference to the user font set, and support changing it from canvas context.

Patch for b2g18. (Untested, but fairly straightforward, so I hope it'll be OK.)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, it's busted :(
https://tbpl.mozilla.org/php/getParsedLog.php?id=32592426&tree=Mozilla-B2g18

I attempted to fix it with this:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/3cd79438d0ec

And got this:
https://tbpl.mozilla.org/php/getParsedLog.php?id=32595914&tree=Mozilla-B2g18

Comment 25

[Jonathan Kew [:jfkthame]]

Drat - sorry about that. I'll see what I can do...

Comment 26

[Jonathan Kew [:jfkthame]]

Attachment: use nsRefPtr for gfxFontGroup's reference to the user font set, and support changing it from canvas context.

Your attempted fix was -almost- correct, Ryan ... this version should be OK. It builds for me locally, at least.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Thanks :)

https://hg.mozilla.org/releases/mozilla-b2g18/rev/66c4a3131c39

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/66c4a3131c39

Comment 29

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed issue in FF28, 2013-11-13.
Verified fixed in 24esr, FF27, FF28 and FF29, 2014-01-18.

Comment 30

[Daniel Veditz [:dveditz]]

There's enough doubts in comment 10 to make this a sec-high rather than an immanent sec-critical issue. Not that it really matters in the long run -- we fixed it.

Comment 31

[Matt Wobensmith [:mwobensmith][:matt:]]

Technically, we haven't verified on B2G, so I'm changing status back to RESOLVED so that we don't lose track of this.