Closed Bug 665548 (CVE-2011-2999) Opened 13 years ago Closed 13 years ago

Named frames can shadow window.location sometimes


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

Not set



Tracking Status
firefox5 --- affected
firefox6 --- fixed
firefox7 --- fixed
blocking1.9.2 --- .23+
status1.9.2 --- .23-fixed


(Reporter: bzbarsky, Assigned: mounir)


(Whiteboard: [sg:high])


(3 files, 2 obsolete files)

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.
Mounir, you want to look into fixing this? I think it should be as simple as reordering the location specific code in nsWindowSH::NewResolve().
Assignee: nobody → mounir.lamouri
OS: Mac OS X → All
Hardware: x86 → All
Attached patch Patch v1 (obsolete) — Splinter Review
I moved .location, .content and .onhaschange before frame name resolution to make sure they are not shadowed.
Attachment #540611 - Flags: review?(jst)
Whiteboard: [needs review]
Do we want all those not-shadowed.

function onhashchange() {
register hashchange listener?
Does the patch affect to that behavior?
Summary: Names frames can shadow window.location sometimes → Named frames can shadow window.location sometimes
I'd really like to keep this bug to the location case, since that's the JSPROP_PERMANENT case here, right?
This patch only moves window.location resolution before named frames.
Attachment #540611 - Attachment is obsolete: true
Attachment #540611 - Flags: review?(jst)
Attachment #540703 - Flags: review?(jst)
BTW, how is that security sensitive?
For the same reasons bug 541530 was: this bug allows circumventing Flash player's same-origin policies....
Comment on attachment 540703 [details] [diff] [review]
Make sure window.location isn't shadowed

Attachment #540703 - Flags: review?(jst) → review+
Whiteboard: [needs review]
Attached patch PatchSplinter Review
Attachment #540703 - Attachment is obsolete: true
Attachment #540987 - Flags: checkin+
Pushed to m-c:
Closed: 13 years ago
Resolution: --- → FIXED
Attached patch TestsSplinter Review
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.
Attachment #540987 - Flags: approval-mozilla-aurora?
Whiteboard: [needs branch]
Attachment #540987 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Can we get a 1.9.2 patch? Or is branch unaffected?
Whiteboard: [needs branch] → [sg:high][needs branch]
Target Milestone: --- → mozilla7
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.
blocking1.9.2: --- → .20+
Do we have a specific testcase for this?
(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 :(
Target Milestone: mozilla7 → mozilla6
Attached patch 1.9.2 patchSplinter Review
Attachment #551623 - Flags: review?(jst)
Dan, Christian, are we definitely going to rebuild for this? Who wants to tell release-drivers if the answer is "yes?"
Attachment #551623 - Flags: review?(jst) → review+
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
Flags: in-testsuite?
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 on attachment 551623 [details] [diff] [review]
1.9.2 patch

Approved for, a=dveditz for release-drivers
Attachment #551623 - Flags: approval1.9.2.20+
We decided not to respin 3.6.20 for this, so we won't announce the Fx6 fix until we release 3.6.21.
blocking1.9.2: .20+ → .21+
Whiteboard: [sg:high][needs branch] → [sg:high][needs branch] don't announce until Fx7
qa- as no QA fix verification needed
Whiteboard: [sg:high][needs branch] don't announce until Fx7 → [sg:high][needs branch] don't announce until Fx7 [qa-]
Do we have a CVE id for this issue?
Alias: CVE-2011-2999
Group: core-security
This test never landed.
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.
Attachment #540988 - Flags: feedback?(mounir)
(In reply to Andrew McCreight [:mccr8] from comment #27)
> This test never landed.

Just did:
Whiteboard: [sg:high][needs branch] don't announce until Fx7 [qa-] → [sg:high]
Attachment #540988 - Flags: feedback?(mounir)
Flags: in-testsuite? → in-testsuite+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.