Last Comment Bug 784508 - Stop using setIsBrowserElement for docShell isolation in Frameworker.jsm
: Stop using setIsBrowserElement for docShell isolation in Frameworker.jsm
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 18
Assigned To: Mark Hammond [:markh]
:
Mentors:
Depends on: 762569
Blocks: 756648
  Show dependency treegraph
 
Reported: 2012-08-21 14:35 PDT by Jason Duell [:jduell] (needinfo me)
Modified: 2012-09-25 11:39 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
replace setIsBrowserElement with iframe.setAttribute("sandbox", "allow-same-origin"); (1.06 KB, patch)
2012-08-29 19:54 PDT, Mark Hammond [:markh]
gavin.sharp: review+
gavin.sharp: feedback+
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Jason Duell [:jduell] (needinfo me) 2012-08-21 14:35:02 PDT
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 [:jduell] (needinfo me) 2012-08-21 14:35:45 PDT
Blocking a blocker.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-21 14:42:13 PDT
I imagine this probably applies equally to the use of mozbrowser in toolkit/identity/Sandbox.jsm.
Comment 3 Jason Duell [:jduell] (needinfo me) 2012-08-21 14:44:57 PDT
Looks like we need to add some other knob to nsGlobalWindow::GetScriptableParent to check besides GetIsContentBoundary.   "GetIsWorkerBoundary"?
Comment 4 Jason Duell [:jduell] (needinfo me) 2012-08-21 14:46:27 PDT
:jlebar or :mounir might have opinions here too.
Comment 5 Justin Lebar (not reading bugmail) 2012-08-21 15:37:23 PDT
> 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 [:jduell] (needinfo me) 2012-08-28 18:17:47 PDT
Any change we can move on this soon?  I'd like to land cookie-jars and this blocks it.
Comment 7 Mark Hammond [:markh] 2012-08-28 18:56:58 PDT
I'll look at this
Comment 8 Mark Hammond [:markh] 2012-08-29 01:04:54 PDT
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) 2012-08-29 07:59:23 PDT
> 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: gavin@gavinsharp.com] 2012-08-29 16:28:06 PDT
(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] 2012-08-29 19:54:39 PDT
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.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-29 20:06:15 PDT
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] 2012-08-29 20:11:30 PDT
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] 2012-08-30 06:54:55 PDT
Blocking cookie jars so basecamp-blocking+.
Comment 16 Ed Morley [:emorley] 2012-09-04 02:56:17 PDT
https://hg.mozilla.org/mozilla-central/rev/6e3beef67bf5
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-25 11:39:11 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/faa63e3c6033

Note You need to log in before you can comment on or make changes to this bug.