786835 - Stop using setIsBrowserElement for docShell isolation in Sandbox.jsm

Status: RESOLVED FIXED

(Core Graveyard :: Identity, defect)

Description

[Matthew N. [:MattN]]

+++ This bug was initially created as a clone of Bug #784508 +++

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

  https://mxr.mozilla.org/mozilla-central/source/toolkit/identity/Sandbox.jsm?rev=3dfc809486f5#96

...with "cookie-jars" (bug 756648) using SetIsBrowserElement() causes the cookie namespace to be different for the worker vs the parent docshell which will be a problem.

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. <iframe sandbox> which has since landed may be enough.

Comment 1

[Justin Lebar (not reading bugmail)]

When you say "worker", do you mean "web worker"?  'cause if so, I didn't think that could read cookies, like you said in bug 784508.

Comment 2

[Matthew N. [:MattN]]

(In reply to Justin Lebar [:jlebar] from comment #1)
> When you say "worker", do you mean "web worker"?  'cause if so, I didn't
> think that could read cookies, like you said in bug 784508.

Sorry, I modified the cloned comment 0 and forgot to update that reference.  It should say "sandbox".

I'm going to test the @sandbox approach.

Comment 3

[Matthew N. [:MattN]]

From what I can tell, setting @sandbox instead in the code mentioned in comment 0 doesn't block popups (for me on OS X). Perhaps because the hidden DOM Window's document is a chrome URI on Mac? Also, from what I understand, @sandbox wouldn't allow the sandboxed document to access IndexedDB,  LocalStorage, or other storage APIs which we need to be shared between the regular browsing tabs and the sandboxed document for BrowserID.

Comment 4

[Ian Melven :imelven]

(In reply to Matthew N. [:MattN] from comment #3)
> From what I can tell, setting @sandbox instead in the code mentioned in
> comment 0 doesn't block popups (for me on OS X). Perhaps because the hidden
> DOM Window's document is a chrome URI on Mac? 

that is.. unexpected. Popups should be blocked in a sandboxed document, but it hasn't been tested with chrome documents, so something odd may be going on there.

> Also, from what I understand,
> @sandbox wouldn't allow the sandboxed document to access IndexedDB, 
> LocalStorage, or other storage APIs which we need to be shared between the
> regular browsing tabs and the sandboxed document for BrowserID.

right, you can't get to anything like that without 'allow-same-origin' which would make the document not have an null principal.

Comment 5

[Andrew Overholt [:overholt]]

Blocking cookie jars so basecamp-blocking+.  Matt, are you the correct owner here?

Comment 6

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

(In reply to Matthew N. [:MattN] from comment #3)
> From what I can tell, setting @sandbox instead in the code mentioned in
> comment 0 doesn't block popups (for me on OS X).

What do you mean by "doesn't block popups"? Per Mark's experience in bug 784508 it should disable all scripts. Passing allow-same-origin should allow the DOM APIs to continue to work correctly.

Comment 7

[Matthew N. [:MattN]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #6)
> (In reply to Matthew N. [:MattN] from comment #3)
> > From what I can tell, setting @sandbox instead in the code mentioned in
> > comment 0 doesn't block popups (for me on OS X).
> 
> What do you mean by "doesn't block popups"? Per Mark's experience in bug
> 784508 it should disable all scripts. Passing allow-same-origin should allow
> the DOM APIs to continue to work correctly.

I specified sandbox="allow-forms allow-scripts" because we need scripts to be able to run in our Identity sandbox since the page needs to call the DOM APIs.  window.open succeeded in this case.  We have tests[1] in the tree to verify and they failed with this change. allow-scripts says that it shouldn't allow pop-ups per MDN: "Allows the embedded browsing context to run scripts (but not create pop-up windows)"

allow-same-origin seems scary in this case since the description on MDN is "Allows the content to be treated as being from the same origin as the containing document." [2] and in this case the containing document would be the hidden window which makes me think it would give the iframe a chrome origin on OS X. 

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/identity/tests/chrome/test_sandbox.xul?rev=59707ed19e48&force=1#249
[2] https://developer.mozilla.org/en-US/docs/HTML/Element/iframe

Comment 8

[Ian Melven :imelven]

(In reply to Matthew N. [:MattN] from comment #7)

> I specified sandbox="allow-forms allow-scripts" because we need scripts to
> be able to run in our Identity sandbox since the page needs to call the DOM
> APIs.  window.open succeeded in this case.  We have tests[1] in the tree to
> verify and they failed with this change. allow-scripts says that it
> shouldn't allow pop-ups per MDN: "Allows the embedded browsing context to
> run scripts (but not create pop-up windows)"

that's odd, because there's tests in the tree for iframe sandbox to make sure window.open and other such methods are blocked (we haven't implemented allow-popups yet, that's bug 766282), see https://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_iframe_sandbox_general.html?force=1#111
 
> allow-same-origin seems scary in this case since the description on MDN is
> "Allows the content to be treated as being from the same origin as the
> containing document." [2] and in this case the containing document would be
> the hidden window which makes me think it would give the iframe a chrome
> origin on OS X. 

right, allow-same-origin means the document will have the same principal it would normally have without the sandbox attribute specified.

Comment 9

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

(In reply to Ian Melven :imelven from comment #8)
> right, allow-same-origin means the document will have the same principal it
> would normally have without the sandbox attribute specified.

Which is not the same principal as its parent element, assuming the iframe has the mozframetype attribute set to "content", and a different origin. So I don't think using allow-same-origin presents a problem.

Comment 10

[Lucas Adamski [:ladamski]]

Eagerly awaited, as this is blocking cookie and appcache jars from landing...

Comment 11

[Justin Lebar (not reading bugmail)]

Can we just write this ourselves?  It should be simple, and we can't afford to keep spinning here...

Comment 12

[Matthew N. [:MattN]]

Attachment: switch to @sandbox with "allow-forms allow-scripts allow-same-origin"

If the MDN defintion of allow-same-origin is inaccurate (see comment 7 paragraph 2) then based on comment 9 this should be safe. Could someone update MDN to clarify this case?

This passes test_sandbox.xul now.  
Try push: https://tbpl.mozilla.org/?tree=Try&rev=6c80dc84a24c

Comment 13

[Matthew N. [:MattN]]

(Quoting Andrew Overholt [:overholt] from comment #5)
> Blocking cookie jars so basecamp-blocking+.  Matt, are you the correct owner
> here?

The bug was marked basecamp-blocking- when this comment was added. I can't fix the flag.

(In reply to Lucas Adamski from comment #10)
> Eagerly awaited, as this is blocking cookie and appcache jars from landing...

I wouldn't block on this since it's for a preffed off feature that probably has only a handful of people testing it.  It's not a pref that can be on for everyday use as it breaks sites using older BrowserID APIs and requires the user to have their own BrowserID compliant Identity Provider.

Comment 14

[Justin Lebar (not reading bugmail)]

> I wouldn't block on this 

Unfortunately I think the only way not to block on this would be to disable the relevant tests.

Comment 15

[Matthew N. [:MattN]]

(In reply to Justin Lebar [:jlebar] from comment #14)
> > I wouldn't block on this 
> 
> Unfortunately I think the only way not to block on this would be to disable
> the relevant tests.

I don't think any tests for this case were failing but I can't be sure because most try results have expired in bug 756648.

Comment 16

[Ian Melven :imelven]

(In reply to Matthew N. [:MattN] from comment #12)
> Created attachment 661370 [details] [diff] [review]
> switch to @sandbox with "allow-forms allow-scripts allow-same-origin"
> 
> If the MDN defintion of allow-same-origin is inaccurate (see comment 7
> paragraph 2) then based on comment 9 this should be safe. Could someone
> update MDN to clarify this case?
> 
> This passes test_sandbox.xul now.  
> Try push: https://tbpl.mozilla.org/?tree=Try&rev=6c80dc84a24c

I updated MDN to :

"allow-same-origin: Allows the content to be treated as being from its normal origin. If this keyword is not used, the embedded content is treated as being from a unique origin."

Comment 17

[Ian Melven :imelven]

Comment on attachment 661370 [details] [diff] [review]
switch to @sandbox with "allow-forms allow-scripts allow-same-origin"

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

::: toolkit/identity/Sandbox.jsm
@@ +87,1 @@
>      frame.style.visibility = "collapse";

i'm curious what we need allow-forms for ?

Comment 18

[Matthew N. [:MattN]]

(In reply to Ian Melven :imelven from comment #17)
> Comment on attachment 661370 [details] [diff] [review]
> switch to @sandbox with "allow-forms allow-scripts allow-same-origin"
>
> ::: toolkit/identity/Sandbox.jsm
> @@ +87,1 @@
> >      frame.style.visibility = "collapse";
> 
> i'm curious what we need allow-forms for ?

The idea is to make it as simple as possible for an Identity Provider (IDP) to support BrowserID and since current authentication systems use forms submitted by JS as a form of SSO (eg. http://support.f5.com/kb/en-us/products/big-ip_apm/manuals/product/apm-sso-config-11-2-0/3.html ) so I was trying not to break that pattern.  

Perhaps we could disallow forms until there is a request for this but the idea was to prevent adding barriers especially since this form pattern works in the persona.org shim AFAIK.

Comment 19

[Justin Lebar (not reading bugmail)]

Comment on attachment 661370 [details] [diff] [review]
switch to @sandbox with "allow-forms allow-scripts allow-same-origin"

I'm happy to defer to Ian on the review here.  r=me for getting rid of setIsBrowserElement.  :)

Comment 20

[Ian Melven :imelven]

(In reply to Matthew N. [:MattN] from comment #18)
>
> > i'm curious what we need allow-forms for ?
> 
> The idea is to make it as simple as possible for an Identity Provider (IDP)
> to support BrowserID and since current authentication systems use forms
> submitted by JS as a form of SSO (eg.
> http://support.f5.com/kb/en-us/products/big-ip_apm/manuals/product/apm-sso-
> config-11-2-0/3.html ) so I was trying not to break that pattern.  
> 
> Perhaps we could disallow forms until there is a request for this but the
> idea was to prevent adding barriers especially since this form pattern works
> in the persona.org shim AFAIK.

That all makes sense to me - seems like what we're really concerned with here is blocking top navigation, plugins, and popups. Matt, i CC'd you on another iframe sandbox bug that I hope to fix ASAP btw.

Comment 21

[Ian Melven :imelven]

Comment on attachment 661370 [details] [diff] [review]
switch to @sandbox with "allow-forms allow-scripts allow-same-origin"

I'm not a peer or anything, but this looks good to me.

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 661370 [details] [diff] [review]
switch to @sandbox with "allow-forms allow-scripts allow-same-origin"

(In reply to Ian Melven :imelven from comment #20)
> That all makes sense to me - seems like what we're really concerned with
> here is blocking top navigation, plugins, and popups. 

Yes, we want to prevent the content from doing obvious things such as escaping out of the frame but also others which aren't appropriate for a hidden document. For example, it doesn't make sense for popups/alerts to open out of (seemingly) nowhere or to have a flash advertising with audio playing with no way to stop it.

> Matt, i CC'd you on another iframe sandbox bug that I hope to fix ASAP btw.

Thanks for that and also clarifying the MDN page.  I'm kind of surprised my tests aren't failing because of that bug. Not sure if this code isn't affected or my testcase needs improving.

Comment 23

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

Comment on attachment 661370 [details] [diff] [review]
switch to @sandbox with "allow-forms allow-scripts allow-same-origin"

Probably a good idea to add a comment explaining why allow-same-origin+allow-script (which is usually an unsafe combination) is OK in this particular case (the embedding page is guaranteed to not be same origin, per comment 9).

Is it necessary to remove the mozbrowser attribute as well? I'm not sure how it actually works, or how it differs from setIsBrowserElement. @sandbox removes the ability for the child frame to navigate the hidden window, AIUI, which is the main concern, but it would be nice to neutralize top/frameElement entirely. Maybe something to investigate later.

Comment 24

[Lucas Adamski [:ladamski]]

The block call is up to :jduell really.  If this is not really necessary to land cookie and appcache jars I'm happy to mark it -, but not until we resolve that.

Comment 25

[Justin Lebar (not reading bugmail)]

> Is it necessary to remove the mozbrowser attribute as well?

The attribute and setIsBrowserElement() should do basically the same thing.  It may be that we were ignoring the attribute because the hidden window doesn't have permission to create mozbrowsers.

In any case, we definitely don't want this window to be a mozbrowser, because that implies that its facebook.com is considered a different origin than the browser's facebook.com, which would defeat the purpose of this frame.

Comment 26

[Jason Duell]

re: comment 13

> I wouldn't block on this since it's for a preffed off feature that probably has only a handful of people testing it. 

OK, well then perhaps we shouldn't block on this anymore

re: comment 14
> I think the only way not to block on this would be to disable the relevant tests

I don't think this is causing any tests to fail, but I'll run through try and check.

Comment 27

[Andrew Overholt [:overholt]]

Based on feedback, not blocking.

Comment 28

[Matthew N. [:MattN]]

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/da306dc448b9

Try results: https://tbpl.mozilla.org/?tree=Try&rev=6c80dc84a24c and https://tbpl.mozilla.org/?tree=Try&rev=222c000ce3a0

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da306dc448b9