Make chrome Window/Location objects opaque to content

RESOLVED FIXED in Firefox 38

Status

()

Core
XPConnect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 1 bug, {dev-doc-needed, sec-other})

unspecified
mozilla38
x86
Mac OS X
dev-doc-needed, sec-other
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [adv-main38-])

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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

4 years ago
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?
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 2

4 years ago
(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.
Flags: needinfo?(bobbyholley)
(Assignee)

Updated

4 years ago
Blocks: 1126925
(Assignee)

Updated

4 years ago
Keywords: sec-other
(Assignee)

Comment 4

4 years ago
Created attachment 8556198 [details] [diff] [review]
Part 1 - Fix test suite. v1
Attachment #8556198 - Flags: review?(gkrizsanits)
(Assignee)

Comment 5

4 years ago
Created attachment 8556199 [details] [diff] [review]
Part 2 - Special-case all chrome objects in wrapper selection. v1
Attachment #8556199 - Flags: review?(gkrizsanits)
(Assignee)

Comment 6

4 years ago
Created attachment 8556200 [details] [diff] [review]
Part 3 - Tests. v1
Attachment #8556200 - Flags: review?(gkrizsanits)
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...
Attachment #8556198 - Flags: review?(gkrizsanits) → review+
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.
Attachment #8556199 - Flags: review?(gkrizsanits) → review+
Attachment #8556200 - Flags: review?(gkrizsanits) → review+
(Assignee)

Comment 10

4 years ago
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?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 11

4 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 12

4 years ago
Created attachment 8558735 [details] [diff] [review]
(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.
(Assignee)

Comment 13

4 years ago
Created attachment 8558742 [details] [diff] [review]
Pound some browser-chrome tests into submission. v1
Attachment #8558742 - Flags: review?(gijskruitbosch+bugs)

Comment 15

4 years ago
(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)
Flags: needinfo?(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.
Flags: needinfo?(mhammond)
Created attachment 8558806 [details] [diff] [review]
t.patch

eg, this fixes browser_aboutAccounts.
(Assignee)

Comment 18

4 years ago
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?
Attachment #8558806 - Flags: review?(gijskruitbosch+bugs)

Comment 19

4 years ago
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.
(Assignee)

Comment 20

4 years ago
(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.
(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.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 22

4 years ago
(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. ;-)
Flags: needinfo?(bobbyholley)
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.
Attachment #8558806 - Attachment is obsolete: true
Attachment #8558806 - Flags: review?(gijskruitbosch+bugs)
Attachment #8559448 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 24

4 years ago
(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
(Assignee)

Comment 25

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

Obsolete with mark's fix I think.
Attachment #8558742 - Attachment is obsolete: true
Attachment #8558742 - Flags: review?(gijskruitbosch+bugs)

Comment 26

4 years ago
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!
Attachment #8559448 - Flags: review?(gijskruitbosch+bugs) → review+
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

Updated

3 years ago
Whiteboard: [adv-main38-]

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release

Comment 29

2 years ago
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).
Keywords: dev-doc-needed

Comment 30

2 years ago
(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

2 years ago
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

2 years ago
(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).
(Assignee)

Comment 33

2 years ago
You can also expose an API via Cu.exportFunction / Cu.cloneInto.
You need to log in before you can comment on or make changes to this bug.