Last Comment Bug 665548 - (CVE-2011-2999) Named frames can shadow window.location sometimes
: Named frames can shadow window.location sometimes
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Mounir Lamouri (:mounir)
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2011-06-20 08:12 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-10-16 01:25 PDT (History)
16 users (show)
continuation: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1 (8.67 KB, patch)
2011-06-20 15:58 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Make sure window.location isn't shadowed (6.01 KB, patch)
2011-06-21 02:05 PDT, Mounir Lamouri (:mounir)
jst: review+
Details | Diff | Splinter Review
Patch (4.28 KB, patch)
2011-06-22 01:03 PDT, Mounir Lamouri (:mounir)
jst: approval‑mozilla‑aurora+
mounir: checkin+
Details | Diff | Splinter Review
Tests (1.59 KB, patch)
2011-06-22 01:05 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
1.9.2 patch (5.08 KB, patch)
2011-08-08 17:03 PDT, Mounir Lamouri (:mounir)
jst: review+
dveditz: approval1.9.2.20+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2011-06-20 08:12:39 PDT
location is a non-configurable property, so if you look up window.location before there is a <frame name="location"> you will always get the right window.location after that.  But if you look it up for the first time _after_ such a frame exists, you will get the Window for the frame.

Effectively, the fix for bug 541530 can be circumvented by this means.
Comment 1 Johnny Stenback (:jst, 2011-06-20 15:01:42 PDT
Mounir, you want to look into fixing this? I think it should be as simple as reordering the location specific code in nsWindowSH::NewResolve().
Comment 2 Mounir Lamouri (:mounir) 2011-06-20 15:58:43 PDT
Created attachment 540611 [details] [diff] [review]
Patch v1

I moved .location, .content and .onhaschange before frame name resolution to make sure they are not shadowed.
Comment 3 Olli Pettay [:smaug] 2011-06-20 16:05:25 PDT
Do we want all those not-shadowed.

function onhashchange() {
register hashchange listener?
Does the patch affect to that behavior?
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-06-20 21:18:40 PDT
I'd really like to keep this bug to the location case, since that's the JSPROP_PERMANENT case here, right?
Comment 5 Mounir Lamouri (:mounir) 2011-06-21 02:05:50 PDT
Created attachment 540703 [details] [diff] [review]
Make sure window.location isn't shadowed

This patch only moves window.location resolution before named frames.
Comment 6 Mounir Lamouri (:mounir) 2011-06-21 02:07:39 PDT
BTW, how is that security sensitive?
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2011-06-21 06:56:53 PDT
For the same reasons bug 541530 was: this bug allows circumventing Flash player's same-origin policies....
Comment 8 Johnny Stenback (:jst, 2011-06-21 11:25:38 PDT
Comment on attachment 540703 [details] [diff] [review]
Make sure window.location isn't shadowed

Comment 9 Mounir Lamouri (:mounir) 2011-06-22 01:03:45 PDT
Created attachment 540987 [details] [diff] [review]

Comment 10 Mounir Lamouri (:mounir) 2011-06-22 01:04:08 PDT
Pushed to m-c:
Comment 11 Mounir Lamouri (:mounir) 2011-06-22 01:05:19 PDT
Created attachment 540988 [details] [diff] [review]

Comment 12 Mounir Lamouri (:mounir) 2011-06-22 01:13:53 PDT
Comment on attachment 540987 [details] [diff] [review]

Only asking approval for Aurora given that I suppose the Beta channel is the same as the release channel.
Comment 13 Mounir Lamouri (:mounir) 2011-06-29 06:16:43 PDT
Pushed in Aurora:
Comment 14 christian 2011-07-06 10:33:00 PDT
Can we get a 1.9.2 patch? Or is branch unaffected?
Comment 15 Daniel Veditz [:dveditz] 2011-08-08 14:41:30 PDT
This bug affects the 1.9.2 branch. Even though we've done "final" builds we may need to take this in to avoid exposing our users to this when we release Firefox 6.
Comment 16 Al Billings [:abillings] 2011-08-08 16:07:30 PDT
Do we have a specific testcase for this?
Comment 17 Mounir Lamouri (:mounir) 2011-08-08 16:50:06 PDT
(In reply to Al Billings [:abillings] from comment #16)
> Do we have a specific testcase for this?

data:text/html,<iframe name='location'></iframe><script>alert(window.location);</script>

should show "data:text/html,<iframe name='location'></iframe><script>alert(window.location);</script>" instead of "[object Window]" in the alert box.

I'm going to attach a patch for in a few minutes. Sorry if I didn't do that before, I obviously missed Christian's comment :(
Comment 18 Mounir Lamouri (:mounir) 2011-08-08 17:03:00 PDT
Created attachment 551623 [details] [diff] [review]
1.9.2 patch
Comment 19 Al Billings [:abillings] 2011-08-08 17:03:36 PDT
Dan, Christian, are we definitely going to rebuild for this? Who wants to tell release-drivers if the answer is "yes?"
Comment 20 Daniel Veditz [:dveditz] 2011-08-08 17:19:28 PDT
It looks like the testcase was not yet checked in to m-c/aurora/beta so we could possibly get away with waiting for the next 3.6.x release. But it's a simple patch (moves a block earlier in the file) and we haven't gone to beta yet. I'd be happier putting this in 3.6.20
Comment 21 Al Billings [:abillings] 2011-08-08 17:27:14 PDT
FYI, This will delay the beta.

If we check in and go to build "now," QA will likely not get builds until tomorrow. Our contract resources are time shifted 9 or 10 hours (in Europe) so we would have to have builds in the next six or seven hours for them to test. This means that we'd need to run automation tomorrow in MV and then have staff locally run the manual tests that contractors normally run, as well as test new snippets for betatest (and snippets must be generated for .19 and .18 both given the unique nature of .19).

This likely means a Wednesday Beta, assuming nothing goes wrong.
Comment 22 Daniel Veditz [:dveditz] 2011-08-08 17:51:30 PDT
Comment on attachment 551623 [details] [diff] [review]
1.9.2 patch

Approved for, a=dveditz for release-drivers
Comment 23 Daniel Veditz [:dveditz] 2011-08-08 18:29:19 PDT
Comment 24 Daniel Veditz [:dveditz] 2011-08-09 15:26:26 PDT
We decided not to respin 3.6.20 for this, so we won't announce the Fx6 fix until we release 3.6.21.
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 15:22:45 PDT
qa- as no QA fix verification needed
Comment 26 Huzaifa Sidhpurwala 2011-09-27 19:07:35 PDT
Do we have a CVE id for this issue?
Comment 27 Andrew McCreight [:mccr8] 2012-10-15 07:41:10 PDT
This test never landed.
Comment 28 Andrew McCreight [:mccr8] 2012-10-15 07:48:05 PDT
Comment on attachment 540988 [details] [diff] [review]

Can I go ahead and land this, Mounir?  With the Makefile unbitrotted, the test passes locally for me.
Comment 29 Mounir Lamouri (:mounir) 2012-10-15 07:49:20 PDT
(In reply to Andrew McCreight [:mccr8] from comment #27)
> This test never landed.

Just did:
Comment 30 Andrew McCreight [:mccr8] 2012-10-15 07:50:26 PDT
Comment 31 Ed Morley [:emorley] 2012-10-16 01:25:52 PDT

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