571287 - (CVE-2010-1207) same-origin checks incorrect on canvas

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

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

Attachment: testcase

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:

  http://hg.mozilla.org/mozilla-central/file/7b15545cf9aa/content/canvas/src/nsCanvasRenderingContext2D.cpp#l3506

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] (needinfo me, slow to respond)]

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] (needinfo me, slow to respond)]

Attachment: 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 [:bzbarsky]]

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] (needinfo me, slow to respond)]

Attachment: 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] (needinfo me, slow to respond)]

Attachment: part 3, webgl

Same thing as the 2d case, but for webgl.

Comment 6

[Daniel Veditz [:dveditz]]

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] (needinfo me, slow to respond)]

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] (needinfo me, slow to respond)]

Attachment: 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] (needinfo me, slow to respond)]

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/94305a24dfdc

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]]

Comment on attachment 450946 [details] [diff] [review]
patch for 1.9.2

Approved for 1.9.2.6, a=dveditz for release-drivers

Comment 11

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e9c2600a5d01

(Accidentally had wrong bug # in push)

Comment 12

[Al Billings [:abillings - ex-MoCo]]

How does the attached testcase work? It's pretty non-intuitive and I don't see any difference in behavior between 1.9.2.7 and 1.9.2.6 on XP.

Comment 13

[Boris Zbarsky [:bzbarsky]]

When you click "Go" in 1.9.2.6 it eventually stops with "failed after 100 count"?

Comment 14

[Al Billings [:abillings - ex-MoCo]]

All right, I see that it stops on 1.9.2.6 and not on 1.9.2.7 after a little while. Verified for 1.9.2 with  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.7) Gecko/20100713 Firefox/3.6.7 (.NET CLR 3.5.30729).