Closed Bug 419718 Opened 14 years ago Closed 14 years ago

Crash if <CANVAS> destroyed but drawing context kept

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta5

People

(Reporter: lcamtuf, Assigned: vlad)

References

()

Details

(Keywords: verified1.8.1.13, Whiteboard: [sg:dos] meta; keep in s-g until other vendors fix)

Attachments

(3 files, 1 obsolete file)

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
Component: Security → Layout: Canvas
Product: Firefox → Core
QA Contact: firefox → layout.canvas
Version: 2.0 Branch → Trunk
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.
Assignee: nobody → vladimir
Whiteboard: [sg:nse dos] meta
Youch.  I'll get on this, should be a straightforward fix.
Flags: blocking1.9+
Priority: -- → P2
Flags: tracking1.9+ → blocking1.9+
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #306304 - Flags: review?(pavlov)
Comment on attachment 306304 [details] [diff] [review]
fix

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

in to a temporary
Attachment #306304 - Flags: review?(pavlov) → review+
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.
Good point.  Updated to my patch to do some null checking along the way there.
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.
Attached patch patch for 1.8.1Splinter Review
Patch for 1.8.1.  Very simple, just adds a few null checks; fewer needed here than in 1.9.
Attachment #307346 - Flags: review?(pavlov)
Attachment #307346 - Flags: approval1.8.1.13?
Updated trunk patch addressing comments.
Attachment #306304 - Attachment is obsolete: true
Attachment #307347 - Flags: review?(pavlov)
Whiteboard: [sg:nse dos] meta → [sg:nse dos] meta; keep in s-g until other vendors fix
Yep, we should be able to fix the issue without opening the bug though, right?
Fine by me.
Attachment #307346 - Flags: review?(pavlov) → review+
Attachment #307347 - Flags: review?(pavlov) → review+
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 on attachment 307346 [details] [diff] [review]
patch for 1.8.1

approved for 1.8.1.13, a=dveditz
Attachment #307346 - Flags: approval1.8.1.13? → approval1.8.1.13+
Checked in, trunk and 1.8.1.
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8.1.13
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
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
Status: RESOLVED → VERIFIED
doesn't seem to be an issue on 1.8.0 branch. If I am wrong, please request blocking1.8.0.15.
Flags: blocking1.8.0.15-
Whiteboard: [sg:nse dos] meta; keep in s-g until other vendors fix → [sg:dos] meta; keep in s-g until other vendors fix
mz links to this fuzzer from his home page now so we don't need to keep this hidden.
Group: core-security
You need to log in before you can comment on or make changes to this bug.