Closed Bug 1352295 Opened 3 years ago Closed 3 years ago

mozilla::dom::CanvasRenderingContext2D is trivially exploitable

Categories

(Core :: Canvas: 2D, defect, P1, critical)

54 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox52 --- wontfix
firefox-esr52 54+ fixed
firefox53 + wontfix
firefox54 + verified
firefox55 + verified

People

(Reporter: mats, Assigned: mats)

References

()

Details

(4 keywords, Whiteboard: [fixed by bug 1355873][post-critsmash-triage][adv-main54+][adv-esr52.2+])

Attachments

(18 files, 1 obsolete file)

237.62 KB, text/plain
Details
3.87 KB, text/plain
Details
1.35 KB, text/html
Details
9.51 KB, patch
mstange
: review+
Details | Diff | Splinter Review
2.28 KB, patch
mstange
: review+
Details | Diff | Splinter Review
11.43 KB, patch
mstange
: review+
Details | Diff | Splinter Review
2.73 KB, patch
mstange
: review+
Details | Diff | Splinter Review
12.32 KB, patch
mstange
: review+
Details | Diff | Splinter Review
17.45 KB, patch
mstange
: review+
Details | Diff | Splinter Review
1.43 KB, patch
mstange
: review+
Details | Diff | Splinter Review
3.23 KB, patch
mstange
: review+
Details | Diff | Splinter Review
9.65 KB, patch
mstange
: review+
Details | Diff | Splinter Review
22.55 KB, patch
mstange
: review+
Details | Diff | Splinter Review
3.24 KB, text/plain
Details
954 bytes, patch
mstange
: review+
Details | Diff | Splinter Review
2.54 KB, patch
mstange
: review+
Details | Diff | Splinter Review
60.57 KB, patch
Details | Diff | Splinter Review
23.46 KB, patch
Details | Diff | Splinter Review
Spawned off from bug 1329796 comment 42:
There is one URL with 8 reported crashes in 55.0a1 and it crashed immediately
for me using a Linux64 debug build:  http://dual-agar.online/

In mozilla::dom::CanvasRenderingContext2D::StrokeRect
 3143      AdjustedTarget(this, bounds.IsEmpty() ? nullptr : &bounds)->
 3144        StrokeRect(gfx::Rect(aX, aY, aW, aH),
 3145                   CanvasGeneralPattern().ForStyle(this, Style::STROKE, mTarget),
 3146                   StrokeOptions(state.lineWidth, state.lineJoin,
 3147                                 state.lineCap, state.miterLimit,
>3148                                 state.dash.Length(),
 3149                                 state.dash.Elements(),
 3150                                 state.dashOffset),
 3151                   DrawOptions(state.globalAlpha, UsedOperation()));
 3152

|state| points to freed memory, so we crash somewhere in nsTArray on line 3148.
Tracking for all branches nominated that are also affected.
This got better with bug 1318283, but it didn't all go away.
I can't reproduce on Windows, but if you have a reproducible case, this patch https://bug1318283.bmoattachments.org/attachment.cgi?id=8814490 (may not apply cleanly) is useful in tracking the disappearance of the target that usually causes this kind of a problem.
Blocks: 1352105
Attached file agarplus_v2c0.js
(saving the main script file of the site for reference)
Attached file Debugging notes
So here's what goes wrong...

In StrokeRect we store a pointer to our CurrentState()...
http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/canvas/CanvasRenderingContext2D.cpp#3086
... which is the address of a slot inside an AutoTArray:
http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/canvas/CanvasRenderingContext2D.h#3086

Then StrokeRect goes on to call a number of methods that may flush layout...
The site in question has a resize listener, which gets called and decides
to do numerous additional canvas calls, among other things Save/Restore,
which adds/removes entries on the array above which causes it to be
reallocated...

It appears this problem was already known (bug 1308859 comment 13) but
no serious analysis was ever made of other code. :(
I have now done that analysis and found that numerous other methods are
affected, in pretty much the same way.  And these methods are exposed
to JS...
This is roughly what this site is doing.
(In reply to Mats Palmgren (:mats) from comment #8)
> It appears this problem was already known (bug 1308859 comment 13) but
> no serious analysis was ever made of other code. :(

I completely dropped the ball on this one. Sorry :(
At the time, there were multiple fixes happening in this code in parallel and I think I was assuming that one of them was taking care of this, but apparently that was not the case.

> I have now done that analysis and found that numerous other methods are
> affected, in pretty much the same way. 

Thank you for looking at this!
Attachment #8853805 - Flags: review?(mstange)
This patch addresses bug 1352105, which is again something *should* have been done in bug 1299062.
Attachment #8853808 - Flags: review?(mstange)
Most of these changes are fairly self-evident, but there are a couple of
changes I'd like to explain a bit:

In part 2, I move these lines:
+  // This is only needed to know if we can know the drawing bounding box easily.
+  const bool doCalculateBounds = NeedToCalculateBounds(); // may flush
a bit earlier in the method.  This is to ensure any potential flush is done
before the GetCurrentFontStyle() call so that we don't have to worry about
whether the |currentFontStyle| pointer is stale too.  I think reversing
the order there shouldn't matter.

In part 7, I removed the CanvasBidiProcessor::mState member because I think
it's just too dangerous to have something like that.
BTW, with these patches there is still an assertion for the testcase:
###!!! ASSERTION: mPathTransformWillUpdate should be false, if all paths are null: '!mPathTransformWillUpdate', file dom/canvas/CanvasRenderingContext2D.cpp, line 3691
I haven't looked in detail what that is about yet.
Here's the stack when we set mPath to null with mPathTransformWillUpdate == true
http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/canvas/CanvasRenderingContext2D.cpp#1728
which triggers the assertion in comment 23 later:
http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/canvas/CanvasRenderingContext2D.cpp#3528-3529

This seems like a minor issue in SetInitialState() - I'm guessing we should
reset mPathTransformWillUpdate there like the other members.
Nice work :mats, once you linked to this Bug from Bug 1352105, I started looking more into it, but I didn't get as far as you did, I was on track with the CurrentState() issue and assumed that the state 'stack' was modified while still holding references to it.

However I had to spent too much time on getting familiar with the code, do we have any kind of documentation of the underlying canvas code and its components?
Comment on attachment 8853800 [details] [diff] [review]
part 3 - Deal with NeedToApplyFilter() being potentially destructive.

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +606,5 @@
>        op = gfx::CompositionOp::OP_OVER;
>      }
>  
>      // Now set up the filter draw target.
> +    const bool applyFilter = aCtx->NeedToApplyFilter(); // may flush

NeedToApplyFilter() calls EnsureTarget() which should make sure that we have a valid target.  Sounds like we have a scenario where we have a valid target, we call NeedToApplyFilter(), we call EnsureTarget() and we end up with the invalid target?
Group: core-security → gfx-core-security
Duplicate of this bug: 1352105
Attachment #8853798 - Flags: review?(mstange) → review+
Attachment #8853799 - Flags: review?(mstange) → review+
Comment on attachment 8853798 [details] [diff] [review]
part 1 - Deal with FlushPendingNotifications being potentially destructive better in UpdateFilter() and its direct callers.

>@@ -2923,16 +2924,23 @@ CanvasRenderingContext2D::UpdateFilter()

>   presShell->FlushPendingNotifications(FlushType::Frames);
>+  if (presShell->IsDestroying() || mStyleStack.IsEmpty()) {
>+    if (mStyleStack.IsEmpty()) {

How can the style stack become empty?

>+      SetErrorState();

Milan brings up a good point - we call EnsureTarget after we call UpdateFilter, and EnsureTarget will re-create the target and overwrite the error state we set here.
Attachment #8853800 - Flags: review?(mstange) → review+
Comment on attachment 8853801 [details] [diff] [review]
part 4 - Deal with the AdjustedTarget constructor being potentially destructive.

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

This is all so extremely unfortunate :(
Attachment #8853801 - Flags: review?(mstange) → review+
Attachment #8853802 - Flags: review?(mstange) → review+
Attachment #8853803 - Flags: review?(mstange) → review+
Attachment #8853804 - Flags: review?(mstange) → review+
Attachment #8853805 - Flags: review?(mstange) → review+
Attachment #8853807 - Flags: review?(mstange) → review+
Attachment #8853808 - Flags: review?(mstange) → review+
Attachment #8853810 - Flags: review?(mstange) → review+
Attachment #8853824 - Flags: review?(mstange) → review+
Mats, do you have ideas for alternative ways of dealing with this problem?

The flush is really only necessary for this case:
  ctx.filter = "url(#someFilterWhichDoesntExistYet");
  // ... create an SVG <filter> element with that ID
  // ... and then draw using that filter:
  ctx.fillRect(...)

Flushing in the "filter" property setter wouldn't find the correct filter element, so we also need to flush in the draw call.

Here are the alternatives I could come up with:
 - Have an explicit flush call that we only call at the beginning of each draw call, and don't flush in NeedToApplyFilter
 - Don't run resize events synchronously - I think you've tried this before, do you remember the bug number?
 - Unship support for url() filters
 - Unship support for all canvas filters

Chrome is the only other browser to support canvas filters.
(In reply to Markus Stange [:mstange] from comment #29)
> >+  if (presShell->IsDestroying() || mStyleStack.IsEmpty()) {
> >+    if (mStyleStack.IsEmpty()) {
> 
> How can the style stack become empty?

I guess we can assert that isn't.
Attachment #8853798 - Attachment is obsolete: true
Attachment #8855959 - Flags: review?(mstange)
Attachment #8855959 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #31)
> Here are the alternatives I could come up with:
>  - Have an explicit flush call that we only call at the beginning of each
> draw call, and don't flush in NeedToApplyFilter

Yes, I think that's how it's normally done.
Something like this should be generally safe I think:

FlushPendingNotifications()
if (some state you depend on is now invalid)
  return;
nsAutoScriptBlocker scriptBlocker;
... do your thing ...

>  - Don't run resize events synchronously - I think you've tried this before,
> do you remember the bug number?

Bug 1149555, and some discussion in bug 1168195.  But note that even if we
resize events, there's likely other things that can lead to script running so
I wouldn't trust FlushPendingNotifications to not do that for the foreseeable
future.

>  - Unship support for url() filters
>  - Unship support for all canvas filters
> 
> Chrome is the only other browser to support canvas filters.

Sounds like it's too late for that.
s/if we resize events/if we fix resize events/
(In reply to Mats Palmgren (:mats) from comment #33)
> (In reply to Markus Stange [:mstange] from comment #31)
> > Here are the alternatives I could come up with:
> >  - Have an explicit flush call that we only call at the beginning of each
> > draw call, and don't flush in NeedToApplyFilter
> 
> Yes, I think that's how it's normally done.
> Something like this should be generally safe I think:
> 
> FlushPendingNotifications()
> if (some state you depend on is now invalid)
>   return;
> nsAutoScriptBlocker scriptBlocker;
> ... do your thing ...

I see. Would you like to try that, or would you prefer to go ahead with your current patches?

> >  - Don't run resize events synchronously - I think you've tried this before,
> > do you remember the bug number?
> 
> Bug 1149555, and some discussion in bug 1168195.  But note that even if we
> resize events, there's likely other things that can lead to script running so
> I wouldn't trust FlushPendingNotifications to not do that for the foreseeable
> future.

:(

Thanks for the pointers.
> I see. Would you like to try that, or would you prefer to go ahead with your current patches?

I think I prefer to take this patch now since it improves error handling
in general and it's probably slightly less risky since the calls still
occurs in the same order.
We can move around the UpdateFilter calls in a follow-up bug.
Sounds good. Thanks so much!
As a rollup patch with the comments removed I think we could pass this off
as a general "cleanup and improved error handling" patch in an open bug.

I suspect that the comments + coming from a hidden bug is a bit too obvious
that this is a security issue, and it's not hard to find something that
crashes here, with just JS.

WDYT?
Flags: needinfo?(dveditz)
This is way more than we'd want to crash-land into 53, but the comment-free patch looks fine for an aurora (or soon beta) landing. Aurora would be better for a patch this large: beta gets more scrutiny. We should delay landing on ESR-52 until pretty late in the cycle since that's an "interesting patches only" branch. We don't get any testing feedback on that branch anyway.
Flags: needinfo?(dveditz)
Filed bug 1355873 as a cover bug.
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [fixed by bug 1355873]
Target Milestone: --- → mozilla55
We're about to ship our final planned ESR45 release, so calling this wontfix there unless it turns into a chemspill at some point between now and June.
Mats, I've verified that bug 1355873 grafts cleanly to Aurora, but it will require rebasing for ESR52. Can you please attach a rebased patch here at some point when you get a chance?
Flags: needinfo?(mats)
Once we fix the regressions seemingly caused by bug 1355873.  See the comments in that bug.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #43)
> but it will require rebasing for ESR52
Added on bug 1355873 (oops, I guess I spilled the info we're considering taking it on ESR).
Flags: needinfo?(mats)
Group: gfx-core-security → core-security-release
Flags: qe-verify+
Whiteboard: [fixed by bug 1355873] → [fixed by bug 1355873][post-critsmash-triage]
Whiteboard: [fixed by bug 1355873][post-critsmash-triage] → [fixed by bug 1355873][post-critsmash-triage][adv-main54+][adv-esr52.2+]
Reproduced using a Linux 64 debug build and the test case from comment 9 under Ubuntu 14.04 LTS 64-bit.
The tab is no longer crashing with 54.0 and 55.0 Linux 64 debug builds.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.