Last Comment Bug 792405 - (CVE-2012-4209) Frames can shadow |top|
: Frames can shadow |top|
: sec-high
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 15 Branch
: All All
: -- normal (vote)
: mozilla19
Assigned To: Andrew McCreight [:mccr8]
: Anthony Hughes (:ashughes) [GFX][QA][Mentor]
: Andrew Overholt [:overholt]
Depends on: CVE-2012-3994
  Show dependency treegraph
Reported: 2012-09-19 07:07 PDT by Mariusz Mlynski
Modified: 2014-07-24 13:44 PDT (History)
13 users (show)
abillings: sec‑bounty+
continuation: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

shadow top.location with a frame -- PoC (259 bytes, text/html)
2012-09-19 07:07 PDT, Mariusz Mlynski
no flags Details
move the resolve code for top up earlier, WIP (3.03 KB, patch)
2012-09-20 17:03 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
test, to be checked in later (2.21 KB, patch)
2012-10-15 08:58 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
move the resolve code for top up earlier (3.02 KB, patch)
2012-10-15 09:32 PDT, Andrew McCreight [:mccr8]
mrbkap: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
dveditz: sec‑approval+
Details | Diff | Splinter Review
simple test (384 bytes, text/html)
2012-10-25 17:15 PDT, Andrew McCreight [:mccr8]
no flags Details

Description Mariusz Mlynski 2012-09-19 07:07:15 PDT
Created attachment 662540 [details]
shadow top.location with a frame -- PoC

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.
Comment 1 David Bolter [:davidb] 2012-09-20 13:43:21 PDT
Assigning to Andrew (during sec triage).
Comment 2 Daniel Veditz [:dveditz] 2012-09-20 13:49:00 PDT
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.
Comment 3 Andrew McCreight [:mccr8] 2012-09-20 17:03:13 PDT
Created attachment 663215 [details] [diff] [review]
move the resolve code for top up earlier, WIP

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).
Comment 4 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-28 11:12:16 PDT
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.
Comment 5 Andrew McCreight [:mccr8] 2012-10-15 08:58:34 PDT
Created attachment 671463 [details] [diff] [review]
test, to be checked in later
Comment 6 Andrew McCreight [:mccr8] 2012-10-15 09:32:49 PDT
Created attachment 671468 [details] [diff] [review]
move the resolve code for top up earlier

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?
Comment 7 Andrew McCreight [:mccr8] 2012-10-15 14:00:41 PDT
Try run looks okay for Linux/Windows
Comment 8 Blake Kaplan (:mrbkap) 2012-10-18 11:03:20 PDT
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 9 Andrew McCreight [:mccr8] 2012-10-19 09:51:26 PDT
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 10 Daniel Veditz [:dveditz] 2012-10-19 19:36:08 PDT
Comment on attachment 671468 [details] [diff] [review]
move the resolve code for top up earlier

Comment 11 Andrew McCreight [:mccr8] 2012-10-20 08:14:58 PDT
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-10-20 15:58:51 PDT
Comment 13 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-21 09:54:37 PDT
We'll want branch uplift nominations for this to both aurora and beta branches.
Comment 14 Andrew McCreight [:mccr8] 2012-10-21 11:48:27 PDT
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 15 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-22 11:52:03 PDT
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.
Comment 16 Andrew McCreight [:mccr8] 2012-10-22 13:58:17 PDT
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 17 Andrew McCreight [:mccr8] 2012-10-22 13:59:02 PDT
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.
Comment 18 Andrew McCreight [:mccr8] 2012-10-25 17:13:23 PDT
Comment 19 Andrew McCreight [:mccr8] 2012-10-25 17:15:00 PDT
Created attachment 675376 [details]
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.
Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-15 15:28:39 PST
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

Note You need to log in before you can comment on or make changes to this bug.