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)
Core
Graphics: Canvas2D
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)
1.21 KB,
text/html
|
Details | |
8.58 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
897 bytes,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
4.57 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
6.12 KB,
patch
|
bzbarsky
:
review+
dveditz
:
approval1.9.2.7+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
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: ? → ---
Assignee | ||
Comment 2•15 years ago
|
||
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 3•15 years ago
|
||
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+
Assignee | ||
Comment 4•15 years ago
|
||
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)
Assignee | ||
Comment 5•15 years ago
|
||
Same thing as the 2d case, but for webgl.
Attachment #450538 -
Flags: review?(bzbarsky)
Updated•15 years ago
|
Attachment #450490 -
Flags: review?(bzbarsky) → review+
Updated•15 years ago
|
Attachment #450538 -
Flags: review?(bzbarsky) → review+
Comment 6•15 years ago
|
||
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: ? → ---
status1.9.2:
--- → unaffected
Keywords: regression
Whiteboard: [sg:crit?] → [sg:high]
Updated•15 years ago
|
Keywords: regressionwindow-wanted
Assignee | ||
Comment 7•15 years ago
|
||
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.
Keywords: regressionwindow-wanted
Assignee | ||
Updated•15 years ago
|
blocking1.9.2: --- → ?
status1.9.2:
? → ---
Assignee | ||
Comment 8•15 years ago
|
||
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)
Updated•15 years ago
|
status1.9.1:
--- → unaffected
status1.9.2:
--- → wanted
Updated•15 years ago
|
Attachment #450946 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•15 years ago
|
Attachment #450946 -
Flags: approval1.9.2.6?
Updated•15 years ago
|
Whiteboard: [sg:high] → [sg:high][critsmash-high:patch]
Assignee | ||
Comment 9•15 years ago
|
||
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
Updated•15 years ago
|
Flags: in-testsuite?
Comment 10•15 years ago
|
||
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+
Assignee | ||
Comment 11•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e9c2600a5d01
(Accidentally had wrong bug # in push)
Updated•15 years ago
|
Alias: CVE-2010-1207
Comment 12•15 years ago
|
||
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•15 years ago
|
||
When you click "Go" in 1.9.2.6 it eventually stops with "failed after 100 count"?
Comment 14•15 years ago
|
||
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
Updated•14 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•