546573 - It's possible to access an outer window's Function constructor (Universal XSS)

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

When an outer window has no inner window yet, it's possible to access the outer
window's Function constructor, which can be used to perform an XSS attack.

Comment 1

[moz_bug_r_a4]

Attachment: testcase

This tries to get cookies for www.mozilla.com.

Comment 2

[Daniel Veditz [:dveditz]]

Note to self: make sure 3rd-party cookies are not blocked when trying to reproduce this.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

I think this is all that's required to fix this bug. Basically, we need to make sure that by the time a window is exposed to script, it has an inner window. I *think* that this is all that's required to do that, but I'm not entirely sure.

This supports a previous argument that I've made that outer windows shouldn't be global objects at all (weird, huh?). At the very least it's extra confusing. At the most, it turns into stuff like this. (The fact that we have *two* Function constructors, one of which is never correct to use is something that we've had trouble with before.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 427444 [details] [diff] [review]
Proposed fix

sr=bzbarsky

Comment 5

[moz_bug_r_a4]

Attachment: testcase 2

Does that patch fix this?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v2

This is the only way I could figure out to EnsureInnerWindow for script that doesn't run for all windows (since we very eagerly create JSObjects for outer windows).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 429648 [details] [diff] [review]
Proposed fix v2

Ok, but maybe we should just eagerly create inner windows and be done with it?  Out of scope here, I know.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/2cf18052b8ee

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Had to back this out... it broke focus mochitests.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

A sample for my reference:

s: moz2-linux-slave2639502 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug430351.html | <iframe xmlns="http://www.w3.org/1999/xhtml"></iframe> in <div xmlns="http://www.w3.org/1999/xhtml" id="parent"><iframe></iframe></div> should not be focusable - Didn't expect [object HTMLIFrameElement], but got it.
39503 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug430351.html | <iframe xmlns="http://www.w3.org/1999/xhtml" tabindex="-1"></iframe> in <div xmlns="http://www.w3.org/1999/xhtml" id="parent"><iframe tabindex="-1"></iframe></div> should not be focusable - Didn't expect [object HTMLIFrameElement], but got it.
39504 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug430351.html | <iframe xmlns="http://www.w3.org/1999/xhtml" tabindex="0"></iframe> in <div xmlns="http://www.w3.org/1999/xhtml" id="parent"><iframe tabindex="0"></iframe></div> should not be focusable - Didn't expect [object HTMLIFrameElement], but got it.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Neil, the test that started failing seems to be bogus: as far as I can tell, the only reason that, before this patch, <iframe>s weren't focusable was that nobody bothered to force an inner window (e.g. by looking up a property on it). It seems, then, like the test that failed is bogus; but I'm not entirely sure... Do you have an opinion?

Comment 12

[Johnny Stenback (:jst)]

I bet bug 546648 is related here, same problem it seems like.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

This fixes the test too. Neil could you review the changes?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/eef17c173aaa

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Backed out... more mochitest failures.

Comment 16

[Damon Sicore (:damons)]

Blake, eta on this?  To be clear:  Not as important as the sg:crits.  Put this one last on your sg: to-be-fixed list.

Comment 17

[Johnny Stenback (:jst)]

Attachment: Updated to tip, and with more test fixes (passes mochi-chrome)

Comment 18

[Johnny Stenback (:jst)]

We may not want the changes to docshell_helpers.js in that diff...

Comment 19

[Johnny Stenback (:jst)]

Attachment: Additional test fix.

This fixes another test failure. With this fix I *think* we're down to one remaining consistent failure, and that looks like this in the logs:

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_500328.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_500328.js | Found an unexpected tab at the end of test run: http://example.com/

But no indication about what actually fails there. I haven't had a chance to run that test locally yet...

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

What's keeping this from landing? Does it affect branches as well? Marked a b3 blocker 10 days ago, no activity since.

Comment 21

[Johnny Stenback (:jst)]

This is not going to make it for beta3, moving to beta4.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/6ea9b217883a

This is actually fixed twice over. I'm going to get rid of CheckWindow in a separate patch. I'll investigate a less threatening patch (one that doesn't break docshell tests) for branches.

Comment 23

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/6ea9b217883a

Comment 24

[Daniel Veditz [:dveditz]]

Time to work on a branch version of this one.

Comment 25

[LegNeato]

sg:high -> punt to next version.

Comment 26

[Daniel Veditz [:dveditz]]

It's the next version--mrbkap will you have time to backport this one now?

Comment 27

[LegNeato]

Punting to the next branch version as it Blake didn't get to this.

Comment 28

[Daniel Veditz [:dveditz]]

This bug is getting very bruised from all the kicking

Comment 29

[LegNeato]

Deferring to a future point release as we have run out of time. If this absolutely needs to be fixed and can land today or tomorrow, please send a note to [e-mail]

Comment 30

[Daniel Veditz [:dveditz]]

Is it even possible to fix this bug on the old branch?

Comment 31

[Daniel Veditz [:dveditz]]

Blake says there's no way to fix this on the 1.9.2 branch