1126911 - Make chrome Window/Location objects opaque to content

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Content is never supposed to get its hands on a chrome object. If it does happen, our security wrapper story is pretty good - we clamp things down pretty tightly, and only allow the same kinds of limited DOM operations that we allow on other cross-origin object (Window and Location).

Really though, there's no good reason why content should be able to do anything to chrome DOM objects at all (especially navigate them). I'm going to see if I can get away with just making these totally opaque.

Comment 1

[Benjamin Smedberg]

Could we be even more restrictive? e.g.

* record in telemetry if this ever does happen
* if we don't see this frequently, make it crash instead of just failing?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> Could we be even more restrictive? e.g.
> 
> * record in telemetry if this ever does happen
> * if we don't see this frequently, make it crash instead of just failing?

I don't see a huge amount of value to that, TBH. Our security wrappers are very solid, and so I'm quite confident that just turning off access at the security wrapper level would prevent content from using them.

Now, crashing if content ever got a reference at all to a chrome object would be interesting, but very likely just cause us to crash unnecessarily all over the place when addons and automation do something dumb.

Comment 3

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa22b0e14742

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix test suite. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Special-case all chrome objects in wrapper selection. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Tests. v1

Comment 7

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3e799c0a8fe

Comment 8

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8556198 [details] [diff] [review]
Part 1 - Fix test suite. v1

Review of attachment 8556198 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the lag here, was on PTO...

Comment 9

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8556199 [details] [diff] [review]
Part 2 - Special-case all chrome objects in wrapper selection. v1

Review of attachment 8556199 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +438,5 @@
>  
> +        // For Vanilla JSObjects exposed from chrome to content, we use a wrapper
> +        // that supports __exposedProps__. We'd like to get rid of these eventually,
> +        // but in their current form they don't cause much trouble.
> +        else if (originIsChrome && !targetIsChrome &&

I think you wanted to get rid of these checks here, since we're already in that if-branch now.

Comment 10

[Bobby Holley (:bholley)]

So, there are a couple of test failures that I'm not sure how to proceed on. Both (browser_aboutAccounts.js and browser_aboutHealthReport.js) follow the same pattern:

(1) Set a pref such that a given http-loaded about dialog loads a special test page (healthreport_testRemoteCommands.html or healthreport_testRemoteCommands.html).
(2) Trigger some browser internals to cause the about test shim to be loaded.
(3) The shim tries to postMessage information to the test by doing window.parent.postMessage(...).

(3) breaks with the changes in this bug, because it's expecting to be able to postMessage to a chrome frame. It's also not great because it relies on the fact that untrusted content is being loaded in a chrome docshell (since window.parent would be null if it were loaded in a content docshell).

Given the layers of intermediate machinery between the pieces of chrome and content test code, I'm not sure how to grab the content and inject the API at the right time (as I do with other tests, see Patch Part 1).

I could add some Cu.allowNormalCrossOriginAccessToChromeObjectsInThisScope(), I guess. But I'd really rather avoid more special cases if I can avoid it.

Gijs, how do you think we should proceed?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #10)
> I could add some
> Cu.allowNormalCrossOriginAccessToChromeObjectsInThisScope(), I guess. But
> I'd really rather avoid more special cases if I can avoid it.

On the other hand, the change in this bug might break addons, and it would be nice to hand them a ready-made opt-out. I think I'll just do that.

Comment 12

[Bobby Holley (:bholley)]

Attachment: (Only land if we need to) - Add opt-out API for the behavior in this bug. v1

I finished implementing this, and then realized that we should probably just
wait to land this until someone really needs it. In the mean time, we can just
use forcePermissiveCOWs to make these two tests work.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Pound some browser-chrome tests into submission. v1

Comment 14

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8318ab35503b

Comment 15

[:Gijs (he/him)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #10)
> So, there are a couple of test failures that I'm not sure how to proceed on.
> Both (browser_aboutAccounts.js and browser_aboutHealthReport.js) follow the
> same pattern:
> 
> (1) Set a pref such that a given http-loaded about dialog loads a special
> test page (healthreport_testRemoteCommands.html or
> healthreport_testRemoteCommands.html).
> (2) Trigger some browser internals to cause the about test shim to be loaded.
> (3) The shim tries to postMessage information to the test by doing
> window.parent.postMessage(...).
> 
> (3) breaks with the changes in this bug, because it's expecting to be able
> to postMessage to a chrome frame. It's also not great because it relies on
> the fact that untrusted content is being loaded in a chrome docshell (since
> window.parent would be null if it were loaded in a content docshell).
> 
> Given the layers of intermediate machinery between the pieces of chrome and
> content test code, I'm not sure how to grab the content and inject the API
> at the right time (as I do with other tests, see Patch Part 1).
> 
> I could add some
> Cu.allowNormalCrossOriginAccessToChromeObjectsInThisScope(), I guess. But
> I'd really rather avoid more special cases if I can avoid it.
> 
> Gijs, how do you think we should proceed?

Now, I could be wrong... but I thought that some of how Firefox accounts actually currently works involves logging in on a login form that comes from the web. Mark, can you clarify if that applies here, too?

(because if so, we could fix these tests the way that the patch does, but the real about:accounts stuff might actually break, which would obviously not be great)

Comment 16

[Mark Hammond [:markh] [:mhammond]]

For about:accounts, I think this is a test-only issue that could be solved by replacing the postMessage calls with a custom dispatchEvent that content_AboutAccounts listens for and shims to a sendAsyncMessage.  about:accounts chrome code uses postMessage *to* content, but never expects to receive one - it only receives custom *events*.  So IIUC, this is a test-only issue that should be fairly easy to fix.

I'm afraid I don't know much about the health-report tests, but I think they will be in the same category.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Attachment: t.patch

eg, this fixes browser_aboutAccounts.

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 8558806 [details] [diff] [review]
t.patch

This seems better than my hammer approach. Gijs, what do you think? Are you able to easily replicate this approach on the other test?

Comment 19

[:Gijs (he/him)]

Blergh. Sorry for not being on top of this - and the bad news is, I am not going to be able to be proactive about this bug today.

To answer the immediate question: no/depends. See bug 840124. Seems about:healthreport actually loads a remote iframe, it seems. Assuming about:healthreport itself satisfies whatever checks we need here (ie we're not breaking actual about:healthreport with this change), this could be fixed by using another iframe layer + content html file wrapper, and only testing postMessage between that and the parent iframe (which shouldn't be affected by this bug) and then using the custom events as illustrated by Mark for the iframeWindow.top <-> browser chrome communication.

I hope this is clear enough, if not please needinfo me and I will try to get back to you either late tonight UK time or tomorrow morning.

Comment 20

[Bobby Holley (:bholley)]

(In reply to :Gijs Kruitbosch from comment #19)
> Blergh. Sorry for not being on top of this - and the bad news is, I am not
> going to be able to be proactive about this bug today.
> 
> To answer the immediate question: no/depends. See bug 840124. Seems
> about:healthreport actually loads a remote iframe, it seems. Assuming
> about:healthreport itself satisfies whatever checks we need here (ie we're
> not breaking actual about:healthreport with this change), this could be
> fixed by using another iframe layer + content html file wrapper, and only
> testing postMessage between that and the parent iframe (which shouldn't be
> affected by this bug) and then using the custom events as illustrated by
> Mark for the iframeWindow.top <-> browser chrome communication.
> 
> I hope this is clear enough, if not please needinfo me and I will try to get
> back to you either late tonight UK time or tomorrow morning.

Ok. Then it sounds like we should take Mark's patch for the accounts stuff and my hammer patch for the FHR thing. Both are in your queue, not super-urgent, but we should do it before the branch.

Comment 21

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs Kruitbosch from comment #19)
> Blergh. Sorry for not being on top of this - and the bad news is, I am not
> going to be able to be proactive about this bug today.
> 
> To answer the immediate question: no/depends. See bug 840124. Seems
> about:healthreport actually loads a remote iframe, it seems.

IIUC, this change should be almost identical - healthreport's chrome code does seem to postMessage *to* the iframe which I believe is fine (and aboutaccounts has the same requitement).  Healthreport itself doesn't seem to rely on that iframe posting back to the chrome code which is where the problem lies.

However, the *test* code does rely on that, using code copied from browser_aboutAccounts - healthreport_testRemoteCommands does a postmessage of the test results, and that could be fixed in the same way as my patch.

Bobby, did you verify my patch fixes the problem with your patches applied? I only verified it works fine without your patches.  If it does I believe I could knock up a similar patch for healthreport.

Comment 22

[Bobby Holley (:bholley)]

(In reply to Mark Hammond [:markh] from comment #21)
> Bobby, did you verify my patch fixes the problem with your patches applied?

I just did - works like a charm.

> I only verified it works fine without your patches.  If it does I believe I
> could knock up a similar patch for healthreport.

That would be wonderful! I'll buy you a beer in whistler. ;-)

Comment 23

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1126911-part-X-avoid-postMessage-from-content-in.patch

This has the same basic fix for browser_aboutAccounts.js and browser_aboutHealthReport.js.  Passes locally but I've not tested it with the rest of Bobby's patches here.

Comment 24

[Bobby Holley (:bholley)]

(In reply to Mark Hammond [:markh] from comment #23)
> Created attachment 8559448 [details] [diff] [review]
> 0001-Bug-1126911-part-X-avoid-postMessage-from-content-in.patch
> 
> This has the same basic fix for browser_aboutAccounts.js and
> browser_aboutHealthReport.js.  Passes locally but I've not tested it with
> the rest of Bobby's patches here.

Works great locally. Pushing to try for good measure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02c55b67e83b

Comment 25

[Bobby Holley (:bholley)]

Comment on attachment 8558742 [details] [diff] [review]
Pound some browser-chrome tests into submission. v1

Obsolete with mark's fix I think.

Comment 26

[:Gijs (he/him)]

Comment on attachment 8559448 [details] [diff] [review]
0001-Bug-1126911-part-X-avoid-postMessage-from-content-in.patch

Review of attachment 8559448 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

Comment 27

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/823612c80ff8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c73754ca898a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2870f06405d0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/903a9d48c1df

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/823612c80ff8
https://hg.mozilla.org/mozilla-central/rev/c73754ca898a
https://hg.mozilla.org/mozilla-central/rev/2870f06405d0
https://hg.mozilla.org/mozilla-central/rev/903a9d48c1df

Comment 29

[Wladimir Palant]

Added dev-doc-needed keyword, as it should be documented that content documents will no longer be able to access their chrome parents - even window.postMessage() is off limits with this change. Frankly, I'm not sure about alternatives when this communication is required, the hack mentioned in bug 1177795 comment 11 merely makes sure that content frames consider themselves the top-level window rather than their chrome parent (this is good enough for Twitter widgets).

Comment 30

[:Gijs (he/him)]

(In reply to Wladimir Palant from comment #29)
> Added dev-doc-needed keyword, as it should be documented that content
> documents will no longer be able to access their chrome parents - even
> window.postMessage() is off limits with this change. Frankly, I'm not sure
> about alternatives when this communication is required

Create a custom event, fire on <html> or |window|, and add a listener from the chrome side?

Comment 31

[Wladimir Palant]

My (rather outdated) knowledge says that events don't bubble up to frames which aren't same-origin. There was custom code ensuring that events could be received on <browser type="content"> nevertheless but it doesn't apply if both the chrome page and its content frame are inside the browser's content area. So it would be great if somebody could verify that this approach actually works.

Comment 32

[:Gijs (he/him)]

(In reply to Wladimir Palant from comment #31)
> My (rather outdated) knowledge says that events don't bubble up to frames
> which aren't same-origin.

Right, my assumption is that the chrome code adds a listener on the content window - access in that direction still works (you'll get wrappers etc., but it works).

Comment 33

[Bobby Holley (:bholley)]

You can also expose an API via Cu.exportFunction / Cu.cloneInto.