1352295 - mozilla::dom::CanvasRenderingContext2D is trivially exploitable

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P1)

Description

[Mats Palmgren (inactive)]

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.

Comment 1

[Mats Palmgren (inactive)]

https://crash-stats.mozilla.com/search/?signature=~CanvasRenderingContext2D

Comment 2

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]:

Comment 3

[Marcia Knous [:marcia]]

Tracking for all branches nominated that are also affected.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

This got better with bug 1318283, but it didn't all go away.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 6

[Mats Palmgren (inactive)]

Attachment: agarplus_v2c0.js

(saving the main script file of the site for reference)

Comment 7

[Mats Palmgren (inactive)]

Regression from: https://hg.mozilla.org/mozilla-central/rev/50c97a5a683a

Comment 8

[Mats Palmgren (inactive)]

Attachment: 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...

Comment 9

[Mats Palmgren (inactive)]

Attachment: CRASH testcase, STR: load it, then resize the window

This is roughly what this site is doing.

Comment 10

[Mats Palmgren (inactive)]

Attachment: part 1 - Deal with FlushPendingNotifications being potentially destructive better in UpdateFilter() and its direct callers.

Comment 11

[Mats Palmgren (inactive)]

Attachment: part 2 - Deal with NeedToCalculateBounds() being potentially destructive.

Comment 12

[Mats Palmgren (inactive)]

Attachment: part 3 - Deal with NeedToApplyFilter() being potentially destructive.

Comment 13

[Markus Stange [:mstange]]

(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!

Comment 14

[Mats Palmgren (inactive)]

Attachment: part 4 - Deal with the AdjustedTarget constructor being potentially destructive.

Comment 15

[Mats Palmgren (inactive)]

Attachment: part 5 - Deal with the AdjustedTarget::MaxSourceNeededBoundsForFilter/BoundsAfterFilter being potentially destructive.

Comment 16

[Mats Palmgren (inactive)]

Attachment: part 6 - Deal with UsedOperation() being potentially destructive.

Comment 17

[Mats Palmgren (inactive)]

Attachment: part 7 - Make CurrentState() usage safe (can't use an earlier result after a flush).

Comment 18

[Mats Palmgren (inactive)]

Attachment: part 8 - Make Save/Restore more robust.

Comment 19

[Mats Palmgren (inactive)]

Attachment: part 9 - Deal with Stroke() being potentially destructive.

Comment 20

[Mats Palmgren (inactive)]

Attachment: part 10 - Add missing IsTargetValid() checks after EnsureTarget() calls.

This patch addresses bug 1352105, which is again something *should* have been done in bug 1299062.

Comment 21

[Mats Palmgren (inactive)]

Attachment: part 11 - Fix various code style issues to make the code easier to read.

(last part)

Comment 22

[Mats Palmgren (inactive)]

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.

Comment 23

[Mats Palmgren (inactive)]

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.

Comment 24

[Mats Palmgren (inactive)]

Attachment: stack for mPath = null with mPathTransformWillUpdate == true

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.

Comment 25

[Mats Palmgren (inactive)]

Attachment: part 12 - Reset mPathTransformWillUpdate in SetInitialState().

Comment 26

[Julian Hector [:tedd] [:jhector]]

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 27

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 29

[Markus Stange [:mstange]]

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.

Comment 30

[Markus Stange [:mstange]]

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 :(

Comment 31

[Markus Stange [:mstange]]

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.

Comment 32

[Mats Palmgren (inactive)]

Attachment: part 1 - Deal with FlushPendingNotifications being potentially destructive better in UpdateFilter() and its direct callers.

(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.

Comment 33

[Mats Palmgren (inactive)]

(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.

Comment 34

[Mats Palmgren (inactive)]

s/if we resize events/if we fix resize events/

Comment 35

[Markus Stange [:mstange]]

(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.

Comment 36

[Mats Palmgren (inactive)]

> 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.

Comment 37

[Markus Stange [:mstange]]

Sounds good. Thanks so much!

Comment 38

[Mats Palmgren (inactive)]

Attachment: Rollup patch with comments removed

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?

Comment 39

[Daniel Veditz [: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.

Comment 40

[Mats Palmgren (inactive)]

Filed bug 1355873 as a cover bug.

Comment 41

[Mats Palmgren (inactive)]

Attachment: patch to re-add the code comments

Comment 42

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 44

[Milan Sreckovic [:milan] (needinfo for best results)]

Once we fix the regressions seemingly caused by bug 1355873.  See the comments in that bug.

Comment 45

[Mats Palmgren (inactive)]

(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).

Comment 46

[Petruta Horea [:phorea], Desktop Test Engineering]

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.