Closed
Bug 792405
(CVE-2012-4209)
Opened 12 years ago
Closed 12 years ago
Frames can shadow |top|
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
People
(Reporter: marius.mlynski, Assigned: mccr8)
References
Details
(Keywords: reporter-external, sec-high, Whiteboard: [adv-track-main17+][adv-track-esr17+])
Attachments
(4 files, 1 obsolete file)
259 bytes,
text/html
|
Details | |
2.21 KB,
patch
|
Details | Diff | Splinter Review | |
3.02 KB,
patch
|
mrbkap
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
akeybl
:
approval-mozilla-esr10+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
384 bytes,
text/html
|
Details |
It is possible to shadow |top| with a frame whose name attribute's value is set to "top". As a result, plugins accessing top.location, such as Adobe Flash Player, may read an arbitrary URL.
The fix to bug 765527 doesn't seem to cover this case.
Reporter | ||
Updated•12 years ago
|
Attachment #662540 -
Attachment mime type: text/plain → text/html
Depends on: CVE-2012-3994
Updated•12 years ago
|
Comment 2•12 years ago
|
||
We're not saying the bug is in Flash so I don't think sec-vector is appropriate.
It'd be nice to fix this in Firefox 16 so we can announce the flaw along with the related bug 765527, rather than trickling these out over multiple releases. But given the time-to-release that may not be realistic.
status-firefox-esr10:
--- → affected
status-firefox16:
--- → affected
status-firefox17:
--- → affected
status-firefox18:
--- → affected
tracking-firefox-esr10:
--- → ?
tracking-firefox16:
--- → ?
tracking-firefox17:
--- → +
tracking-firefox18:
--- → +
Keywords: sec-vector
Updated•12 years ago
|
Assignee | ||
Comment 3•12 years ago
|
||
I talked to jst about this, and he said the problem is that we look up iframes before we hit this hardcoded top lookup.
I guess we don't care about iframes overriding other non-location properties like .document and .navigator? I guess top isn't a meaningful property on documents (which is the other place .location is protected).
Updated•12 years ago
|
Comment 4•12 years ago
|
||
Too late in 16 cycle to risk landing this with no further betas where a backout could happen if needed, we're OK with having this in one more round of announcements but getting this into 17 early would be very helpful so we have time to let it bake.
Assignee | ||
Comment 5•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 6•12 years ago
|
||
This patch moves the resolve code earlier. I confirmed by inspection that there's nothing earlier that should capture top.
The equivalent code for location has this pre-hg comment at the top:
// This must be done even if we're just getting the value of
// window.location (i.e. no checking flags & JSRESOLVE_ASSIGNING
// here) since we must define window.location to prevent the
// getter from being overriden (for security reasons).
Is this going to be a problem for top, too, or is this something the JS engine changes in bug 756719 should cover?
Attachment #663215 -
Attachment is obsolete: true
Attachment #671468 -
Flags: review?(mrbkap)
Assignee | ||
Comment 7•12 years ago
|
||
Try run looks okay for Linux/Windows
https://tbpl.mozilla.org/?tree=Try&rev=fc8fadd806bf
Comment 8•12 years ago
|
||
Comment on attachment 671468 [details] [diff] [review]
move the resolve code for top up earlier
Review of attachment 671468 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/base/nsDOMClassInfo.cpp
@@ +7188,5 @@
> + NS_ENSURE_SUCCESS(rv, rv);
> +
> + // Hold on to the top window object as a global property so we
> + // don't need to worry about losing expando properties etc.
> + if (!::JS_DefinePropertyById(cx, obj, id, v, nullptr, nullptr,
While you're moving this, nuke the :: please?
Attachment #671468 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Comment on attachment 671468 [details] [diff] [review]
move the resolve code for top up earlier
[Security approval request comment]
How easily can the security issue be deduced from the patch? The patch just moves a chunk of code (we check for .top before we check for iframes), but this is related to the location stuff that has been in the news, and is basically a combination of bug 665548 (which was fixed a year ago) and bug 665548 (which was fixed in 16), so if anybody can stop their eyes from bleeding looking at nsWindowSH::NewResolve it is probably not that difficult to work out. Of course, the problem can just be figured out from known problems, I'm sure.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Kind of, I think it is publically known that the top property is important to security of Flash.
Which older supported branches are affected by this flaw? Probably all of them.
If not all supported branches, which bug introduced the flaw?
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Shouldn't be a problem. It is just moving code around.
How likely is this patch to cause regressions; how much testing does it need? This is a web-exposed change, so there is some risk, but this should only cause problems on pages with iframes named "top".
Attachment #671468 -
Flags: sec-approval?
Comment 10•12 years ago
|
||
Comment on attachment 671468 [details] [diff] [review]
move the resolve code for top up earlier
sec-approval+
Attachment #671468 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 11•12 years ago
|
||
Comment 12•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
status-firefox19:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 13•12 years ago
|
||
We'll want branch uplift nominations for this to both aurora and beta branches.
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 671468 [details] [diff] [review]
move the resolve code for top up earlier
[Approval Request Comment]
Bug caused by (feature/regressing bug #): old
User impact if declined: security problems
Testing completed (on m-c, etc.): not much, just landed on m-c recently
Risk to taking this patch (and alternatives if risky): this changed web-exposed behavior, but in a fairly obscure case (setting properties on iframes named "top"), and other browsers probably don't allow this, so hopefully it isn't too bad.
String or UUID changes made by this patch: none
Attachment #671468 -
Flags: approval-mozilla-beta?
Attachment #671468 -
Flags: approval-mozilla-aurora?
Comment 15•12 years ago
|
||
Comment on attachment 671468 [details] [diff] [review]
move the resolve code for top up earlier
Given the issues with location lately, it seems worth taking this up to branches even if it's not a frequent use case.
Attachment #671468 -
Flags: approval-mozilla-beta?
Attachment #671468 -
Flags: approval-mozilla-beta+
Attachment #671468 -
Flags: approval-mozilla-aurora?
Attachment #671468 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 16•12 years ago
|
||
Oh, I just meant that the infrequency should reduce the riskiness of the patch, not that it reduces the danger of the security problem.
https://hg.mozilla.org/releases/mozilla-aurora/rev/0dfd8d88e420
https://hg.mozilla.org/releases/mozilla-beta/rev/5357284cc014
Assignee | ||
Comment 17•12 years ago
|
||
Comment on attachment 671468 [details] [diff] [review]
move the resolve code for top up earlier
See above. Sorry, I forgot this had to go on ESR10, too.
Attachment #671468 -
Flags: approval-mozilla-esr10?
Updated•12 years ago
|
Attachment #671468 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Assignee | ||
Comment 18•12 years ago
|
||
Keywords: verifyme
Assignee | ||
Comment 19•12 years ago
|
||
Here's a simple test that can be used for verification. It pops up an alert that says "should be true" twice, and it should say "true" after both of them.
Updated•12 years ago
|
Whiteboard: [adv-track-main17+][adv-track-esr17+]
Updated•12 years ago
|
Alias: CVE-2012-4209
Comment 20•12 years ago
|
||
Verified fixed using Andrew's simple testcase for:
* Firefox 19.0a1 2012-11-15
* Firefox 18.0a2 2012-11-15
* Firefox 17.0b6
* Firefox 10.0.11esr candidate build1
Updated•12 years ago
|
Flags: sec-bounty?
Updated•12 years ago
|
Flags: sec-bounty? → sec-bounty+
Updated•12 years ago
|
Group: core-security
Assignee | ||
Updated•7 years ago
|
Flags: in-testsuite?
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
Updated•6 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•