Crash if <CANVAS> destroyed but drawing context kept

VERIFIED FIXED in mozilla1.9beta5

Status

()

Core
Canvas: 2D
P2
minor
VERIFIED FIXED
10 years ago
4 years ago

People

(Reporter: Michal Zalewski, Assigned: vlad)

Tracking

({verified1.8.1.13})

Trunk
mozilla1.9beta5
verified1.8.1.13
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.0.next -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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

Updated

10 years ago
Flags: tracking1.9+ → blocking1.9+
Created attachment 306304 [details] [diff] [review]
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.
Attachment #306304 - Flags: review?(pavlov)

Comment 4

10 years ago
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.
Created attachment 307336 [details]
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.
Created attachment 307346 [details] [diff] [review]
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.
Attachment #307346 - Flags: review?(pavlov)
Attachment #307346 - Flags: approval1.8.1.13?
Created attachment 307347 [details] [diff] [review]
updated trunk patch with comments

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?
(Reporter)

Comment 11

10 years ago
Fine by me.

Updated

10 years ago
Attachment #307346 - Flags: review?(pavlov) → review+

Updated

10 years ago
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
Last Resolved: 10 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
Keywords: fixed1.8.1.13 → verified1.8.1.13

Comment 16

10 years ago
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.