Justin Lebar (not reading bugmail) Assignee
	Description • 13 years ago

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?

	Bobby Holley (:bholley)
	Comment 1 • 13 years ago

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

	Justin Lebar (not reading bugmail) Assignee
	Comment 2 • 13 years ago

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.

	Justin Lebar (not reading bugmail) Assignee
	Comment 4 • 13 years ago

Chris, this will get us correct window.top/parent/frameElement semantics for in-process mozbrowser as well as oop.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 13 years ago

Comment on attachment 623930 [details] [diff] [review]
Patch v1.1

r=me.  Sorry for the lag.  :(

	Justin Lebar (not reading bugmail) Assignee
	Comment 6 • 13 years ago

No worries; thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6a1e991d80

	Ed Morley [:emorley]
	Comment 7 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/6b6a1e991d80

	Philip Chee
	Comment 8 • 13 years ago

I don't think this file should have been checked in!
https://hg.mozilla.org/mozilla-central/rev/6b6a1e991d80#l4.1

	Justin Lebar (not reading bugmail) Assignee
	Comment 9 • 13 years ago

Yes, I pushed a follow-up yesterday: https://hg.mozilla.org/integration/mozilla-inbound/rev/899fc2b47336

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 10 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/899fc2b47336