Stop using setIsBrowserElement for docShell isolation in Frameworker.jsm

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jduell, Assigned: markh)

Tracking

unspecified
Firefox 18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox17 fixed)

Details

Attachments

(1 attachment)

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.
(Reporter)

Comment 1

5 years ago
Blocking a blocker.
blocking-basecamp: --- → ?
I imagine this probably applies equally to the use of mozbrowser in toolkit/identity/Sandbox.jsm.
(Reporter)

Comment 3

5 years ago
Looks like we need to add some other knob to nsGlobalWindow::GetScriptableParent to check besides GetIsContentBoundary.   "GetIsWorkerBoundary"?
(Reporter)

Comment 4

5 years ago
:jlebar or :mounir might have opinions here too.
> 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.
(Reporter)

Comment 6

5 years ago
Any change we can move on this soon?  I'd like to land cookie-jars and this blocks it.
(Assignee)

Comment 7

5 years ago
I'll look at this
Assignee: nobody → mhammond
(Assignee)

Comment 8

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

Comment 11

5 years ago
Created attachment 656730 [details] [diff] [review]
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.
Attachment #656730 - Flags: feedback?(gavin.sharp)
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.
Attachment #656730 - Flags: feedback?(gavin.sharp) → feedback+
(Assignee)

Comment 13

5 years ago
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
Attachment #656730 - Flags: review?(gavin.sharp)
Blocking cookie jars so basecamp-blocking+.
blocking-basecamp: ? → +
Attachment #656730 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e3beef67bf5
https://hg.mozilla.org/mozilla-central/rev/6e3beef67bf5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Attachment #656730 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/faa63e3c6033
status-firefox17: --- → fixed
You need to log in before you can comment on or make changes to this bug.