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

RESOLVED FIXED in Firefox -esr52

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tedd, Assigned: mats)

Tracking

({csectype-uaf, sec-high})

unspecified
mozilla55
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr4554+ wontfix, firefox-esr5254+ fixed, firefox53 wontfix, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [adv-main54+][adv-esr52.2+])

Attachments

(1 attachment)

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
Group: core-security → gfx-core-security
Related to bug 1352295?
Flags: needinfo?(mats)
No, this isn't directly related to that bug.
Flags: needinfo?(mats)
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?
Component: Canvas: 2D → XPCOM
: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.
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!
Component: XPCOM → Canvas: 2D
Posted patch fixSplinter Review
No testing done so far, but hopefully it should be OK. :-)
Assignee: nobody → mats
Attachment #8853595 - Flags: review?(mstange)
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.
But it's used to count live allocated objects, so you'll run out of memory long before that.
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 :)
Ok the out of memory will probably happen on a 32 bit system.
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.
Attachment #8853595 - Flags: review?(mstange) → review+
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.
Attachment #8853595 - Flags: sec-approval?
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.
Attachment #8853595 - Flags: sec-approval? → sec-approval+
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
Attachment #8853595 - Flags: approval-mozilla-esr52?
Attachment #8853595 - Flags: approval-mozilla-beta?
We're don't have any more planned ESR45 releases, so marking this wontfix on that branch.
Whiteboard: [checkin on 5/3]
https://hg.mozilla.org/mozilla-central/rev/9faaded6a48d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8853595 [details] [diff] [review]
fix

Sec-high, Beta54+, ESR52.2+
Attachment #8853595 - Flags: approval-mozilla-esr52?
Attachment #8853595 - Flags: approval-mozilla-esr52+
Attachment #8853595 - Flags: approval-mozilla-beta?
Attachment #8853595 - Flags: approval-mozilla-beta+
Group: gfx-core-security → core-security-release
(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.
Flags: qe-verify-
Whiteboard: [adv-main54+][adv-esr52.2+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.