784508 - Stop using setIsBrowserElement for docShell isolation in Frameworker.jsm

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Jason Duell]

Right now we've got a call to setIsBrowserElement here:

  http://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/FrameWorker.jsm?rev=677cdab1da6a#245

Gavin tells me this is to prevent social worker code from accessing the parent docShell (.top).  Alas, with "cookie-jars" (bug 756648)  using SetIsBrowserElement() causes the cookie namespace to be different for the worker vs the parent docshell, and :mixedpuppy tells me that's not good (cookies need to be shared).

The solution is presumably to come up with some other mechanism for preventing .top access without changing the AppID or isInBrowserElement fields of the docshell.

With patches for bug 756648 applied, I see a hang in the chrome mochitest toolkit/components/social/test/browser/browser_workerAPI.js (I've verified it's due to cookie namespace issue), so that's the way to verify a fix here.

Comment 1

[Jason Duell]

Blocking a blocker.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

I imagine this probably applies equally to the use of mozbrowser in toolkit/identity/Sandbox.jsm.

Comment 3

[Jason Duell]

Looks like we need to add some other knob to nsGlobalWindow::GetScriptableParent to check besides GetIsContentBoundary.   "GetIsWorkerBoundary"?

Comment 4

[Jason Duell]

:jlebar or :mounir might have opinions here too.

Comment 5

[Justin Lebar (not reading bugmail)]

> Gavin tells me this is to prevent social worker code from accessing the parent docShell 
> (.top).

There are other good reasons: We don't want the social worker code (which aiui we don't trust) to launch plugins or open popups.

It sounds like we should use <iframe sandbox> here, now that that landed.

Comment 6

[Jason Duell]

Any change we can move on this soon?  I'd like to land cookie-jars and this blocks it.

Comment 7

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

I'll look at this

Comment 8

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

I'm trying to understand how the worker can access .top, launch plugins or open popups, mainly to ensure some test coverage and ensure that once the call to setIsBrowserElement() is removed, whatever we use to replace it (presumably a sandboxed iframe) also prevents it.

However, I can't see a way a worker can do any of these things at the moment.  Best I can tell, it has no access to |window|, |document| or document elements, so, eg |top|, |window.top|, |open|, |window.open| are all undefined with or without a call to setIsBrowserElement.

Can someone help me understand the threat model here?

Comment 9

[Justin Lebar (not reading bugmail)]

> I'm trying to understand how the worker can access .top, launch plugins or open popups, 
> mainly to ensure some test coverage and ensure that once the call to 
> setIsBrowserElement() is removed, whatever we use to replace it (presumably a sandboxed 
> iframe) also prevents it.

The untrusted code is running in a web worker?  Then I agree -- I don't think we need setIsBrowserElement.  That's not how I remember it working when I signed off on using mozbrowser here; maybe it changed?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mark Hammond (:markh) from comment #8)
> However, I can't see a way a worker can do any of these things at the
> moment.  Best I can tell, it has no access to |window|, |document| or
> document elements, so, eg |top|, |window.top|, |open|, |window.open| are all
> undefined with or without a call to setIsBrowserElement.
> 
> Can someone help me understand the threat model here?

"A worker" can't do any of the things you list, but the current implementation (FrameWorker.jsm) isn't a worker - it first loads the "workerURL" in a normal iframe, and just relies on the fact that this will be a text/plain document that we can slurp the script text out of and insert into the JS "worker" sandbox. If that workerURL were to return an HTML document, it could run any script it wants in that hidden iframe, and so there needs to be some protection against it doing all of these potentially-dangerous things.

Comment 11

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

Attachment: replace setIsBrowserElement with iframe.setAttribute("sandbox", "allow-same-origin");

I can verify experimentally that this patch prevents <script> code from executing if the worker URL serves up HTML (which seems the only way top etc could be accessed).  I'm struggling to write a test for this though - but given we don't have existing tests for the "worker supplying html" case that might be OK.

It probably is possible to test that certain content (images, plugins etc) from the HTML also fails to load, but given the blocking nature of this bug, I propose opening a new bug for that.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 656730 [details] [diff] [review]
replace setIsBrowserElement with iframe.setAttribute("sandbox", "allow-same-origin");

We should get a bug on file for tests, yeah.

Comment 13

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

Comment on attachment 656730 [details] [diff] [review]
replace setIsBrowserElement with iframe.setAttribute("sandbox", "allow-same-origin");

Sounds like you are happy with this approach, so requesting formal r+ pending a successful try run from https://tbpl.mozilla.org/?tree=Try&rev=8585b1e1f472

Comment 14

[Andrew Overholt [:overholt]]

Blocking cookie jars so basecamp-blocking+.

Comment 15

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

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

Comment 16

[Ed Morley [:emorley]]

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

Comment 17

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/faa63e3c6033