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

RESOLVED FIXED in Firefox 27

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

({sec-high})

unspecified
mozilla29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox26 wontfix, firefox27+ verified, firefox28+ verified, firefox29+ verified, firefox-esr2427+ verified, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.227+ fixed, b2g-v1.3 fixed)

Details

(Whiteboard: [adv-main27+][adv-esr24.3+])

Attachments

(7 attachments, 4 obsolete attachments)

1.57 KB, text/html
Details
2.78 KB, patch
abillings
: sec-approval+
Details | Diff | Splinter Review
5.88 KB, patch
abillings
: sec-approval+
Details | Diff | Splinter Review
3.87 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.28 KB, patch
Details | Diff | Splinter Review
7.28 KB, patch
Details | Diff | Splinter Review
9.47 KB, patch
Details | Diff | Splinter Review
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
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.
Attachment #8348681 - Flags: review?(roc)
Assignee: nobody → jfkthame
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.
Attachment #8348694 - Flags: review?(roc)
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
Attachment #8348883 - Flags: review?(roc)
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 :-)
Attachment #8348681 - Flags: review?(roc) → review+
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?
Attachment #8348883 - Flags: review?(roc) → review-
(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.
Now with a cleaner constructor. Carrying forward r=roc (but speak up if you see something wrong with it).
Attachment #8348681 - Attachment is obsolete: true
Attachment #8348694 - Attachment is obsolete: true
OK, here's a simplified version of the reftest.
Attachment #8349249 - Flags: review?(roc)
Attachment #8348883 - Attachment is obsolete: true
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.]
Attachment #8349247 - Flags: sec-approval?
Attachment #8349248 - Flags: sec-approval?
Attachment #8349247 - Flags: sec-approval? → sec-approval+
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.
Attachment #8349248 - Flags: sec-approval? → sec-approval+
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.
Target Milestone: --- → mozilla29
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
Attachment #8350543 - Flags: approval-mozilla-aurora?
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
Attachment #8350544 - Flags: approval-mozilla-aurora?
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.
Attachment #8350543 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8350544 - Flags: approval-mozilla-aurora? → approval-mozilla-esr24?
Attachment #8349247 - Flags: approval-mozilla-aurora?
Attachment #8349248 - Flags: approval-mozilla-aurora?
Attachment #8349247 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8349248 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8350543 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8350544 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Patch for b2g18. (Untested, but fairly straightforward, so I hope it'll be OK.)
Drat - sorry about that. I'll see what I can do...
Your attempted fix was -almost- correct, Ryan ... this version should be OK. It builds for me locally, at least.
Attachment #8355227 - Attachment is obsolete: true
Flags: in-testsuite?
Confirmed issue in FF28, 2013-11-13.
Verified fixed in 24esr, FF27, FF28 and FF29, 2014-01-18.
Status: RESOLVED → VERIFIED
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.
Keywords: sec-criticalsec-high
Whiteboard: [adv-main27+][adv-esr24.3+]
Technically, we haven't verified on B2G, so I'm changing status back to RESOLVED so that we don't lose track of this.
Status: VERIFIED → RESOLVED
Closed: 6 years ago6 years ago
Group: core-security
You need to log in before you can comment on or make changes to this bug.