886164 - CSP not enforced in sandboxed iframe

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Deian Stefan]

Attachment: Associate CSP with NullPrincipal

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.

Comment 1

[Ian Melven :imelven]

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.

Comment 2

[Ian Melven :imelven]

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.

Comment 3

[Deian Stefan]

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.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 5

[Daniel Veditz [:dveditz]]

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.

Comment 6

[Deian Stefan]

Attachment: Basic test cases

Comment 7

[Deian Stefan]

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

Comment 8

[Daniel Veditz [:dveditz]]

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.

Comment 9

[Deian Stefan]

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

Comment 10

[Deian Stefan]

Attachment: CSP should not hold owning refs to channel and principal

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.

Comment 11

[Deian Stefan]

Attachment: CSP should not hold owning refs to channel and principal (fixed)

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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?

Comment 13

[Deian Stefan]

Attachment: Check SetCsp failure

Comment 14

[Deian Stefan]

(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?)

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Deian Stefan]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 18

[Boris Zbarsky [:bzbarsky]]

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?  ;)

Comment 19

[Deian Stefan]

(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 :)

Comment 20

[Frederik Braun [:freddy]]

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

Comment 21

[Deian Stefan]

Attachment: FIx tests

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

Comment 22

[Frederik Braun [:freddy]]

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

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

Looks good.

Comment 23

[Ian Melven :imelven]

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 !

Comment 24

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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

Comment 25

[Deian Stefan]

Attachment: Rebase with head

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?

Comment 26

[Garrett Robinson [:grobinson]]

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.

Comment 27

[Deian Stefan]

Attachment: 0001-Bug-886164-Enforce-CSP-in-sandboxed-iframe.patch

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

Comment 28

[Deian Stefan]

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

Comment 29

[Garrett Robinson [:grobinson]]

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)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Ian Melven :imelven]

Thanks for driving this one home, Deian !

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Ed Morley [:emorley]]

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

Comment 34

[James Lal [:lightsofapollo]]

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 ?

Comment 35

[James Lal [:lightsofapollo]]

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

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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

Comment 37

[Deian Stefan]

Thanks, Ryan. Will investigate this over the weekend.

Comment 38

[James Lal [:lightsofapollo]]

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

Comment 39

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 40

[bhavana bajaj [:bajaj]]

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

Comment 41

[Deian Stefan]

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

Comment 42

[Deian Stefan]

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.

Comment 43

[Frederik Braun [:freddy]]

Deian, can you please look at this again?

Comment 44

[Deian Stefan]

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.

Comment 45

[Deian Stefan]

Attachment: 0001-Bug-886164-Enforce-CSP-in-sandboxed-iframe.patch

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

New try:

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

Comment 46

[Garrett Robinson [:grobinson]]

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

Comment 47

[Deian Stefan]

Attachment: 0001-Bug-886164-Enforce-CSP-in-sandboxed-iframe.patch

Addressed comments. Carrying over r+ from grobingson

Comment 48

[Deian Stefan]

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

Comment 49

[Phil Ringnalda (:philor)]

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

Comment 50

[Phil Ringnalda (:philor)]

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

Comment 52

[MB]

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.