Closed Bug 1036399 Opened 10 years ago Closed 9 years ago

Multiple CSP policies should be combined towards an intersection (i.e., become stricter)

Categories

(Core :: DOM: Security, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox-esr31 --- wontfix
firefox-esr38 --- fixed
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: freddy, Assigned: ckerschb)

References

(Blocks 1 open bug, )

Details

(Keywords: sec-moderate, Whiteboard: [b2g-adv-main2.2-][post-critsmash-triage])

Attachments

(1 file, 3 obsolete files)

This was reported to me out of band by Mario Heiderich (in CC):

If a HTTP response contains two CSP headers, the second one wins.
The spec however, mandates that they are combined to a stricter intersection (see URL field).

Example policies
1) content-security-policy: default-src 'self' 'unsafe-inline'
2) content-security-policy: default-src 'self'

Here's what I observe:
* two CSP headers (1, then 2): 2 wins.
* two CSP headers (2, then 1): 1 wins. XSS possible.

This is especially interesting with header injections.
(The behavior is the same if the header injection attempts to resume the previous header. This is possible with injecting the newline and a space character and then overwriting a directive, i.e. injecting: "\n default-src 'self' 'unsafe-inline'"
Summary: enforcing multiple csp policies → Multiple CSP policies should be combined towards an intersection (i.e., become stricter)
Group: core-security
I'm hiding this bug so I can add my suspicion, that this code path is likely to be hit by CSP in meta tag (pending bug 663570) and CSPs through app manifests.
In what versions can this be reproduced?  We are in the process of replacing the entire CSP backend, and I want to make sure we don't try to fix the new implementation if it's already working there.
Flags: needinfo?(fbraun)
This was in aurora, but I can test again for nightly if you want.
Flags: needinfo?(fbraun)
Yes, please do check in nightly.  I believe we have some automated tests for this and I'm not sure why they aren't catching the problem.  (If you are ambitious, you could create a mochitest...)
Flags: needinfo?(fbraun)
For duplicate headers, the second policy allows overriding the previous.
PHP Code follows.

Variant one:
> header("content-security-policy: default-src 'self' 'unsafe-inline'");
> header("content-security-policy: default-src 'self'");
No inline script executed. Web Console:
> Content Security Policy: The page's settings blocked the loading of a resource at http://other.example.com/foo.png ("default-src http://localhost").
> Content Security Policy: The page's settings blocked the loading of a resource at self ("").

Variant two:
> header("content-security-policy: default-src 'self'");
> header("content-security-policy: default-src 'self' 'unsafe-inline'");
Inline Script executed. Web Console:
> Content Security Policy: The page's settings blocked the loading of a resource at http://other.example.com/foo.png ("default-src http://localhost 'unsafe-inline'").
Flags: needinfo?(fbraun)
I no longer get the same behavior for duplicate directives in the same header line.

Excerpt from header data)
> Accept-Ranges: bytes
> Content-Security-Policy: default-src \'self\'; media-src *; 
>  default-src \'unsafe-inline\';
> Vary: Accept-Encoding
(note how newline+space continue the previous header. This is the same as no newline at all.)

Web Console:
> Content Security Policy: Duplicate default-src directives detected.  All but the first instance will be ignored.
> Content Security Policy: The page's settings blocked the loading of a resource at self ("").
(In reply to Frederik Braun [:freddyb] from comment #6)
> I no longer get the same behavior for duplicate directives in the same
> header line.
>
> Web Console:
> > Content Security Policy: Duplicate default-src directives detected.  All but the first instance will be ignored.
> > Content Security Policy: The page's settings blocked the loading of a resource at self ("").

Despite the facts we discuss in this bug, we shouldn't have an empty message ("") ^^ within the parens in the console message - that looks like another bug to me.
The console error seems similar to bug 1031417.  Maybe this has regressed in nightly?
Assignee: nobody → mozilla
Priority: -- → P1
Attached patch bug_1036399.patch (obsolete) — Splinter Review
Luckily the reported problem is not an issue in our C++ implementation anymore. I am providing a testcase that serves a page using 2 CSP headers (so that appendPolicy is called twice).

Some important observations:
* These are two separate policies, which means that we do not (and also should not) report duplicate directive 'default-src' to the console.
* We report correctly that the inline script is blocked to the console where violated policy part reported within the () is *not* empty anymore.
> Content Security Policy: The page's settings blocked the loading of a resource at self ("default-src http://mochi.test:8888").
* Our algorithm starts out with "allow" and sets the flag whether to load the resource within a loop taking all policies into consideration. Not only for the allows-family (which is responsible amongst others for 'unsafe-inline') but also for permits which is responsible for external resource loads.

Sid, I don't think I am missing something here, am I? I think we should land the test and mark this bug as fixed.

Freddy, is that answering all of your questions/concerns? Btw, thanks for filing.
Flags: needinfo?(fbraun)
Attachment #8561729 - Flags: review?(sstamm)
Comment on attachment 8561729 [details] [diff] [review]
bug_1036399.patch

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

Yes, this looks sane to me. Thanks!

::: dom/base/test/csp/file_dual_header_testserver.sjs
@@ +8,5 @@
> +  // avoid confusing cache behaviors
> +  response.setHeader("Cache-Control", "no-cache", false);
> +
> +  // deliver *TWO* comma separated policies (so that AppendPolicy is called twice)
> +  var csp = "default-src 'self', default-src 'self' 'unsafe-inline'";

This is not what I intuitively expected when re-reading the bug, as it does not exactly match my original comment.
Maybe add a clarification that goes beyond the appendPolicy calls and also explains that this is the same as two separate HTTP headers?
Flags: needinfo?(fbraun)
(In reply to Frederik Braun [:freddyb] from comment #12)
> Comment on attachment 8561729 [details] [diff] [review]
> bug_1036399.patch
> 
> Review of attachment 8561729 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes, this looks sane to me. Thanks!
> 
> ::: dom/base/test/csp/file_dual_header_testserver.sjs
> @@ +8,5 @@
> > +  // avoid confusing cache behaviors
> > +  response.setHeader("Cache-Control", "no-cache", false);
> > +
> > +  // deliver *TWO* comma separated policies (so that AppendPolicy is called twice)
> > +  var csp = "default-src 'self', default-src 'self' 'unsafe-inline'";
> 
> This is not what I intuitively expected when re-reading the bug, as it does
> not exactly match my original comment.
> Maybe add a clarification that goes beyond the appendPolicy calls and also
> explains that this is the same as two separate HTTP headers?

Initially I wanted to send to separate headers, but unfortunately our *.sjs files are not set up to do that. Once you add a second header, it overwrite the first one :-( hence I decided to do the comma separation which in fact is the same as two separate headers.
Anyway, I will add a comment to clarify and explain in a little more detail once Sid reviewed the patches. Thanks Frederik!
Comment on attachment 8561729 [details] [diff] [review]
bug_1036399.patch

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

I'm a little concerned that the "load" event may occasionally fire before the inline script finishes executing, making this intermittently not test anything.  
To catch this please add a second test that verifies the inline script runs when the two policies both allow the script so we can verify that when it should be allowed it is detected properly as allowed.

You could probably add a request parameter to the requests into file_dual_header_testserver.sjs that serves a CSP pair that allows or disallows the script based on the parameter.

Other than that, this looks great.  r=me once you add another test that verifies the script runs when the two policies both allow it.
Attachment #8561729 - Flags: review?(sstamm) → review+
Added an additional test (so we have one that tests that the script is *blocked* and one where the script is *allowed* to run.

Carrying over r+ from Sid.
Attachment #8459440 - Attachment is obsolete: true
Attachment #8459441 - Attachment is obsolete: true
Attachment #8561729 - Attachment is obsolete: true
Attachment #8575516 - Flags: review+
The new C++ backend for CSP was switched on in bug 925004, which landed in 33. The testcase here just verifies that. Closing this bug now.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [b2g-adv-main2.2+]
Whiteboard: [b2g-adv-main2.2+] → [b2g-adv-main2.2-]
Group: core-security → core-security-release
Whiteboard: [b2g-adv-main2.2-] → [b2g-adv-main2.2-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: