665548 - (CVE-2011-2999) Named frames can shadow window.location sometimes

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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)]

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)]

Attachment: Patch v1

I moved .location, .content and .onhaschange before frame name resolution to make sure they are not shadowed.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Do we want all those not-shadowed.

does 
function onhashchange() {
}
register hashchange listener?
Does the patch affect to that behavior?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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)]

Attachment: Make sure window.location isn't shadowed

This patch only moves window.location resolution before named frames.

Comment 6

[Mounir Lamouri (:mounir)]

BTW, how is that security sensitive?

Comment 7

[Boris Zbarsky [:bzbarsky]]

For the same reasons bug 541530 was: this bug allows circumventing Flash player's same-origin policies....

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 540703 [details] [diff] [review]
Make sure window.location isn't shadowed

r=jst!

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Patch

r=jst

Comment 10

[Mounir Lamouri (:mounir)]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/433a6c04a18d

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Tests

r=jst

Comment 12

[Mounir Lamouri (:mounir)]

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.

Comment 13

[Mounir Lamouri (:mounir)]

Pushed in Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/ae5547ff8e67

Comment 14

[LegNeato]

Can we get a 1.9.2 patch? Or is branch unaffected?

Comment 15

[Daniel Veditz [:dveditz]]

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.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Do we have a specific testcase for this?

Comment 17

[Mounir Lamouri (:mounir)]

(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 :(

Comment 18

[Mounir Lamouri (:mounir)]

Attachment: 1.9.2 patch

Comment 19

[Al Billings [:abillings - ex-MoCo]]

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]]

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 - ex-MoCo]]

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]]

Comment on attachment 551623 [details] [diff] [review]
1.9.2 patch

Approved for 1.9.2.20, a=dveditz for release-drivers

Comment 23

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/535985445918

Comment 24

[Daniel Veditz [:dveditz]]

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

[u279076]

qa- as no QA fix verification needed

Comment 26

[Huzaifa Sidhpurwala]

Do we have a CVE id for this issue?

Comment 27

[Andrew McCreight [:mccr8]]

This test never landed.

Comment 28

[Andrew McCreight [:mccr8]]

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.

Comment 29

[Mounir Lamouri (:mounir)]

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

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

Comment 30

[Andrew McCreight [:mccr8]]

Thanks.

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/938c4afc92b9