Move <iframe mozbrowser> method overrides back into C++

RESOLVED FIXED in mozilla15

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug)

14 Branch
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 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?
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
Blocks: 693515
(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.
(Assignee)

Updated

5 years ago
Blocks: 741587
(Assignee)

Comment 2

5 years ago
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.
Attachment #623929 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

5 years ago
Created attachment 623930 [details] [diff] [review]
Patch v1.1
Attachment #623930 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #623929 - Attachment is obsolete: true
Attachment #623929 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

5 years ago
Chris, this will get us correct window.top/parent/frameElement semantics for in-process mozbrowser as well as oop.
(Assignee)

Updated

5 years ago
Blocks: 740481
(Assignee)

Updated

5 years ago
Blocks: 740479
Comment on attachment 623930 [details] [diff] [review]
Patch v1.1

r=me.  Sorry for the lag.  :(
Attachment #623930 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 6

5 years ago
No worries; thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6a1e991d80
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/6b6a1e991d80
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 8

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

Comment 9

5 years ago
Yes, I pushed a follow-up yesterday: https://hg.mozilla.org/integration/mozilla-inbound/rev/899fc2b47336
https://hg.mozilla.org/mozilla-central/rev/899fc2b47336
You need to log in before you can comment on or make changes to this bug.