Last Comment Bug 571287 - (CVE-2010-1207) same-origin checks incorrect on canvas
: same-origin checks incorrect on canvas
: regression, verified1.9.2
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Vladimir Vukicevic [:vlad] [:vladv]
: 490535 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2010-06-10 11:13 PDT by Vladimir Vukicevic [:vlad] [:vladv]
Modified: 2011-02-01 03:19 PST (History)
9 users (show)
dveditz: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (1.21 KB, text/html)
2010-06-10 11:13 PDT, Vladimir Vukicevic [:vlad] [:vladv]
no flags Details
use cycle collection between DOM & canvas (8.58 KB, patch)
2010-06-10 13:14 PDT, Vladimir Vukicevic [:vlad] [:vladv]
bzbarsky: review+
Details | Diff | Review
part 2, ensure contexts participate in CC (897 bytes, patch)
2010-06-10 16:10 PDT, Vladimir Vukicevic [:vlad] [:vladv]
bzbarsky: review+
Details | Diff | Review
part 3, webgl (4.57 KB, patch)
2010-06-10 17:44 PDT, Vladimir Vukicevic [:vlad] [:vladv]
bzbarsky: review+
Details | Diff | Review
patch for 1.9.2 (6.12 KB, patch)
2010-06-13 12:52 PDT, Vladimir Vukicevic [:vlad] [:vladv]
bzbarsky: review+
dveditz: approval1.9.2.7+
Details | Diff | Review

Description Vladimir Vukicevic [:vlad] [:vladv] 2010-06-10 11:13:54 PDT
Created attachment 450392 [details]

We currently have a notion of a "write-only" canvas, to not allow reading data after an image or other non-same-origin content has been rendered into it.  However, that flag lives on the canvas, and not the context; the context can outlive the canvas.

We do the check here:

but only if we have a mCanvasElement -- which can become null if that element is GC'd.  In that case as part of the destruction process, we call SetCanvasElement with nsnull, and the context doesn't keep a strong reference to the canvas.

The right fix seems to be to keep the element alive for the lifetime of the context: you can then reattach it to the dom via ctx.canvas, whereas once you orphan a context you can't ever attach it to a canvas.  The other potential fix is to move the origin-clean flag to the context itself.

The first is probably the right solution, patch coming up.
Comment 1 Vladimir Vukicevic [:vlad] [:vladv] 2010-06-10 12:41:30 PDT
This was introduced in bug 502715 (which I reviewed, alas); previous to that, we disallowed GetImageData if mCanvasElement was null.  The good news is that because of that, this problem doesn't exist in 1.9.1.
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2010-06-10 13:14:49 PDT
Created attachment 450426 [details] [diff] [review]
use cycle collection between DOM & canvas

Ok, here's a fix.  This sets up a CC relationship between the DOM element and the 2D canvas context.  I'm about to do the same thing for WebGL in a separate patch (and add a check to make sure that the context is a CC participant), but this patch should also apply to 1.9.2.
Comment 3 Boris Zbarsky [:bz] 2010-06-10 14:01:56 PDT
Comment on attachment 450426 [details] [diff] [review]
use cycle collection between DOM & canvas

This looks fine, but you could just us an nsRefPtr<nsHTMLCanvasElement> member and use ambiguous CC stuff for it and avoid the extra method, right?  Either way on this.

Either way you don't need the .get() call in GetCanvas.

No need to explicitly null out mCurrentContext in the element dtor; the nsCOMPtr dtr is about to null it out, no?
Comment 4 Vladimir Vukicevic [:vlad] [:vladv] 2010-06-10 16:10:45 PDT
Created attachment 450490 [details] [diff] [review]
part 2, ensure contexts participate in CC

Thanks, will make those fixes/changes.

This is a small part 2, to ensure that contexts participate in CC.  Part 3 for webgl coming up late tonight.

I'll also put together a patch for 1.9.2 then as well.

I could use some suggestions on how to test this -- maybe my original test case and the assumption that if it doesn't happen in 100 iterations it's probably fine? And also how to land it -- the problem would be fairly obvious for anyone looking at the patches, though I guess there's enough CC goop in here that I can just call it "add cycle collector support to canvas".
Comment 5 Vladimir Vukicevic [:vlad] [:vladv] 2010-06-10 17:44:53 PDT
Created attachment 450538 [details] [diff] [review]
part 3, webgl

Same thing as the 2d case, but for webgl.
Comment 6 Daniel Veditz [:dveditz] 2010-06-11 12:30:42 PDT
Doesn't seem to work in 1.9.2, getImageData returns an empty object. Do we know when this started happening? If it's not a change local to canvas and it's some higher-level change (like a wrapper) we should look around for other similar regressions.
Comment 7 Vladimir Vukicevic [:vlad] [:vladv] 2010-06-12 16:41:28 PDT
Yep, see comment #1 for when it started.  Canvas-only change, bug 502715.

It definitely affects 1.9.2, just tested it; the empty object thing looks like a fluke in how toSource works there.  I just changed how the data is displayed.  The testcase should basically always throw (which would cause the counter to keep incrementing), it should never return an object.

I've got a 1.9.2 patch that I'm testing on the try server.
Comment 8 Vladimir Vukicevic [:vlad] [:vladv] 2010-06-13 12:52:01 PDT
Created attachment 450946 [details] [diff] [review]
patch for 1.9.2

Rolled up patch for 1.9.2.  I didn't patch WebGL here, since it'll just get disallowed via the getContext check, and it's broken/not to be used on 1.9.2 anyway.  (Defiintely pref'd off, might not even be compiled in; can't remember.)
Comment 9 Vladimir Vukicevic [:vlad] [:vladv] 2010-06-15 14:40:02 PDT
Pushed to m-c:

Need to come up with a test for the testsuite, but I don't want to land that until after 1.9.2 lands/ships anyway.
Comment 10 Daniel Veditz [:dveditz] 2010-06-16 10:58:05 PDT
Comment on attachment 450946 [details] [diff] [review]
patch for 1.9.2

Approved for, a=dveditz for release-drivers
Comment 11 Vladimir Vukicevic [:vlad] [:vladv] 2010-06-21 14:36:45 PDT

(Accidentally had wrong bug # in push)
Comment 12 Al Billings [:abillings] 2010-07-16 15:16:58 PDT
How does the attached testcase work? It's pretty non-intuitive and I don't see any difference in behavior between and on XP.
Comment 13 Boris Zbarsky [:bz] 2010-07-16 21:04:47 PDT
When you click "Go" in it eventually stops with "failed after 100 count"?
Comment 14 Al Billings [:abillings] 2010-07-19 16:57:06 PDT
All right, I see that it stops on and not on after a little while. Verified for 1.9.2 with  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20100713 Firefox/3.6.7 (.NET CLR 3.5.30729).
Comment 15 :Ms2ger 2011-02-01 03:19:26 PST
*** Bug 490535 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.