Closed Bug 885433 Opened 11 years ago Closed 11 years ago

CSP 1.0 should not block inline scripts or eval unless script-src or default-src are included

Categories

(Core :: Security, defect)

20 Branch
x86
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla25
Tracking Status
firefox23 + verified
firefox24 + verified
firefox25 + verified

People

(Reporter: dveditz, Assigned: grobinson)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

Between the Mozilla proposal and the CSP 1.0 spec the rules for when inline-scripts and eval are blocked changed. In the final spec site authors opt-in to the stricter regime NOT merely by using CSP but by opting into "script" restrictions specifically. This is done explicitly by including script-src or implicitly by using default-src The same applies to inline style restrictions, which should not be enforced unless there's a style-src or default-src directive. A policy of CSP: sandbox; frame-options mysite.com yoursite.com; should not block any inline scripts or styles, and should allow eval
Summary: CSP should not block inline scripts or evail unless script-src or default-src are included → CSP should not block inline scripts or eval unless script-src or default-src are included
Bug 886132 might be an instance of this for inline styles. It looks like the policy has a script-src only and not default-src or style-src, but inline styles are being blocked per the messages in the web console. A fix for this would be to initialize allowsInlineScripts/Styles/Eval to true and flip it the appropriate inline script/style flag to false if a script-src/style-src directive is seen, flipping both to false if default-src is explicitly seen (not inferred).
Attached patch Test 1 (obsolete) — Splinter Review
Mochitest with two different CSP's: one that does not mention default-src, script-src, or style-src (and so does not trigger unsafe-inline or unsafe-eval protections) and one that is just default-src 'self' (which should trigger unsafe-inline protection for scripts and stylesheets and unsafe-eval protection for scripts). Right now three tests fail, representing the original issue created by the bug, and three tests pass, representing the blocking that is now applied by default whenever CSP is used but should only be applied if default-src, script-src, or style-src are used. I note that while writing this test, I had difficulty with 'unsafe-eval'. In my experience, the eval worked if I did not provide a CSP but would not work with a CSP, even if I set "script-src 'unsafe-inline' 'unsafe-eval';". This may be a separate bug, and if so it also implies problems with the evalscript mochitest.
Assignee: nobody → grobinson
Status: NEW → ASSIGNED
Fixed Bug 887974, which was the cause of the 'unsafe-eval' weirdness mentioned above.
Depends on: 887974
Attached patch Patch 1 (obsolete) — Splinter Review
First pass at implementing the correct behavior of "opting-in" to default blocking of inline and eval. Not the most elegant but it gets the job done (otherwise would need to refactor that whole function). CSP test suite passes.
Attachment #768091 - Attachment is obsolete: true
Attachment #768738 - Flags: feedback?(sstamm)
Attachment #768738 - Flags: feedback?(imelven)
Attached patch Patch 2 (obsolete) — Splinter Review
Nicer code. Passes the CSP test suite. I also checked the problematic URL from Bug 886132 and was not able to reproduce the problems with Disqus.
Attachment #768738 - Attachment is obsolete: true
Attachment #768738 - Flags: feedback?(sstamm)
Attachment #768738 - Flags: feedback?(imelven)
Attachment #768986 - Flags: feedback?(sstamm)
Attachment #768986 - Flags: feedback?(imelven)
Comment on attachment 768986 [details] [diff] [review] Patch 2 Review of attachment 768986 [details] [diff] [review]: ----------------------------------------------------------------- A few small things, but pretty much there. We talked yesterday about needing to loop twice, I'm not too worried about it for now because as we all know, we want to rewrite this in C++ some day... Good tests ! ::: content/base/src/CSPUtils.jsm @@ +528,5 @@ > + dirs[dirname] = dirvalue; > + } > + > + /* Spec compliant policies have different default behavior for inline > + * scripts, styles, and eval. Bug 885433 */ the usual style seems to be to have the closing */ on its own line Good comment, especially referencing the bug # @@ +535,5 @@ > + aCSPR._allowInlineStyles = true; > + > + /* In CSP 1.0, you need to opt-in to blocking inline scripts and eval by > + * specifying either default-src or script-src, and to blocking inline > + * styles by specifying either default-src or style-src. */ same as above, closing */ on its own line ::: content/base/test/Makefile.in @@ +634,5 @@ > + test_CSP_bug885433.html \ > + file_CSP_bug885433_allows.html \ > + file_CSP_bug885433_allows.html^headers^ \ > + file_CSP_bug885433_blocks.html \ > + file_CSP_bug885433_blocks.html^headers^ \ these don't look indented the same as the rest of the file ::: content/base/test/file_CSP_bug885433_allows.html @@ +19,5 @@ > + > + // Use eval to set a style attribute > + try { > + eval('document.getElementById("unsafe-eval-script-allowed").style.color = "green";'); > + } catch (e) {} do you need the try/catch ? this is a failing test if the eval is blocked anyways.. ::: content/base/test/file_CSP_bug885433_blocks.html @@ +3,5 @@ > +The Content-Security-Policy header for this file does not include any of the > +default-src, script-src, or style-src directives. It should not use the old > +default behavior of blocking unsafe-inline and unsafe-eval on scripts, and > +unsafe-inline on styles, because no directives related to scripts or styles > +are specified. --> this comment is not true ;) It needs to be reversed, the header does include default-src, everything should be blocked etc. ::: content/base/test/test_CSP_bug885433.html @@ +1,4 @@ > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Test for Content Security Policy inline stylesheets stuff</title> I'd like it if the title was more explicit about what's being tested :) @@ +50,5 @@ > + SimpleTest.finish(); > +} > + > +SpecialPowers.pushPrefEnv( > + {'set':[["security.csp.speccompliant", true]]}, do we need to set this any more ? The pref is flipped in Fx23 and later..
Attachment #768986 - Flags: feedback?(imelven) → feedback+
Comment on attachment 768986 [details] [diff] [review] Patch 2 Review of attachment 768986 [details] [diff] [review]: ----------------------------------------------------------------- looks pretty good to me. ::: content/base/src/CSPUtils.jsm @@ +528,5 @@ > + dirs[dirname] = dirvalue; > + } > + > + /* Spec compliant policies have different default behavior for inline > + * scripts, styles, and eval. Bug 885433 */ super-Nit: comments inside functions should really be line comments, not block comments. ::: content/base/test/file_CSP_bug885433_allows.html @@ +19,5 @@ > + > + // Use eval to set a style attribute > + try { > + eval('document.getElementById("unsafe-eval-script-allowed").style.color = "green";'); > + } catch (e) {} I think eval throws a security exception if CSP blocks it. The try/catch can help us detect and recover from it if there are other tests in this document. (This is probably more precise if it doesn't just choke and stop generating results.) Can you make the failure case more explicit by handling the exception or providing a comment that explains what a thrown exception means? ::: content/base/test/file_CSP_bug885433_blocks.html @@ +3,5 @@ > +The Content-Security-Policy header for this file does not include any of the > +default-src, script-src, or style-src directives. It should not use the old > +default behavior of blocking unsafe-inline and unsafe-eval on scripts, and > +unsafe-inline on styles, because no directives related to scripts or styles > +are specified. --> I recommend pasting a duplicate of the ^headers^ in a comment here and briefly explain what is being tested and what should succeed. This way it's easy to see the test case and the expected results all in one spot.
Attachment #768986 - Flags: feedback?(sstamm) → feedback+
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #8) > Comment on attachment 768986 [details] [diff] [review] > Patch 2 > > Review of attachment 768986 [details] [diff] [review]: > ----------------------------------------------------------------- > > looks pretty good to me. > > ::: content/base/src/CSPUtils.jsm > @@ +528,5 @@ > > + dirs[dirname] = dirvalue; > > + } > > + > > + /* Spec compliant policies have different default behavior for inline > > + * scripts, styles, and eval. Bug 885433 */ > > super-Nit: comments inside functions should really be line comments, not > block comments. > Just curious - why? > ::: content/base/test/file_CSP_bug885433_allows.html > @@ +19,5 @@ > > + > > + // Use eval to set a style attribute > > + try { > > + eval('document.getElementById("unsafe-eval-script-allowed").style.color = "green";'); > > + } catch (e) {} > > I think eval throws a security exception if CSP blocks it. The try/catch > can help us detect and recover from it if there are other tests in this > document. (This is probably more precise if it doesn't just choke and stop > generating results.) Can you make the failure case more explicit by > handling the exception or providing a comment that explains what a thrown > exception means? > That was the motivation for the try/catch. I'll comment it and have it trigger a test failure. > ::: content/base/test/file_CSP_bug885433_blocks.html > @@ +3,5 @@ > > +The Content-Security-Policy header for this file does not include any of the > > +default-src, script-src, or style-src directives. It should not use the old > > +default behavior of blocking unsafe-inline and unsafe-eval on scripts, and > > +unsafe-inline on styles, because no directives related to scripts or styles > > +are specified. --> > > I recommend pasting a duplicate of the ^headers^ in a comment here and > briefly explain what is being tested and what should succeed. This way it's > easy to see the test case and the expected results all in one spot. Good idea!
(In reply to Garrett Robinson [:grobinson] from comment #9) > Just curious - why? For consistent style with the rest of the file. Doesn't matter a whole lot, but I find it easier to manage line comments in general, or at the very least, a consistent style. e.g.: http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#307 http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#572 http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#792
(In reply to Ian Melven :imelven from comment #7) > Comment on attachment 768986 [details] [diff] [review] > Patch 2 > > Review of attachment 768986 [details] [diff] [review]: > ----------------------------------------------------------------- > > A few small things, but pretty much there. We talked yesterday about needing > to loop twice, > I'm not too worried about it for now because as we all know, we want to > rewrite this in C++ some day... > I think this rewrite actually improves that situation. Now there is one loop that parses the directives into an object, which we can then simply query for the special-case directives rather than doing a loop-and-compare. More efficient and more readable. > Good tests ! > > ::: content/base/src/CSPUtils.jsm > @@ +528,5 @@ > > + dirs[dirname] = dirvalue; > > + } > > + > > + /* Spec compliant policies have different default behavior for inline > > + * scripts, styles, and eval. Bug 885433 */ > > the usual style seems to be to have the closing */ on its own line > > Good comment, especially referencing the bug # > > @@ +535,5 @@ > > + aCSPR._allowInlineStyles = true; > > + > > + /* In CSP 1.0, you need to opt-in to blocking inline scripts and eval by > > + * specifying either default-src or script-src, and to blocking inline > > + * styles by specifying either default-src or style-src. */ > > same as above, closing */ on its own line > > ::: content/base/test/Makefile.in > @@ +634,5 @@ > > + test_CSP_bug885433.html \ > > + file_CSP_bug885433_allows.html \ > > + file_CSP_bug885433_allows.html^headers^ \ > > + file_CSP_bug885433_blocks.html \ > > + file_CSP_bug885433_blocks.html^headers^ \ > > these don't look indented the same as the rest of the file > Ah, my editor inserts spaces by default and the convention in Makefiles appears to be tabs. I know that is a requirement for GNU Make, doesn't appear to be a requirement for whatever we're using. I'll make it conform to the rest of the file. > ::: content/base/test/file_CSP_bug885433_allows.html > @@ +19,5 @@ > > + > > + // Use eval to set a style attribute > > + try { > > + eval('document.getElementById("unsafe-eval-script-allowed").style.color = "green";'); > > + } catch (e) {} > > do you need the try/catch ? this is a failing test if the eval is blocked > anyways.. > Yes, otherwise eval throws a security exception which would derail the rest of the tests. > ::: content/base/test/file_CSP_bug885433_blocks.html > @@ +3,5 @@ > > +The Content-Security-Policy header for this file does not include any of the > > +default-src, script-src, or style-src directives. It should not use the old > > +default behavior of blocking unsafe-inline and unsafe-eval on scripts, and > > +unsafe-inline on styles, because no directives related to scripts or styles > > +are specified. --> > > this comment is not true ;) It needs to be reversed, the header > does include default-src, everything should be blocked etc. > Thanks for the catch - copy/paste forgetfulness. > ::: content/base/test/test_CSP_bug885433.html > @@ +1,4 @@ > > +<!DOCTYPE HTML> > > +<html> > > +<head> > > + <title>Test for Content Security Policy inline stylesheets stuff</title> > > I'd like it if the title was more explicit about what's being tested :) > > @@ +50,5 @@ > > + SimpleTest.finish(); > > +} > > + > > +SpecialPowers.pushPrefEnv( > > + {'set':[["security.csp.speccompliant", true]]}, > > do we need to set this any more ? The pref is flipped in Fx23 and later.. Good point! I'll remove it.
Attached patch Patch 3 (obsolete) — Splinter Review
Incorporates imelven and sstamm's feedback. Specific changes and rationale are described in the previous comments.
Attachment #768986 - Attachment is obsolete: true
Attachment #769221 - Flags: review?(sstamm)
Attachment #769221 - Flags: review?(imelven)
Attached patch Patch 4 (obsolete) — Splinter Review
Forgot to fix the title of the bug per imelven's review.
Attachment #769221 - Attachment is obsolete: true
Attachment #769221 - Flags: review?(sstamm)
Attachment #769221 - Flags: review?(imelven)
Attachment #769229 - Flags: review?(sstamm)
Attachment #769229 - Flags: review?(imelven)
Comment on attachment 769229 [details] [diff] [review] Patch 4 Review of attachment 769229 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me :D
Attachment #769229 - Flags: review?(imelven) → review+
Blocks: 888172
Given the breakage in current Aurora (24.0a2), this should also land there.
From what I hear, 23 should be unaffected by this (correct me if I'm wrong), but both 24 and 25 are, and as the bug blocks the very popular Disqus system from working correctly, I think we should track this for both affected trains.
B2G VM (arm) failed on the try run with the tests in the patch : 19:40:51 INFO - 40 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_bug885433.html | Inline script should be blocked 19:40:51 INFO - 41 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_bug885433.html | Eval should be blocked 19:40:51 INFO - 42 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_bug885433.html | Inline style should be blocked 20:20:44 INFO - I/GeckoDump( 814): 40 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_bug885433.html | Inline script should be blocked 20:20:44 INFO - I/GeckoDump( 814): 41 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_bug885433.html | Eval should be blocked 20:20:44 INFO - I/GeckoDump( 814): 42 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_bug885433.html | Inline style should be blocked
(In reply to Ian Melven :imelven from comment #19) > B2G VM (arm) failed on the try run with the tests in the patch : > > 19:40:51 INFO - 40 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/base/test/test_CSP_bug885433.html | Inline script should be > blocked > 19:40:51 INFO - 41 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/base/test/test_CSP_bug885433.html | Eval should be blocked > 19:40:51 INFO - 42 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/base/test/test_CSP_bug885433.html | Inline style should be > blocked > 20:20:44 INFO - I/GeckoDump( 814): 40 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/base/test/test_CSP_bug885433.html | Inline script should be > blocked > 20:20:44 INFO - I/GeckoDump( 814): 41 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/base/test/test_CSP_bug885433.html | Eval should be blocked > 20:20:44 INFO - I/GeckoDump( 814): 42 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/base/test/test_CSP_bug885433.html | Inline style should be > blocked This is because this fix is only for CSP 1.0, which is not on by default in B2G (Bug 858787). The test originally used SpecialPowers.pushPrefEnv to flip security.csp.speccompliant for this test, but as you pointed out in Comment 7 this should no longer be necessary because the pref is on by default in >= Fx23. I changed the title of the bug to clarify that this solely affects CSP 1.0. Our options here are 1. Restore the use of pushPrefEnv to flip the spec and land this ASAP, or 2. Wait until Bug 858787 lands Since this patches fixes two other bugs (at the moment), I prefer the first option. We should land it as soon as possible to fix desktop Firefox. I also think it is logical because the bug is only for CSP 1.0, to explicitly set that pref even if it is now on by default.
Summary: CSP should not block inline scripts or eval unless script-src or default-src are included → CSP 1.0 should not block inline scripts or eval unless script-src or default-src are included
(In reply to Garrett Robinson [:grobinson] from comment #20) > (In reply to Ian Melven :imelven from comment #19) > > B2G VM (arm) failed on the try run with the tests in the patch : > > > > 19:40:51 INFO - 40 ERROR TEST-UNEXPECTED-FAIL | > > /tests/content/base/test/test_CSP_bug885433.html | Inline script should be > > blocked > > 19:40:51 INFO - 41 ERROR TEST-UNEXPECTED-FAIL | > > /tests/content/base/test/test_CSP_bug885433.html | Eval should be blocked > > 19:40:51 INFO - 42 ERROR TEST-UNEXPECTED-FAIL | > > /tests/content/base/test/test_CSP_bug885433.html | Inline style should be > > blocked > > 20:20:44 INFO - I/GeckoDump( 814): 40 ERROR TEST-UNEXPECTED-FAIL | > > /tests/content/base/test/test_CSP_bug885433.html | Inline script should be > > blocked > > 20:20:44 INFO - I/GeckoDump( 814): 41 ERROR TEST-UNEXPECTED-FAIL | > > /tests/content/base/test/test_CSP_bug885433.html | Eval should be blocked > > 20:20:44 INFO - I/GeckoDump( 814): 42 ERROR TEST-UNEXPECTED-FAIL | > > /tests/content/base/test/test_CSP_bug885433.html | Inline style should be > > blocked > > This is because this fix is only for CSP 1.0, which is not on by default in > B2G (Bug 858787). The test originally used SpecialPowers.pushPrefEnv to flip > security.csp.speccompliant for this test, but as you pointed out in Comment > 7 this should no longer be necessary because the pref is on by default in >= > Fx23. Ah yes, but it's only on by default for desktop at the moment. Thanks for reminding me. > I changed the title of the bug to clarify that this solely affects CSP 1.0. > > Our options here are > > 1. Restore the use of pushPrefEnv to flip the spec and land this ASAP, or > 2. Wait until Bug 858787 lands > > Since this patches fixes two other bugs (at the moment), I prefer the first > option. We should land it as soon as possible to fix desktop Firefox. I also > think it is logical because the bug is only for CSP 1.0, to explicitly set > that pref even if it is now on by default. I prefer #1 as well, and totally agree that we should get this fixed for desktop as soon as possible. We can clean up the specCompliant pushPrefEnvs after we get CSP 1.0 on for Firefox OS, and I think your general strategy of fixing everything on desktop that we know about first and then coming back to B2G again is a very good one.
Attached patch Patch 5Splinter Review
Reinstate running the test with the speccompliant pref. I confirmed that the Mochitest passes on B2G, which was the only failure in the last try run. Carrying over r+ from imelven.
Attachment #769229 - Attachment is obsolete: true
Attachment #769229 - Flags: review?(sstamm)
Attachment #769948 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment on attachment 769948 [details] [diff] [review] Patch 5 [Approval Request Comment] Bug caused by (feature/regressing bug #): CSP 1.0 / bug 842657 User impact if declined: sites that expect CSP to behave per spec may break e.g. disqus Testing completed (on m-c, etc.): just landed on m-c, but comes with a mochitest, and we have many other CSP tests that should catch regression. Risk to taking this patch (and alternatives if risky): possible CSP regressions, but see above re user impact and testing String or IDL/UUID changes made by this patch: none This doesn't need to go to beta because the bug fix that exposed this inconsistency with the spec first landed in Fx24.
Attachment #769948 - Flags: approval-mozilla-aurora?
Do we know if this has already landed in Nightly 25? The online commenting provider disqus says this bug is preventing nightly users from using their commenting system which is use by millions of sites. Just wondering?
@Benjamin, it's already landed in Nightly. (In reply to Benjamin Kerensa from comment #26) > Do we know if this has already landed in Nightly 25? The online commenting > provider disqus says this bug is preventing nightly users from using their > commenting system which is use by millions of sites. > > Just wondering?
(In reply to krzysztof.glebowicz from comment #27) > @Benjamin, it's already landed in Nightly. (In reply to Benjamin Kerensa > from comment #26) > > Do we know if this has already landed in Nightly 25? The online commenting > > provider disqus says this bug is preventing nightly users from using their > > commenting system which is use by millions of sites. > > > > Just wondering? Right, this should be in the 7/3 Nightly.
I just verified that disqus works for me on Windows with the 7/3 Fx25 Nightly using the test case from bug 886132 (http://edition.cnn.com/2013/06/22/politics/nsa-leaks/index.html)
Disqus is broken in the latest Aurora due to this bug. Is this something that will be uplifted into 24?
Waiting on approval for uplift (comment 25).
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #31) > Waiting on approval for uplift (comment 25). Thanks! </tldr>
Comment on attachment 769948 [details] [diff] [review] Patch 5 We definitely wan this fix and better sooner than later, approving for aurora.Based on your comment regarding possible fallouts, please add qawanted to get targeted qa testing that may be need here if you have any testcases in mind.
Attachment #769948 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to bhavana bajaj [:bajaj] from comment #33) > Comment on attachment 769948 [details] [diff] [review] > Patch 5 > > We definitely wan this fix and better sooner than later, approving for > aurora.Based on your comment regarding possible fallouts, please add > qawanted to get targeted qa testing that may be need here if you have any > testcases in mind. Thanks, I'll land it on aurora. Regarding test cases, the best thing to do would be to run through csptesting.herokuapp.com or the W3C test suite and hopefully see that our score improves :)
We actually need this on FF23 as well for bug 888172 to be able to land and have a spec compliant CSP 1.0 implementation so marking this affected on FF23 and we'll take this in our second-to-last Beta. I spoke with Garrett in IRC and the risk here is low due to this only touching CSP code and having tests for that area as well.
Attachment #769948 - Flags: approval-mozilla-beta+
Confirmed pre-1.0 behavior. Verified 1.0 correct behavior in FF23, 24, 25.
I'm still seeing this is 28.
(In reply to Benjamin Kerensa from comment #39) > I'm still seeing this is 28. Might have been a regression sometime between 25 and 28, there were definitely a few bugs in CSP very recently - can you file a new bug for this with steps to repro (assuming they are different from the ones above ?) Thanks !
Flags: needinfo?(bkerensa)
I'm checking on this with Disqus which still does not work on Nightly they previously said this bug was the reason.
Flags: needinfo?(bkerensa)
Did you hear anything from Disqus? Does the issue persist?
Flags: needinfo?(bkerensa)
(In reply to Florian Bender from comment #42) > Did you hear anything from Disqus? Does the issue persist? Nothing back from Disqus but the issues does not persist in Nightly (currently 32) which is what I run.
Flags: needinfo?(bkerensa)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: