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)
Tracking
()
RESOLVED
FIXED
mozilla39
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)
5.60 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
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'"
Reporter | ||
Updated•10 years ago
|
Summary: enforcing multiple csp policies → Multiple CSP policies should be combined towards an intersection (i.e., become stricter)
Reporter | ||
Updated•10 years ago
|
Group: core-security
Reporter | ||
Comment 1•10 years ago
|
||
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.
Comment 2•10 years ago
|
||
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)
Reporter | ||
Comment 3•10 years ago
|
||
This was in aurora, but I can test again for nightly if you want.
Flags: needinfo?(fbraun)
Comment 4•10 years ago
|
||
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)
Reporter | ||
Comment 5•10 years ago
|
||
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)
Reporter | ||
Comment 6•10 years ago
|
||
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 ("").
Reporter | ||
Comment 7•10 years ago
|
||
Reporter | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
(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.
Comment 10•10 years ago
|
||
The console error seems similar to bug 1031417. Maybe this has regressed in nightly?
Updated•9 years ago
|
Assignee: nobody → mozilla
Priority: -- → P1
Assignee | ||
Comment 11•9 years ago
|
||
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)
Reporter | ||
Comment 12•9 years ago
|
||
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?
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(fbraun)
Assignee | ||
Comment 13•9 years ago
|
||
(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 14•9 years ago
|
||
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+
Assignee | ||
Comment 15•9 years ago
|
||
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+
Assignee | ||
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/961205421a71
Target Milestone: --- → mozilla39
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/961205421a71
status-firefox39:
--- → fixed
Assignee | ||
Comment 18•9 years ago
|
||
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
Updated•9 years ago
|
status-firefox-esr31:
--- → wontfix
Updated•9 years ago
|
status-b2g-v2.0:
--- → wontfix
status-b2g-v2.0M:
--- → wontfix
status-b2g-v2.1:
--- → fixed
status-b2g-v2.1S:
--- → fixed
status-b2g-v2.2:
--- → fixed
status-b2g-master:
--- → fixed
status-firefox38:
--- → fixed
status-firefox38.0.5:
--- → fixed
status-firefox-esr38:
--- → fixed
Flags: in-testsuite+
Updated•9 years ago
|
Whiteboard: [b2g-adv-main2.2+]
Updated•9 years ago
|
Whiteboard: [b2g-adv-main2.2+] → [b2g-adv-main2.2-]
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Whiteboard: [b2g-adv-main2.2-] → [b2g-adv-main2.2-][post-critsmash-triage]
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•