Closed Bug 571287 (CVE-2010-1207) Opened 15 years ago Closed 15 years ago

same-origin checks incorrect on canvas

Categories

(Core :: Graphics: Canvas2D, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta1+
blocking1.9.2 --- .7+
status1.9.2 --- .7-fixed
status1.9.1 --- unaffected

People

(Reporter: vlad, Assigned: vlad)

References

Details

(Keywords: regression, verified1.9.2, Whiteboard: [sg:high][critsmash-high:patch])

Attachments

(5 files)

Attached file 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.
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.
blocking1.9.1: ? → ---
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.
Attachment #450426 - Flags: review?(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?
Attachment #450426 - Flags: review?(bzbarsky) → review+
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".
Attachment #450490 - Flags: review?(bzbarsky)
Attached patch part 3, webglSplinter Review
Same thing as the 2d case, but for webgl.
Attachment #450538 - Flags: review?(bzbarsky)
Attachment #450490 - Flags: review?(bzbarsky) → review+
Attachment #450538 - Flags: review?(bzbarsky) → review+
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.
blocking1.9.2: ? → ---
Keywords: regression
Whiteboard: [sg:crit?] → [sg:high]
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.
blocking1.9.2: --- → ?
status1.9.2: ? → ---
Attached patch patch for 1.9.2Splinter Review
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.)
Attachment #450946 - Flags: review?(bzbarsky)
blocking1.9.2: ? → .6+
Attachment #450946 - Flags: review?(bzbarsky) → review+
Whiteboard: [sg:high] → [sg:high][critsmash-high:patch]
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.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Comment on attachment 450946 [details] [diff] [review] patch for 1.9.2 Approved for 1.9.2.6, a=dveditz for release-drivers
Attachment #450946 - Flags: approval1.9.2.6? → approval1.9.2.6+
Alias: CVE-2010-1207
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.
When you click "Go" in 1.9.2.6 it eventually stops with "failed after 100 count"?
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).
Keywords: verified1.9.2
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: