It seems that this is a regression from bug 918345. It's possible to call some methods on a cross-origin window. For example: - it's possible to get Selection from a target page. - it's possible to open a popup on a target page. - it's possible to spoof document.referrer.

This is used to show document.referrer.

Investigating.

Looks like we're getting our inners/outers wrong. Does the quickstubs unwrapping code do an UncheckedUnwrap somewhere?

Assigning to bz per comment 5, adjust as desired.

Ping me if you need anything bz.

So getSelection started working because instead of computing this (which outerizes), we actually use the right inner as this for bareword method calls now. That means that getSelection lands in nsGlobalWindow code, which blindly forwards to outer. We should really fix this in some principled way... Maybe by doing both computethis (which will give us the outer) and getting the global of the function and then throwing a security exception if they're not same-origin? Or maybe forwarding to outer should only work from the current inner, at least for DOM-accessible stuff? It's weird to do getSelection on some inner and get the selection for whatever the current iiner is. Testcase 2 is the same issue but with prompt(). Testcase 3 I'm still looking into, since setTimeout _does_ try to check whether it has the right inner, last I checked.

Ah, Testcase 3 is just setting a timeout on a non-current inner, which then does a location set. And location sets are allowed cross-origin and end up using a referrer based on the JSContext or something, which is now associated with the new inner. So that part is basically bug 936129, I think.

But the upshot is that we either need to change bindings to do something different here or we need to audit everything that forwards to outer.

Yeah, I think that we should rewrite FORWARD_TO_OUTER to bail if the inner is not the current inner. The current setup is just asking for trouble. Any internal consumers that this change breaks can just be rewritten to explicitly grab the outer.

Peter is looking at this.

I've started by changing just FORWARD_TO_OUTER_OR_THROW (and that's green on try). Am now looking at all the WebIDL methods/getters/setters and found a couple that use FORWARD_TO_OUTER that I'll change next. That should get us back to where we were. I'll see if it's doable to audit all the others.

Variants of this in bug 940085 affect 24 to 28, so I'm just going to mark those as affected here, where the patch will likely live (bz says the same fix should work).

This just moves code around, preparation for next patch.

Green on try. The assertion fix is a result from removing unused code in GetPrivateRoot. I'm still working on automating the testcases, almost done.

Comment on attachment 8346631 [details] [diff] [review] Move GetTopImpl v1 r=me

Comment on attachment 8346632 [details] [diff] [review] v1 r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef46d8ff19e https://hg.mozilla.org/integration/mozilla-inbound/rev/b67b9a6a65fa

landed on central https://hg.mozilla.org/mozilla-central/rev/b67b9a6a65fa https://hg.mozilla.org/mozilla-central/rev/3ef46d8ff19e

Comment on attachment 8346632 [details] [diff] [review] v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or IDL/UUID changes made by this patch: [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 918345 User impact if declined: security bug, this allows code from a different origin to access web page data Testing completed (on m-c, etc.): landed on m-c, still working on the automated testcase, but we should probably land that separately anyway as it shows the problem pretty clearly Risk to taking this patch (and alternatives if risky): this might break some code that relies on calling APIs on non-current inner windows, but that should be rare. I went through a lot of callers and checked that they either worked or I fixed the API involved. I'm fairly confident we won't see breakage. String or IDL/UUID changes made by this patch: none

I don't think this affects anything before 27 (where bug 918345 landed). Let me know if I'm missing something.

It's affecting earlier releases due to bug 936056; see bug 940085. But I think the right fix there is to backport the much simpler patch for bug 936056.

This should have gotten sec-approval before landing (https://wiki.mozilla.org/Security/Bug_Approval_Process).

(In reply to Boris Zbarsky [:bz] from comment #24) > It's affecting earlier releases due to bug 936056; see bug 940085. > > But I think the right fix there is to backport the much simpler patch for > bug 936056. Yeah, I've sort of considered the problems with earlier branches as a result of bug 936056, not this bug. (In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #25) > This should have gotten sec-approval before landing > (https://wiki.mozilla.org/Security/Bug_Approval_Process). Hrmpf, yes, sorry about that. Old habits die hard.

Comment on attachment 8346632 [details] [diff] [review] v1 well, it's in now so let's uplift.

https://hg.mozilla.org/releases/mozilla-aurora/rev/417fdd07fa58 https://hg.mozilla.org/releases/mozilla-aurora/rev/f6f24ce23978 https://hg.mozilla.org/releases/mozilla-beta/rev/82f563712074 https://hg.mozilla.org/releases/mozilla-beta/rev/22fe8c3d4f3b