Bug 665548 (CVE-2011-2999)

Named frames can shadow window.location sometimes

RESOLVED FIXED in Firefox 6

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bz, Assigned: mounir)

Tracking

Trunk
mozilla6
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox5 affected, firefox6 fixed, firefox7 fixed, blocking1.9.2 .23+, status1.9.2 .23-fixed)

Details

(Whiteboard: [sg:high])

Attachments

(3 attachments, 2 obsolete attachments)

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
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 2

6 years ago
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.
Attachment #540611 - Flags: review?(jst)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Whiteboard: [needs review]

Comment 3

6 years ago
Do we want all those not-shadowed.

does 
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?
(Assignee)

Comment 5

6 years ago
Created attachment 540703 [details] [diff] [review]
Make sure window.location isn't shadowed

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)
(Assignee)

Comment 6

6 years ago
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

r=jst!
Attachment #540703 - Flags: review?(jst) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [needs review]
(Assignee)

Comment 9

6 years ago
Created attachment 540987 [details] [diff] [review]
Patch

r=jst
Attachment #540703 - Attachment is obsolete: true
Attachment #540987 - Flags: checkin+
(Assignee)

Comment 10

6 years ago
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/433a6c04a18d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

6 years ago
Created attachment 540988 [details] [diff] [review]
Tests

r=jst
(Assignee)

Updated

6 years ago
status-firefox5: --- → affected
status-firefox6: --- → affected
status-firefox7: --- → fixed
(Assignee)

Comment 12

6 years ago
Comment on attachment 540987 [details] [diff] [review]
Patch

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?
(Assignee)

Updated

6 years ago
Whiteboard: [needs branch]

Updated

6 years ago
Attachment #540987 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 13

6 years ago
Pushed in Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/ae5547ff8e67
status-firefox6: affected → fixed

Comment 14

6 years ago
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 1.9.2.20 to avoid exposing our users to this when we release Firefox 6.
blocking1.9.2: --- → .20+
status1.9.2: --- → wanted
Do we have a specific testcase for this?
(Assignee)

Comment 17

6 years ago
(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 1.9.2.20 in a few minutes. Sorry if I didn't do that before, I obviously missed Christian's comment :(
Target Milestone: mozilla7 → mozilla6
(Assignee)

Comment 18

6 years ago
Created attachment 551623 [details] [diff] [review]
1.9.2 patch
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?"

Updated

6 years ago
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 1.9.2.20, a=dveditz for release-drivers
Attachment #551623 - Flags: approval1.9.2.20+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/535985445918
status1.9.2: wanted → .20-fixed
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+
status1.9.2: .20-fixed → .21-fixed
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-]

Comment 26

6 years ago
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]
Tests

Can I go ahead and land this, Mounir?  With the Makefile unbitrotted, the test passes locally for me.
Attachment #540988 - Flags: feedback?(mounir)
(Assignee)

Comment 29

5 years ago
(In reply to Andrew McCreight [:mccr8] from comment #27)
> This test never landed.

Just did:
https://hg.mozilla.org/integration/mozilla-inbound/rev/938c4afc92b9
(Assignee)

Updated

5 years ago
Whiteboard: [sg:high][needs branch] don't announce until Fx7 [qa-] → [sg:high]
(Assignee)

Updated

5 years ago
Attachment #540988 - Flags: feedback?(mounir)
Thanks.
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/938c4afc92b9
You need to log in before you can comment on or make changes to this bug.