Last Comment Bug 754997 - Move <iframe mozbrowser> method overrides back into C++
: Move <iframe mozbrowser> method overrides back into C++
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: 14 Branch
: All All
: -- normal (vote)
: mozilla15
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on:
Blocks: browser-api 740479 740481 741587
  Show dependency treegraph
 
Reported: 2012-05-14 13:12 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-05-25 08:35 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (14.17 KB, patch)
2012-05-14 21:04 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v1.1 (13.35 KB, patch)
2012-05-14 21:06 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-05-14 13:12:22 PDT
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?
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-05-14 14:59:04 PDT
(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.
Comment 2 Justin Lebar (not reading bugmail) 2012-05-14 21:04:06 PDT
Created attachment 623929 [details] [diff] [review]
Patch v1

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.
Comment 3 Justin Lebar (not reading bugmail) 2012-05-14 21:06:22 PDT
Created attachment 623930 [details] [diff] [review]
Patch v1.1
Comment 4 Justin Lebar (not reading bugmail) 2012-05-16 13:15:11 PDT
Chris, this will get us correct window.top/parent/frameElement semantics for in-process mozbrowser as well as oop.
Comment 5 Boris Zbarsky [:bz] 2012-05-22 20:42:40 PDT
Comment on attachment 623930 [details] [diff] [review]
Patch v1.1

r=me.  Sorry for the lag.  :(
Comment 6 Justin Lebar (not reading bugmail) 2012-05-23 08:29:58 PDT
No worries; thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6a1e991d80
Comment 7 Ed Morley [:emorley] 2012-05-24 10:56:09 PDT
https://hg.mozilla.org/mozilla-central/rev/6b6a1e991d80
Comment 8 Philip Chee 2012-05-24 23:15:11 PDT
I don't think this file should have been checked in!
https://hg.mozilla.org/mozilla-central/rev/6b6a1e991d80#l4.1
Comment 9 Justin Lebar (not reading bugmail) 2012-05-25 07:14:41 PDT
Yes, I pushed a follow-up yesterday: https://hg.mozilla.org/integration/mozilla-inbound/rev/899fc2b47336
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-05-25 08:35:39 PDT
https://hg.mozilla.org/mozilla-central/rev/899fc2b47336

Note You need to log in before you can comment on or make changes to this bug.