CSP not enforced in sandboxed iframe

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: deian, Assigned: deian)

Tracking

(Blocks 1 bug, {sec-moderate})

Trunk
mozilla28
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr24 affected, b2g18?)

Details

Attachments

(1 attachment, 9 obsolete attachments)

Assignee

Description

6 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.
Attachment #766488 - Flags: review?(imelven)
Attachment #766488 - Flags: review?(grobinson)
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.
Keywords: sec-moderate
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.
Attachment #766488 - Flags: review?(sstamm)
Attachment #766488 - Flags: review?(imelven)
Attachment #766488 - Flags: review+

Updated

6 years ago
Attachment #766488 - Flags: review?(bzbarsky)
Assignee

Comment 3

6 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.
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.
Attachment #766488 - Flags: review?(sstamm)
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.
Assignee

Comment 6

6 years ago
Posted patch Basic test cases (obsolete) — Splinter Review
Assignee

Comment 7

6 years ago
dveditz: would it make sense to also update the comments in nsNullPrincipal in this bug?
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.
Assignee

Comment 9

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

Comment 10

6 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.
Assignee

Comment 11

6 years ago
Fixes carelss bug where weakDocRef was null vs. object that returns null when calling get()
Attachment #771712 - Attachment is obsolete: true
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?
Attachment #766488 - Flags: review?(bzbarsky) → review+
Assignee

Comment 13

6 years ago
Posted patch Check SetCsp failure (obsolete) — Splinter Review
Assignee

Comment 14

6 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?)
Yes, that's what I meant.  What happens with the return value of InitCSP?
Assignee

Comment 16

6 years ago
That's handled properly. StartDocumentLoad fails when InitCSP fails.
Blocks: 897524
Assignee

Updated

6 years ago
Attachment #771896 - Flags: review?(bzbarsky)
Assignee

Updated

6 years ago
Attachment #774791 - Flags: review?(bzbarsky)
Comment on attachment 771896 [details] [diff] [review]
CSP should not hold owning refs to channel and principal (fixed)

r=me
Attachment #771896 - Flags: review?(bzbarsky) → review+
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?  ;)
Attachment #774791 - Flags: review?(bzbarsky) → review+
Assignee

Comment 19

6 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 :)
The test case have an iframe attribute "same-origin" while they should be "allow-same-origin".
Assignee

Comment 21

6 years ago
Posted patch FIx tests (obsolete) — Splinter Review
(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.
Attachment #767469 - Attachment is obsolete: true
Attachment #784158 - Flags: review?(fbraun)
Comment on attachment 784158 [details] [diff] [review]
FIx tests

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

Looks good.
Attachment #784158 - Flags: review?(fbraun) → review+
Assignee

Updated

6 years ago
Attachment #766488 - Flags: review?(grobinson)
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 !
CSP is pretty important to B2G, so adding this to tracking
tracking-b2g18: --- → ?
Assignee

Comment 25

6 years ago
Posted patch Rebase with head (obsolete) — Splinter Review
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?
Attachment #766488 - Attachment is obsolete: true
Attachment #771896 - Attachment is obsolete: true
Attachment #774791 - Attachment is obsolete: true
Attachment #784158 - Attachment is obsolete: true
Attachment #805444 - Flags: review?(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.
Attachment #805444 - Flags: review?(grobinson) → review+
Assignee

Comment 27

6 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.
Attachment #805444 - Attachment is obsolete: true
Assignee

Comment 28

6 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.)
Attachment #806186 - Flags: review?(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)
Attachment #806186 - Flags: review?(grobinson) → review+
Assignee

Updated

6 years ago
Keywords: checkin-needed
Thanks for driving this one home, Deian !
https://hg.mozilla.org/mozilla-central/rev/18f9b93fe3f2
https://hg.mozilla.org/mozilla-central/rev/47bb120b49f0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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 ?
Ryan can you direct us to the right person to help get this done?
Flags: needinfo?(ryanvm)
Backed out per comment 34.
https://hg.mozilla.org/mozilla-central/rev/28e5d67b6b5a
Status: RESOLVED → REOPENED
Flags: needinfo?(ryanvm)
Flags: in-testsuite+
Resolution: FIXED → ---
Target Milestone: mozilla27 → ---
Assignee

Comment 37

6 years ago
Thanks, Ryan. Will investigate this over the weekend.
Here is one of the more serious issues we saw (that the backout fixed) https://bugzilla.mozilla.org/show_bug.cgi?id=919587
Depends on: 919587
(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...
Depends on: 919006
needinfo on :grobinso/Deian here to help with what the next steps may be as this is blocking 897524
Flags: needinfo?(grobinson)
Flags: needinfo?(deian)
Assignee

Comment 41

6 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.
Flags: needinfo?(grobinson)
Flags: needinfo?(deian)
Assignee

Updated

6 years ago
No longer blocks: 919209
Assignee

Comment 42

6 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.
Deian, can you please look at this again?
Flags: needinfo?(deian)
Assignee

Comment 44

6 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.
Flags: needinfo?(deian)
Assignee

Comment 45

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

New try:

https://tbpl.mozilla.org/?tree=Try&rev=a4ae3eddb9c9
Attachment #806186 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8333598 - Flags: review?(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).
Attachment #8333598 - Flags: review?(grobinson) → review+
Assignee

Comment 47

6 years ago
Addressed comments. Carrying over r+ from grobingson
Attachment #8333598 - Attachment is obsolete: true
Assignee

Comment 48

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

Updated

6 years ago
Keywords: checkin-needed
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/aa14392ee63a
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Duplicate of this bug: 1018310

Comment 52

5 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.
You need to log in before you can comment on or make changes to this bug.