1352093 - Use-after-free due to ref counter overflow in CanvasRenderingContext2D

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

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

A ref counter overflow can be triggered by creating a lot of CanvasRenderingContext2D instances. This triggers free()ing the |sErrorTarget| object while some instances still hold a reference to with |mTarget|. Thus leading to a use-after-free.

The affected code is shown here [1], no check of |sNumLivingContexts| is performed whatsoever.

|sNumLivingContexts| is of type uint32_t which takes a while to overflow, nevertheless this should be fixed.

The instantiation of CanvasRenderingContext2D can be triggered by content by running:

> document.createElement('canvas').getContext('2d');

For an invalid canvas |mTarget| will be set to an error target by calling SetErrorState() [2].

In order to test the issue I changed the type to uint16_t so I don't have to run it for hours and executed the following script (also adding some debug prints)

> <canvas id="zero" width="133234324">
> </canvas>
> <script>
> c = document.getElementById("zero").toBlob(alert, "image/bmp");
> for (i = 0; i<0xffff+1; i++) {
>   document.createElement("canvas").getContext("2d");
> }
> </script>

calling toBlob() triggers the code:

> mTarget = sErrorTarget;

now |mTarget| holds a reference to the object stored in |sErrorTarget|, the loops wraps the ref counter around and sets it to 1.

Once the garbage collection kicks in, it will be decremented and the NS_IF_RELEASE is triggered on line 1113 [1], thus freeing sErrorTarget and setting the pointer to 0.

However the <canvas> element with id 'zero' still holds a reference to the free()ed object with |mTarget|. Any operation done on |c| in the javacsript context leads to using the using the free()ed mTarget object.

[1] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/canvas/CanvasRenderingContext2D.cpp#1088,1111
[2] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/canvas/CanvasRenderingContext2D.cpp#1741

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Related to bug 1352295?

Comment 2

[Mats Palmgren (inactive)]

No, this isn't directly related to that bug.

Comment 3

[Mats Palmgren (inactive)]

It seems to me we should fix this problem in general, not just for canvas.

Perhaps we should simply abort() if we overflow the uint32_t ref count?
Alternatively, change the type to size_t?

Comment 4

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

:mats, there are a couple of places where a 'custom' ref counting is used but they seem to mostly use nsrefcnt (or something like that) which is a typedef to uintptr_t so on a 64 bit system  this should be a 64 bit counter which takes forever to overflow.

But yes, probably a good idea to handle all cases, if there is a way to figure where those are.

Comment 5

[Mats Palmgren (inactive)]

It seems our default ref count is uintptr_t, although it has some flags in there too I guess.
http://searchfox.org/mozilla-central/rev/f668d2b2c1413452d536210b3ea2999eb81824f3/xpcom/base/nsISupportsImpl.h#272

So it looks like this is just a CanvasRenderingContext2D problem after all.
Sorry for not reading your description more carefully!

Comment 6

[Mats Palmgren (inactive)]

Attachment: fix

No testing done so far, but hopefully it should be OK. :-)

Comment 7

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

I don't think changing the counter type really fixes this issue.

On 32 bit the size of the value is still bound to 32 bit because afaik sizeof(uintptr_t) == sizeof(void*) which is 32 bit on a 32 bit arch.

Comment 8

[Mats Palmgren (inactive)]

But it's used to count live allocated objects, so you'll run out of memory long before that.

Comment 9

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

Maybe this can be fixed by checking if the counter is about to overflow and if so abort() or fail to create the CanvasRenderingContext2D object, or have a maximum number of contexts that can exist where that upper bound is < UINT32_MAX (or whatever the data type).

And then reject any subsequent instantiations.

I guess it is up to the reviewer :)

Comment 10

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

Ok the out of memory will probably happen on a 32 bit system.

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8853595 [details] [diff] [review]
fix

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

Nice and simple patch! We can never have more than uintptr_t-max different objects, so this should be completely sufficient.

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 8853595 [details] [diff] [review]
fix

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

It's kind of obvious what issue is given the code changes.  However, it's probably quite hard to exploit this bug since you'd have to request >4G canvas2d objects which takes many more gigabytes of RAM and hours to execute, so it's not really practical to exploit.

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

No, but the code change itself probably does.

Which older supported branches are affected by this flaw?

All branches. (The bug doesn't affect 32-bit builds, if we still do those.)

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

The patch should apply to all branches.

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

Zero risk.  No testing required.

Comment 13

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

Sec-approval+ for checkin on May 3, which is two weeks into the next cycle. We're releasing 53 in the next few days.

We'll want branch patches nominated as well so we can ship this on both ESR branches and 54 and 55.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8853595 [details] [diff] [review]
fix

https://hg.mozilla.org/integration/mozilla-inbound/rev/9faaded6a48d90c21b2773c656f6b87da552b197

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: potential use-after-free
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: just changes the type of the counter
[String changes made/needed]: none

Comment 15

[Ryan VanderMeulen [:RyanVM]]

We're don't have any more planned ESR45 releases, so marking this wontfix on that branch.

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/9faaded6a48d

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8853595 [details] [diff] [review]
fix

Sec-high, Beta54+, ESR52.2+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fe9a9a0ccf20
https://hg.mozilla.org/releases/mozilla-esr52/rev/d7c06f2d0d13

Comment 19

[Andrei Vaida [:avaida]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: no
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Ryan's assessment on manual testing needs and the fact that this fix has automated coverage.