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.

Assigning to Andrew (during sec triage).

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.

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?

Try run looks okay for Linux/Windows https://tbpl.mozilla.org/?tree=Try&rev=fc8fadd806bf

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?

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".

Comment on attachment 671468 [details] [diff] [review] move the resolve code for top up earlier sec-approval+

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

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.

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

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.

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.

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