419718 - Crash if <CANVAS> destroyed but drawing context kept

Status: VERIFIED FIXED

(Core :: Canvas: 2D, defect, P2)

Description

[Michal Zalewski]

As per subject line. If canvas.getContext() is accessed after canvas object is destroyed, the browser will crash on NULL ptr. To repro, visit demo URL, select first two checkboxes.

This is discovered as a part of <CANVAS> tag fuzzing. There are also several DoS conditions triggered to this fuzzer, and some one-off crashes I'm yet to troubleshoot - but overall, Firefox seems to have the most robust implementation of all browsers that support this functionality (that is, all other browsers have more serious flaws).

Please keep this marked as security-sensitive in the coming weeks, or remove demo URL if needs to be unlocked earlier.

Cheers,
/mz

Comment 1

[Daniel Veditz [:dveditz]]

Trunk crash is in nsCanvasRenderingContext2D::DoDrawImageSecurityCheck
http://crash-stats.mozilla.com/report/index/6328d8bd-e4ce-11dc-a660-001a4bd46e84

Same crash in FF2.0.0.12, mCanvas is null on the line linked to by the above crash report so it does look safe.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Youch.  I'll get on this, should be a straightforward fix.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix

We already correctly see that the canvas element went away, we just need some more null checks in place.  This fixes the issue for me -- i'm letting the fuzzer churn with the first, third, and fourth option checked now to see what happens.  (No more crash with first and second).

Note that we'll need a smaller variant of this patch on Fx2 as well.

Comment 4

[Stuart Parmenter]

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

shift elem->GetOwnerDoc()->GetPrimaryShell()->GetPresContext()

in to a temporary

Comment 5

[Olli Pettay [:smaug]]

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

>+        nsCOMPtr<nsINode> elem = do_QueryInterface(mCanvasElement);
>+        if (elem) {
>+            devPixel = elem->GetOwnerDoc()->GetPrimaryShell()->GetPresContext()->AppUnitsPerDevPixel();
>+            cssPixel = elem->GetOwnerDoc()->GetPrimaryShell()->GetPresContext()->AppUnitsPerCSSPixel();
You're relying quite many things to be alive. Is this code only called when document really has a presshell?
Or what if it is called during shutdown where element may not actually have even ownerdoc.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Good point.  Updated to my patch to do some null checking along the way there.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: crashtest, don't click, use view-source:

crashtest for Jesse -- I can't get a crash instantly, usually takes a few seconds.  I'm not sure how to get the canvas element to be destroyed sooner.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: patch for 1.8.1

Patch for 1.8.1.  Very simple, just adds a few null checks; fewer needed here than in 1.9.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated trunk patch with comments

Updated trunk patch addressing comments.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep, we should be able to fix the issue without opening the bug though, right?

Comment 11

[Michal Zalewski]

Fine by me.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

dveditz, do I need to wait until this goes in on 1.8.1 before checking this in on the trunk?  It's only ("only") a potential null deref.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 307346 [details] [diff] [review]
patch for 1.8.1

approved for 1.8.1.13, a=dveditz

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in, trunk and 1.8.1.

Comment 15

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13pre) Gecko/20080311 BonEcho/2.0.0.13pre
While it did crash with a branch build of 2008-02-02.

Also verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031109 Minefield/3.0b5pre

Comment 16

[Alexander Sack]

doesn't seem to be an issue on 1.8.0 branch. If I am wrong, please request blocking1.8.0.15.

Comment 17

[Daniel Veditz [:dveditz]]

mz links to this fuzzer from his home page now so we don't need to keep this hidden.