Deian Stefan Assignee
	Description • 11 years ago

Sandboxed iframes run with a null principal for which Get/SetCsp is not implemented. This results in a scenario (iframe sandbox) wherein CSP is not enforced.

	Ian Melven :imelven
	Comment 1 • 11 years ago

CSP bypass, marking sec-moderate but very possibly a sec-low

Thanks for finding this and especially thanks for writing the patch ! I'll take a look shortly.

	Ian Melven :imelven
	Comment 2 • 11 years ago

Comment on attachment 766488 [details] [diff] [review]
Associate CSP with NullPrincipal

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

Makes sense to me, nsNullPrincipal doesn't inherit from nsBasePrincipal so it needs
its own copies of the getter and setter.

Thanks again for catching this :(

I'll flag Sid and Boris to have a look as well. 

It would be awesome to add a mochitest for this. The general idea would be : add an observer for 
the csp-on-violate-policy topic a la http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_CSP.html?force=1#87
and then have the test page have a sandboxed iframe containing a document with a CSP specified (using a ^headers^ file a la http://mxr.mozilla.org/mozilla-central/source/content/base/test/file_CSP_main_spec_compliant.html%5Eheaders%5E - you definitely want to use the unprefixed Content-Security-Policy header, not X-Content-Security-Policy).
The sandboxed iframe can then try and load a crossdomain <img> and the csp-on-violate-policy notification should fire.

	Deian Stefan Assignee
	Comment 3 • 11 years ago

Thanks! Will write the mochitest now.(In reply to Ian Melven :imelven from comment #2)

> Comment on attachment 766488 [details] [diff] [review]
> Associate CSP with NullPrincipal
> 
> Review of attachment 766488 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Makes sense to me, nsNullPrincipal doesn't inherit from nsBasePrincipal so
> it needs
> its own copies of the getter and setter.
> 
> Thanks again for catching this :(
> 
> I'll flag Sid and Boris to have a look as well. 
> 
> It would be awesome to add a mochitest for this. The general idea would be :
> add an observer for 
> the csp-on-violate-policy topic a la
> http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_CSP.
> html?force=1#87
> and then have the test page have a sandboxed iframe containing a document
> with a CSP specified (using a ^headers^ file a la
> http://mxr.mozilla.org/mozilla-central/source/content/base/test/
> file_CSP_main_spec_compliant.html%5Eheaders%5E - you definitely want to use
> the unprefixed Content-Security-Policy header, not
> X-Content-Security-Policy).
> The sandboxed iframe can then try and load a crossdomain <img> and the
> csp-on-violate-policy notification should fire.

	Sid Stamm [:geekboy or :sstamm]
	Comment 4 • 11 years ago

Comment on attachment 766488 [details] [diff] [review]
Associate CSP with NullPrincipal

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

Someone have a pointer to why the sandboxed iframes use a null principal?  I'm a little confused especially given some of the comments in nsNullPrincipal.cpp:

http://mxr.mozilla.org/mozilla-central/source/caps/src/nsNullPrincipal.cpp#149
"We don't actually do security policy caching.  And it's not like anyone can set a security policy for us anyway."

http://mxr.mozilla.org/mozilla-central/source/caps/src/nsNullPrincipal.cpp#7
"This is the principal that has no rights and can't be accessed by anything other than itself and chrome;"

I think this probably does what you want, but I feel a little uneasy about adding security policies to nsNullPrincipal.

	Daniel Veditz [:dveditz]
	Comment 5 • 11 years ago

nsNullPrincipal was invented for use on data: documents when we didn't have an appropriate parent document. When loaded via the command-line by the OS (originated in some other program) for instances -- wouldn't necessarily want two different data: urls to be able to interact with each other.

Two completely different epochs in what is meant by "security policy". Long ago in Gecko source/caps stood for "capabilities" and principals supported a huge blob of rules for property accesses that could override the default same-origin policy (being either stricter or looser, property by property). These blobs were called "security policies".

Policies other than the default were specified by special prefs (iirc "capability()" rather than "pref()") and associated with origins. nsNullPrincipals never get the same origin twice therefore it was not possible to ever specify a capability "security policy" for it. That was an outcome of the design, it doesn't reflect a desire that nsNullPrincipal represent a pristine container.

"Content Security Policy" is a very different type of security policy and does not seem unreasonable to be part of a nsNullPrincipal document.
Some of these design decisions wouldn't seem so odd if we had chosen the name nsUniquePrincipal which IMHO would have been equally appropriate.

	Deian Stefan Assignee
	Comment 7 • 11 years ago

dveditz: would it make sense to also update the comments in nsNullPrincipal in this bug?

	Daniel Veditz [:dveditz]
	Comment 8 • 11 years ago

IMHO yes (just delete them I think), but we should get r+ from bz on this bug anyway and he can make the final call.

	Deian Stefan Assignee
	Comment 9 • 11 years ago

I'll keep hacking on this later this week, but to update those that weren't on IRC: the current approach has a leak. The implementation of nsIContentSecurityPolicy has an owning ref to the channel and principal. So when setting the csp in the principal, we're introducing a cycle (and neither HttpBasChannel nor nsNullPrincipal participate in the cycle collection.)

	Deian Stefan Assignee
	Comment 10 • 11 years ago

CSP was holding strong reference to both the http channel and channel principal. However, the principal has a strong reference to the CSP policy. Hence we have a cycle which results in a memory leak.  Since the channel and principal should outlive the policy, we only need a weak reference to them.

This seems simpler than making nsNullPrincipal/nsBasePrincipal/BaseHttpChannel/etc. participate in the cycle collector.

	Deian Stefan Assignee
	Comment 11 • 11 years ago

Fixes carelss bug where weakDocRef was null vs. object that returns null when calling get()

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 11 years ago

Comment on attachment 766488 [details] [diff] [review]
Associate CSP with NullPrincipal

r=me but it sounds like whoever is calling SetCsp ignores exceptions somehow or something... Should they?

	Deian Stefan Assignee
	Comment 14 • 11 years ago

(In reply to Boris Zbarsky (:bz) from comment #12)
> Comment on attachment 766488 [details] [diff] [review]
> Associate CSP with NullPrincipal
> 
> r=me but it sounds like whoever is calling SetCsp ignores exceptions somehow
> or something... Should they?

Indeed, the return value of SetCsp in InitCSP was ignored. (Or did you mean something else?)

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 11 years ago

Yes, that's what I meant.  What happens with the return value of InitCSP?

	Deian Stefan Assignee
	Comment 16 • 11 years ago

That's handled properly. StartDocumentLoad fails when InitCSP fails.

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 11 years ago

Comment on attachment 771896 [details] [diff] [review]
CSP should not hold owning refs to channel and principal (fixed)

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 11 years ago

Comment on attachment 774791 [details] [diff] [review]
Check SetCsp failure

This is OK as far as it goes, but I think the callers don't handle the error return here well.... In particular, nsDSURIContentListener::DoContent needs to either set *aAbortProcess to true or return a failure nsresult or return a non-null *aContentHandler.  Please file a followup bug on that, and for bonus points fix it?  ;)

	Deian Stefan Assignee
	Comment 19 • 11 years ago

(In reply to Boris Zbarsky (:bz) from comment #18)
> Comment on attachment 774791 [details] [diff] [review]
> Check SetCsp failure
> 
> This is OK as far as it goes, but I think the callers don't handle the error
> return here well.... In particular, nsDSURIContentListener::DoContent needs
> to either set *aAbortProcess to true or return a failure nsresult or return
> a non-null *aContentHandler.  Please file a followup bug on that, and for
> bonus points fix it?  ;)

Thanks for reviewing these! Will look at the DoContent and file a bug sometime today :)

	Frederik Braun [:freddy]
	Comment 20 • 11 years ago

The test case have an iframe attribute "same-origin" while they should be "allow-same-origin".

	Deian Stefan Assignee
	Comment 21 • 11 years ago

(In reply to Frederik Braun [:freddyb] from comment #20)
> The test case have an iframe attribute "same-origin" while they should be
> "allow-same-origin".

Fixed allow-same-origin. Thanks for catching this.

	Frederik Braun [:freddy]
	Comment 22 • 11 years ago

Comment on attachment 784158 [details] [diff] [review]
FIx tests

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

Looks good.

	Ian Melven :imelven
	Comment 23 • 11 years ago

Deian, this looks like it's ready to land ? Have you done a try push ? :) 

Also did you file the followup mentioned in comment 19 and if so, what's the bug # ? 

thanks !

	Paul Theriault [:pauljt] (no longer reading bugmail)
	Comment 24 • 11 years ago

CSP is pretty important to B2G, so adding this to tracking

	Deian Stefan Assignee
	Comment 25 • 11 years ago

Rebased and squashed everything into a single patch.

Try is good, except for 1 case on B2G; Sid said this is due to the test requring observer.

https://tbpl.mozilla.org/?tree=Try&rev=f3f22dc9ba27

Garrett: mind taking a look at the tests?

	Garrett Robinson [:grobinson]
	Comment 26 • 11 years ago

Comment on attachment 805444 [details] [diff] [review]
Rebase with head

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

This patch looks great, Deian. Thanks for taking the time to write such comprehensive tests (including helpful comments)!

Note that my way of avoiding lots of file_x and file_x^headers^ files is only applicable in the case where you want to test the *same* file_x with different headers. In your case, since each iframe-loaded test file is different, it would not help. I think this is actually the clearest and most understandable way to write these tests.

r=me with the following cleanup:

::: content/base/test/csp/test_bug886164.html
@@ +2,5 @@
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <title>Bug 886164 - Enforce CSP in sandboxed iframe</title>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>        

Nit: trailing whitespace (there's a bunch in this file, please clean it up).

@@ +80,5 @@
> +  if (result) {
> +    passedTests++;
> +  }
> +
> +  if (completedTests == (/*test5 =*/2 + /*test6 =*/3)) {

Remove these comments if they are no longer needed.

	Deian Stefan Assignee
	Comment 27 • 11 years ago

(In reply to Garrett Robinson [:grobinson] from comment #26)
> Comment on attachment 805444 [details] [diff] [review]
> Rebase with head
> 
> Review of attachment 805444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch looks great, Deian. Thanks for taking the time to write such
> comprehensive tests (including helpful comments)!
> 
> Note that my way of avoiding lots of file_x and file_x^headers^ files is
> only applicable in the case where you want to test the *same* file_x with
> different headers. In your case, since each iframe-loaded test file is
> different, it would not help. I think this is actually the clearest and most
> understandable way to write these tests.
> 
> r=me with the following cleanup:
> 
> ::: content/base/test/csp/test_bug886164.html
> @@ +2,5 @@
> > +<html>
> > +<head>
> > +  <meta charset="utf-8">
> > +  <title>Bug 886164 - Enforce CSP in sandboxed iframe</title>
> > +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>        
> 
> Nit: trailing whitespace (there's a bunch in this file, please clean it up).
> 
> @@ +80,5 @@
> > +  if (result) {
> > +    passedTests++;
> > +  }
> > +
> > +  if (completedTests == (/*test5 =*/2 + /*test6 =*/3)) {
> 
> Remove these comments if they are no longer needed.

Thanks for looking at this. Addressed the trailing whitespace issue and removed those comments.

	Deian Stefan Assignee
	Comment 28 • 11 years ago

Comment on attachment 806186 [details] [diff] [review]
0001-Bug-886164-Enforce-CSP-in-sandboxed-iframe.patch

(Sorry if you requested that I just r+ it by r=me.)

	Garrett Robinson [:grobinson]
	Comment 29 • 11 years ago

Comment on attachment 806186 [details] [diff] [review]
0001-Bug-886164-Enforce-CSP-in-sandboxed-iframe.patch

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

Yeah, that is what I meant. It is customary to say "carrying over r+ from grobinson" or similar in the bug comments so it is easy to keep track of who r'd what.

(easiest review ever)

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/18f9b93fe3f2

	Ian Melven :imelven
	Comment 31 • 11 years ago

Thanks for driving this one home, Deian !

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 11 years ago

Follow-up to disable the test on B2G.
https://hg.mozilla.org/integration/mozilla-inbound/rev/47bb120b49f0

	Ed Morley [:emorley]
	Comment 33 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/18f9b93fe3f2
https://hg.mozilla.org/mozilla-central/rev/47bb120b49f0

	James Lal [:lightsofapollo]
	Comment 34 • 11 years ago

Looks like this is breaking functionality in b2g (related to xhr and DOMParser usage)... I am not 100% sure if it is actually because we are violating things or something else.

Can we back this out ?

	James Lal [:lightsofapollo]
	Comment 35 • 11 years ago

Ryan can you direct us to the right person to help get this done?

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 11 years ago

Backed out per comment 34.
https://hg.mozilla.org/mozilla-central/rev/28e5d67b6b5a

	Deian Stefan Assignee
	Comment 37 • 11 years ago

Thanks, Ryan. Will investigate this over the weekend.

	James Lal [:lightsofapollo]
	Comment 38 • 11 years ago

Here is one of the more serious issues we saw (that the backout fixed) https://bugzilla.mozilla.org/show_bug.cgi?id=919587

	Andrew Sutherland [:asuth] (he/him)
	Comment 39 • 11 years ago

(In reply to James Lal [:lightsofapollo] from comment #34)
> Looks like this is breaking functionality in b2g (related to xhr and
> DOMParser usage)... I am not 100% sure if it is actually because we are
> violating things or something else.

Another possible source of regressions was bug 919006 which we papered over by explicitly setting the XHR type.

What actually happens here between the 2 bugs is:

- For bug 919006, on the main thread we were issuing a standard (non-mozSystem, non-mozAnon) XHR request against a snippet of html in a packaged 'app://' URL by using a relative path.  We did not set responseType on that XHR.  The XHR started returning a 500 error until we set responseType to 'text'.  We set responseType and the status code became happy again.

- For bug 919587, on a worker thread, we create an XHR with { mozSystem: true } (for which we are authorized), we do not set responseType, it's a relative URL in a packaged 'app://' like '/autoconfig/gmail.com'.  When get a status in the range [200, 300), we then remote that string to the main thread so we can use DOMParser to create an XML parser.  Sadly the explosion comes in the form of a very generic NS_ERROR_FAILURE XPCOM-style error.  But what we would expect is now there is something wrong with the contents of the string.

Given that this patch changes the behaviour of the null principal and that we create an XMLHttpRequest with 'mozSystem' enabled, the bit where XHR creates a null principal at http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1903 because of mozSsystem seems notable.  There's also a bit where nsXMLHttpRequest::CheckChannelForCrossSiteRequest will call nsIScriptSecurityManager.CheckLoadURIWithPrincipal which I guess might result in the CSP then coming into play?  However, that really only explains bug 919587.  But maybe bug 919006 is/was something else...

	bhavana bajaj [:bajaj]
	Comment 40 • 11 years ago

needinfo on :grobinso/Deian here to help with what the next steps may be as this is blocking 897524

	Deian Stefan Assignee
	Comment 41 • 11 years ago

(In reply to bhavana bajaj [:bajaj] from comment #40)
> needinfo on :grobinso/Deian here to help with what the next steps may be as
> this is blocking 897524

Testing this on b2g. (Sorry for the delays.) Will have a fix by end of week.

	Deian Stefan Assignee
	Comment 42 • 11 years ago

As Bobby noted in (https://bugzilla.mozilla.org/show_bug.cgi?id=919209#c4), Bug 919209 is actually not a result of this so I'm removing the block.

	Frederik Braun [:freddy]
	Comment 43 • 11 years ago

Deian, can you please look at this again?

	Deian Stefan Assignee
	Comment 44 • 11 years ago

Bug 919587 (see bug 919587 comment 10) is due to our checking the
SetCsp failure in InitCSP.

It seems like a reasonable approach with this bug is to remove this
check, since it applies to all principals (created a bug 935690 for
this, which I intend to fix after the 13th).  If this sounds
reasonable to you guys, I can update the patch tonight.

	Deian Stefan Assignee
	Comment 45 • 11 years ago

Removed the SetCsp check in nsDocument (following up in bug 935690).

New try:

https://tbpl.mozilla.org/?tree=Try&rev=a4ae3eddb9c9

	Garrett Robinson [:grobinson]
	Comment 46 • 11 years ago

Comment on attachment 8333598 [details] [diff] [review]
0001-Bug-886164-Enforce-CSP-in-sandboxed-iframe.patch

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

r=me modulo comments below. And we definitely want to follow up on bug 935690 when we can.

::: caps/src/nsNullPrincipal.cpp
@@ +149,4 @@
>  NS_IMETHODIMP
>  nsNullPrincipal::GetSecurityPolicy(void** aSecurityPolicy)
>  {
> +  // We don't actually do security policy caching.

I'm not sure if removing "and it's not like anyone can set a security policy for us anyway" is the best way to clear up confusion here (after all, this function always returns nullptr, which demands an explanation). Could you instead return the original comment, with clarification (based on dveditz's comments) as to why the security policy mentioned here is not the same as CSP?

::: content/base/test/csp/test_bug886164.html
@@ +146,5 @@
> +  if (window.tests[testname] != -1)
> +    return;
> +
> +  window.tests[testname] = result;
> +  is(result, true, testname + ' test: ' + msg);

Nit: replace this with ok(result, test + ' test: ' + msg);

@@ +151,5 @@
> +
> +  // if any test is incomplete, keep waiting
> +  for (var v in window.tests)
> +    if(tests[v] == -1) {
> +      console.log(v + " is not complete");

Nit: Is this leftover from debugging? Best to remove it or comment it out (with a comment about how it can be used for debugging).

	Deian Stefan Assignee
	Comment 47 • 11 years ago

Addressed comments. Carrying over r+ from grobingson

	Deian Stefan Assignee
	Comment 48 • 11 years ago

Since we've been talking about "security policies" it's worth noting that these are going away once bug 913734 lands.

	Phil Ringnalda (:philor)
	Comment 49 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa14392ee63a

	Phil Ringnalda (:philor)
	Comment 50 • 11 years ago

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

	MB
	Comment 52 • 10 years ago

Does this fix affect current addons that use PageMod frame/existing? 

Injecting contentScriptFile/contentScript onto existing iframes with no src/sandbox attributes fails in FFv28 and after. The failure is silent with no security error messages. 

Bug 792479 talks about what seems like a similar problem. However, the workaround mentioned in comment 13 therein does not work for me, or maybe adding "about:blank?magicStr" to iframe src and pageMod include is not the complete workaround.

On a side note, silent failures are making things extremely difficult to debug, particularly with such significant behavior changes in high-level jetpack API.