Closed Bug 792405 (CVE-2012-4209) Opened 12 years ago Closed 12 years ago

Frames can shadow |top|


(Core :: DOM: Core & HTML, defect)

15 Branch
Not set



Tracking Status
firefox16 + wontfix
firefox17 + verified
firefox18 + verified
firefox19 --- verified
firefox-esr10 17+ verified


(Reporter: marius.mlynski, Assigned: mccr8)



(Keywords: reporter-external, sec-high, Whiteboard: [adv-track-main17+][adv-track-esr17+])


(4 files, 1 obsolete file)

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.
Attachment #662540 - Attachment mime type: text/plain → text/html
Ever confirmed: true
Keywords: sec-high, sec-vector
Assigning to Andrew (during sec triage).
Assignee: nobody → continuation
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.
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).
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.
Flags: in-testsuite?
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)
Try run looks okay for Linux/Windows
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+
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 on attachment 671468 [details] [diff] [review]
move the resolve code for top up earlier

Attachment #671468 - Flags: sec-approval? → sec-approval+
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
We'll want branch uplift nominations for this to both aurora and beta branches.
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 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+
Oh, I just meant that the infrequency should reduce the riskiness of the patch, not that it reduces the danger of the security problem.
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?
Attachment #671468 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Attached file simple test
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.
Whiteboard: [adv-track-main17+][adv-track-esr17+]
Alias: CVE-2012-4209
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
Keywords: verifyme
QA Contact:
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security
Flags: in-testsuite?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.