This is basically backing out bug 736688.
We'll still have JS for mozbrowser, but we'll override methods on window by modifying the C++ implementation. In some cases (e.g., window.top), the C++ implementation may be complete. In other cases (e.g. window.alert, window.open), we'll probably forward the call to the JS code implementing mozbrowser.
I'm not thrilled about churning this code yet again, but bholley and mrbkap convinced me today that securing things with the current approach (doing Object.defineProperty on window objects as they're created) will require C++ changes. So our options are either this bug or XPCOM hackery. This seems simpler, and since I've already done it, I know it will work.
For an example of the security bug we're preventing (I hope I get this right): With the Object.defineProperty("alert") approach, it's imperative that code inside an mozbrowser never see a window with the un-overridden alert property on it.
So suppose I load evil.com in a mozbrowser, and evil.com contains <iframe id="frame" src="evil.com/frame.html"> (a vanilla iframe). evil.com does
var frameWin = document.getElementById('frame').contentWindow
frameWin is now a cross-compartment wrapper (thanks to compartment-per-global). This isn't going to have my overridden |alert| function on it (*)! Now we're screwed.
This will also fix the security bugs we have open on mozbrowser: bug 740479, bug 740481, which are both about other ways to get around the Object.defineProperty() wrapping.
(*) This actually seems a little weird to me. Is the trick that the iframe has to be cross-origin in addition to cross-compartment?
(In reply to Justin Lebar [:jlebar] from comment #0)
> So suppose I load evil.com in a mozbrowser, and evil.com contains <iframe
> id="frame" src="evil.com/frame.html"> (a vanilla iframe). evil.com does
> var frameWin = document.getElementById('frame').contentWindow
> frameWin is now a cross-compartment wrapper (thanks to
> compartment-per-global). This isn't going to have my overridden |alert|
> function on it (*)! Now we're screwed.
Not quite. Assuming you registered your global creation listener, you should be able to override it on all the scopes. The issue is that, if you have a mozbrowser containing content from origin A, and that content loads an iframe with origin B, then A and B will have Xray wrappers when accessing each others' |window| objects. And the Xray wrappers go directly to the native properties, so they see straight through the defineProperty shenanigans.
Created attachment 623929 [details] [diff] [review]
Boris, I don't think this needs a particularly careful review; all of the non-test changes here are verbatim from the patch you reviewed in bug 725796. (This isn't the whole patch from that bug, but almost all of the changes not in the patch were already in the tree.)
The main wrinkle here is with nsIDocShell's isBrowserFrame property. At the moment, we set that only for in-process browser frames. That's ok so long as all we're overriding is window.top/parent/frameElement, because the OOP implementations work fine with their current implementation.
But when we override window.open/close/alert/prompt/confirm, we're going to want to do that for both in- and out-of-process, so we'll need isBrowserFrame to work everywhere. If it's OK with you, I'd like to fix that in a separate bug. It may turn out that isBrowserFrame needs to be replaced with a pointer to a JS object which has implementations for open/close/alert/etc., so I'd rather figure it out once I need it.
Created attachment 623930 [details] [diff] [review]
Chris, this will get us correct window.top/parent/frameElement semantics for in-process mozbrowser as well as oop.
Comment on attachment 623930 [details] [diff] [review]
r=me. Sorry for the lag. :(
No worries; thanks for the review!
I don't think this file should have been checked in!
Yes, I pushed a follow-up yesterday: https://hg.mozilla.org/integration/mozilla-inbound/rev/899fc2b47336