Open Bug 493857 (CSP) Opened 11 years ago Updated 1 month ago

Implement Content Security Policy

Categories

(Core :: Security, enhancement, P1)

enhancement

Tracking

()

People

(Reporter: bsterne, Unassigned)

References

(Depends on 17 open bugs, Blocks 2 open bugs, )

Details

(Keywords: doc-bug-filed)

Attachments

(2 files, 14 obsolete files)

18.61 KB, text/plain
Details
15.42 KB, application/x-javascript
Details
Implement the specification for Content Security Policy to mitigate code injection attacks.

Background information:
https://wiki.mozilla.org/Security/CSP
http://people.mozilla.org/~bsterne/content-security-policy/
Blocks: 485488
Umm... this part really worries me:
https://wiki.mozilla.org/Security/CSP/Spec#No_inline_JavaScript_will_execute

Specifically, breaking the onclick event listener and the <a
href="javascript:foo()"></a> functionality.  The onclick attribute is part of
the HTML 4.01 specification, and onclick has worked forever (DOM Level 0).

Also, your specification states: "The contents of internal <script> nodes" will
be restricted.  What's an internal script node (as opposed to an external one)?

Finally, under "Activation and Enforcement", I wonder what would happen were a
malicious script to do its dirty work, and then forcibly insert the meta tag.
(I'm not saying this is a problem - I'm just curious.)
First rough impl. of the CSP data structures.  Eventually will turn into a Javascript module, thus the .jsm extension.  Does not fetch external policies yet.
Here are some unit tests for the CSPUtils.jsm data structures and methods.  They were kind of hacked out quickly and we could surely use more cases to test.  These are meant to be stand-alone tests, and don't rely on any other source code except the CSPUtils.

To run them, install some JS shell or interpreter of some sort (I used rhino) and load this TestCSPUtils.js file in the interpreter.  Make sure the other (CSPUtils.jsm) file is in the same directory since it's referenced from the TestCSPUtils.js file.
Attachment #378464 - Attachment mime type: application/octet-stream → text/plain
(In reply to comment #1)
> Specifically, breaking the onclick event listener and the <a
> href="javascript:foo()"></a> functionality.  The onclick attribute is part of
> the HTML 4.01 specification, and onclick has worked forever (DOM Level 0).

It is, however, an opt-in mechanism.  We aren't breaking DOM across the board, only for sites that choose to use CSP.  You have highlighted, though, the biggest challenge for sites wanting to adopt CSP.  We plan to provide documentation and possibly tools to sites to help ease their transition.

> Also, your specification states: "The contents of internal <script> nodes" will
> be restricted.  What's an internal script node (as opposed to an external one)?

An internal <script> node is one in the top-level protected document.  An external <script> node is one in an externally-linked JavaScript file.

> Finally, under "Activation and Enforcement", I wonder what would happen were a
> malicious script to do its dirty work, and then forcibly insert the meta tag.
> (I'm not saying this is a problem - I'm just curious.)

Since the policy can only place additional "restrictions" on a page, we feel the risk of injected policy is very low.  For instance, and injected policy could prevent a resource's images or script files from loading, but couldn't de-escalate the security policy for a resource below the defaults (same-origin, etc.).
(In reply to comment #1)
> Specifically, breaking the onclick event listener and the <a
> href="javascript:foo()"></a> functionality.  The onclick attribute is part of
> the HTML 4.01 specification, and onclick has worked forever (DOM Level 0).

The highest priority goal of CSP is to help sites eliminate XSS, a problem which has been found to affect something like 80-90% of even the best-run sites. Funny you should mentions the onclick attribute as that one specifically is a popular one to abuse. Whether the burden of rewriting your site to the supported safe subset of HTML is worth it depends on how valuable the contents of your site are.

Note that we are not eliminating event handlers, just the ability to specify them inline. AddEventListener() will still work, as will setting the .click property of a DOM node. This is a little cumbersome, but there are already sites that do this for some of their content.

CSP is a gamble, it could be that the hurdle will turn out to be too high. But if we can get authors over that hurdle we can promise them a safer site.
Duplicate of this bug: 463948
Attached patch CSP work in progress (obsolete) — Splinter Review
Adds CSP object as part of nsDocument with stub for RefinePolicy.  Adds nsIContentPolicy which locates the CSP object on the document when ShouldLoad is called.
Attached patch CSP Work in Progress (v2) (obsolete) — Splinter Review
CSP work in progress (updated)

Ties CSP policy data structure objects and enforcement hooks all together.  With this patch, policies are loaded from the HTTP response header and parsed, then enforced.

This patch also supports policy-uri (though synchronously, and probably with a bit of UI lag).
Attachment #382041 - Attachment is obsolete: true
Attached patch CSP Work in Progress (v3) (obsolete) — Splinter Review
This patch is an upgrade from the v2 patch and includes a rough implementation of Policy Violation reporting (via asynchronous XHR POST) and frame-ancestor checking.
Attachment #382990 - Attachment is obsolete: true
Alias: CSP
Severity: normal → enhancement
Attached patch Incremental upgrade from v3 (obsolete) — Splinter Review
This patch includes:
- parsing the "inline" and "eval" keywords in the script-src directive
- suppresses cookies from requests sent for policy URI fetch and violation report sending
- converted hand-rolled unit tests to xpcshell tests (make -C caps/test xpcshell-tests)
Attachment #384026 - Attachment is obsolete: true
Spoke with sicking and jst.  Returning early from nsEventListenerManager::AddScriptEventListener prevents event listeners from being added due to on<event> attributes.  I posted a test for this behavior here:
http://hackmill.com/csp/tests/event-handling-attrs.php
Attachment #387470 - Attachment is patch: true
Attached patch CSP Work in Progress (v3.6) (obsolete) — Splinter Review
Updated patch to fix some URI scheme parsing issues.
Attachment #387098 - Attachment is obsolete: true
Attached patch CSP Work in Progress (v4) (obsolete) — Splinter Review
Adds "security.csp.enable" pref (default true) that can be set to false to disable CSP globally.  Also merges event listener patch with main WIP patch.
Attachment #387470 - Attachment is obsolete: true
Attachment #387569 - Attachment is obsolete: true
If this goes in 1.9.2 at all I think it needs to block the alpha.
Flags: blocking1.9.2+
Priority: -- → P1
Attached patch CSP Work in Progress (v4.1) (obsolete) — Splinter Review
Adds:
- Hooks into nsJSTimeoutHandler to turn off setTimeout() and setInterval() for string arguments (unless of course the CSP allows Eval stuff).
- Cleans out printf() statements, replacing them with PR_LOGGING stuff.  Still need to update the .js files for this, but can probably be done by changing CSPdebug, CSPError and CSPWarning.
Attachment #388837 - Attachment is obsolete: true
Comment on attachment 389817 [details] [diff] [review]
CSP Work in Progress (v4.1)

>+ * The Initial Developer of the Original Code is
>+ *   Sid Stamm <sid@mozilla.com>

No, the initial developer is MoCo or MoFo. You should just list yourself under contributors.
Attached patch CSP Work in Progress (v5) (obsolete) — Splinter Review
This patch includes miscellaneous fixes and rewriting of the parser.  Unit tests for CSPUtils.jsm have been heavily modified for correctness and to include "self" definitions where necessary.  (e.g., can't create a source "a.com" without knowing the scheme and port of 'self')  Highlights in this update from patch v4.1:
- Fixed license "initial developer" string (thanks reed, didn't know this was the common practice, now I do)
- 'self' keyword can only be stand-alone, not used used as anything other than scheme, host *and* port
- keywords are now quoted as per spec (e.g., 'none')
- host-less schemes (like data: and javascript:) are valid sources
- application/xml sent as MIME type for violation reports
- font-src and xml-src directives are now supported
- "report-only" variation based on the HTTP header name (https://wiki.mozilla.org/Security/CSP/Spec#Report-Only_mode)
Attachment #389817 - Attachment is obsolete: true
Per the platform meeting today, this is going to miss 1.9.2 and we don't want to take it after this week's beta, so this will have to wait for 1.9.3.
Flags: wanted1.9.2-
Flags: blocking1.9.2-
Flags: blocking1.9.2+
Attached patch CSP Work in Progress (v5.1) (obsolete) — Splinter Review
This patch includes miscellaneous fixes from v5.  Unit tests for CSPUtils.jsm have been fixed to support quoted keywords ('self', etc) and for correct behavior with source lists containing unidentifiable tokens.
- CSP parser enforces same-origin (scheme/host/port) for policy URI fetching
- CSP parser enforces ETLD+1 (public suffix + 1) matching for report URIs
- Variety of other minor fixes.
Attachment #390366 - Attachment is obsolete: true
Oops, previous attachment for v5.1 was incomplete.  This attachment fixes that.
Attachment #394402 - Attachment is obsolete: true
Attached patch CSP Work in Progress (v5.2) (obsolete) — Splinter Review
- fixed a few policy initialization bugs ('inline' and 'eval' were ignored due to the way the policy was intersected with "allow *" initially)  'inline' and 'eval' are now digested properly
- added checking on link clicks to block javascript: URIs when 'inline' is not specified in the policy
Attachment #394403 - Attachment is obsolete: true
Here is a proof of concept for a workaround to the redirects-don't-call-into-Content-Policy problem.  This patch only implements the restrictions for image loading, so a lot of other code would need to be added to handle all the other types of loads.

The basic idea is to let all new channel creation go through a helper function, NewChannelIfPolicyOK, which works like NS_NewChannel but also takes a CSP object and a load type.  The CSP and load type are added to the initial channel's property bag when it's created.  These can be propagated forward as a channel redirects and can be used at each hop to decide whether or not to allow the redirect.
Attached patch CSP Work in Progress (v5.3) (obsolete) — Splinter Review
This new version is a rewrite:
- Completely rewrote the CSP parser ... it's now not as fragile and easier to read
- Reviewed and revised unit tests so they reflect the spec
Attachment #394589 - Attachment is obsolete: true
Depends on: 515433
Depends on: 515437
Depends on: 515442
Depends on: 515443
Depends on: 515458
Depends on: 515460
Attachment #396000 - Attachment is obsolete: true
Patches to deploy CSP are now split out into more bite-size pieces for the bits of functionality involved with what we call "CSP."  As a result, the patches in this bug are invalid: see the bugs this one depends on.
Status: NEW → ASSIGNED
I think someone should try posting about this at http://connect.microsoft.com/feedback/default.aspx?SiteID=136

With some luck this might make into ie too fairly soon.
Depends on: CVE-2010-0182
Depends on: 544061
Blocks: xss
Depends on: 548193
Depends on: 548949
Depends on: 548984
Depends on: 550442
Blocks: clickjacking
Depends on: 552523
Depends on: 556625
Depends on: 558429
Depends on: 558431
Comment on attachment 395460 [details] [diff] [review]
Redirect handling PoC - applies to v5.2

Redirects in CSP are handled in bug 515797, bug 523239, and bug 515460.
Attachment #395460 - Attachment is obsolete: true
Depends on: 560574
Depends on: 561051
Depends on: 570017
Depends on: 570505
We have initial documentation here:

https://developer.mozilla.org/en/Introducing_Content_Security_Policy

What more is needed?
Depends on: 576200
Depends on: 578075
Depends on: 594446
Depends on: 600584
Depends on: 602063
Awesome, sheppy.  I'll review the docs tomorrow.
I've added the doc on the policy violation reports here:

https://developer.mozilla.org/en/Security/CSP/Using_CSP_violation_reports

Paul has given the docs a once-over, and I've emailed a few questions to bsterne. Other than that, and bsterne's review, these are complete.
Depends on: 604177
Depends on: 606039
Depends on: 607067
Depends on: 607069
Depends on: 608131
Depends on: 615707
Depends on: 615708
Depends on: 615711
No longer depends on: 615707
Depends on: 617195
Depends on: 631040
Depends on: 634773
Depends on: 634778
Depends on: 609748
Depends on: 638320
Depends on: 639533
Depends on: 650386
Depends on: 671389
Depends on: 672961
Depends on: 673645
Depends on: 737064
Depends on: 746978
Depends on: 612391
Depends on: 766536
No longer depends on: 766536
Depends on: 766536
Depends on: 766569
Depends on: 784315
Depends on: 779918
Depends on: 663570
Depends on: 788337
Depends on: 792214
Depends on: 792542
Depends on: 687086
Depends on: 802872
Depends on: 802905
Depends on: 805929
Depends on: 808292
Depends on: 809982
Depends on: 809983
Depends on: 702176
Depends on: 832398
Depends on: 801783
Depends on: 832558
Depends on: 663567
Depends on: 792161
Depends on: 837682
Depends on: 843311
Depends on: 846978
Depends on: 847081
Depends on: 847067
Component: DOM: Core & HTML → Security
Depends on: 529697
Depends on: 855326
Depends on: 858789
Depends on: 858787
Depends on: 858780
Depends on: 842657
Depends on: 864675
Depends on: 866522
Depends on: 841402
Depends on: 722547
Depends on: 826805
Depends on: 886164
Blocks: 836922
No longer blocks: 836922
Depends on: 836922
Depends on: 741019
Depends on: 909029
Hi Bradon. I want to test this feature but don't really know where to start from. Can you please provide any guildline please so I can create a test plan and some test cases for sign-off.
Flags: needinfo?(brandon)
Hi Bradon. I want to test this feature but don't really know where to start from. Can you please provide any guildline so I can create a test plan and some test cases for sign-off.
Mihai: Brandon is no longer actively working on this.

Here's the specification for the feature: http://www.w3.org/TR/CSP/

We have many tests for this in our mochitest suite already.  This is a metabug, please look at all the blocking bugs to see the real work.
Assignee: brandon → nobody
Flags: needinfo?(brandon)
Thanks Sid. If there are any test plan or additional test cases required please let me know.
QA Contact: mihai.morar
I ran on local machines following Run Tests and got 23 failures. Results: (164/187)

Test Runs:
http://csptesting.herokuapp.com/

Failures: Same failures for Windows 7 x64, Ubuntu 13.04 x86 and Mac OS 10.8 on FF24b7 BuildID: 20130829135643

13	Style in data-uri allowed
15	Use inline styles
17	Use inline style attributes
61	Style wants image, and allowed by img-src
78	Load embed from default-src 'self'
80	Load embed from object-src 'self'
83	Load embed from object-src with redirect from allowed to allowed
84	Load embed from default-src csptesting.herokuapp.com
89	Load embed from object-src with redirect from allowed to allowed
106	Load font from default-src 'self'
108	Load font from font-src 'self'
111	Load font from font-src with redirect from allowed to allowed
112	Load font from default-src csptesting.herokuapp.com
114	Load font from font-src csptesting.herokuapp.com
117	Load font from font-src with redirect from allowed to allowed
150	Load xhr from connect-src with redirect from allowed to allowed
156	Load xhr from connect-src with redirect from allowed to allowed
165	Load WebSockets from default-src ws://csptesting.herokuapp.com
167	Load WebSockets from connect-src ws://csptesting.herokuapp.com
171	SVG - scripting event handler
183	Sandbox
185	Sandbox

Any idea why all these test are failling?
Flags: needinfo?(sstamm)
There could be many reasons.  See all the bugs blocking this (and blocking the bug aliased to csp-w3c-1.0)?  Some of those might explain various failures.

Also, there could possibly be bugs in the tests. Not sure without digging into them more.

Garrett: you've probably looked at this stuff more recently than me, do you have any insight or should we file a bug to follow up with all these test failures?
Flags: needinfo?(sstamm) → needinfo?(grobinson)
I've triaged all of the test failures from http://csptesting.herokuapp.com/. No new bugs need to be filed :)

13-117 all failed because the iframes used to load the test requests had 'style="display:none"', to avoid cluttering up the page. However, FF does not compute style (or perform certain other rendering tasks) on elements that are/are children of 'display:none'. These test failures were false negatives. They were addressed (swiftly!) by eoftedal in https://github.com/eoftedal/csp-testing/issues/6

150-167 were all failing because they were performing cross-domain XHR without CORS properly configured (these tests failed on Chrome as well). This was a bug in the test suite, and was addressed by eoftedal in https://github.com/eoftedal/csp-testing/issues/7

171 failed because it tests the execution of script in an onload event handler in a <g> child of an <svg> element. FF's SVG code purposely does not implement the "load event dispatched on every element" behavior for performance reasons. For context (thanks dholbert!), see

* https://bugzilla.mozilla.org/show_bug.cgi?id=552938#c27
* https://bugzilla.mozilla.org/show_bug.cgi?id=639950

Finally, 183 and 185 fail because we have not yet implemented the sandbox directive (optional in 1.0). See Bug 671389.

eoftedal quickly fixed the issues in the test suite. I now see 180/187 passing tests, 3 of which are 171, 183, and 185. I will further triage the remaining 4 tests.
Flags: needinfo?(grobinson)
Depends on: 903080
Depends on: 916054
Depends on: 918397
Depends on: 919209
Depends on: 918724
Depends on: 925186
(In reply to Garrett Robinson [:grobinson] from comment #38)
> I've triaged all of the test failures from http://csptesting.herokuapp.com/.

I see 180 test passing in 25b6, but Nightly 27.0a1 (2013-10-11) only passes 142/187! Which version did you try this with? What's the reason for the regression?
Flags: needinfo?(grobinson)
(In reply to Florian Bender from comment #39)
> (In reply to Garrett Robinson [:grobinson] from comment #38)
> > I've triaged all of the test failures from http://csptesting.herokuapp.com/.
> 
> I see 180 test passing in 25b6, but Nightly 27.0a1 (2013-10-11) only passes
> 142/187! Which version did you try this with? What's the reason for the
> regression?

Could be bug 925186 or could be other regressions from bug 836922 possibly.
I just built mozilla-central, built and confirmed 142/187.  When I applied the fixes for bug 925186 and bug 924708 I get 180/187.  I probably regressed all the things with multipolicy support, but looks like we found most of 'em.
Flags: needinfo?(grobinson)
Thanks Sid for your feedback. I get Results: (142/187), same results as you got in Comment 41. I had used Windows 7 x64 and Latest Aurora 26 for testing. 

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 
BuildID: 20131014004003
Garrett, are there any additional tests required for testing this feature on it's enought to confirm that the fixed bugs from "Depends On" section are verified?
Depends on: 928719
No longer depends on: 928719
Depends on: 929653
(In reply to Mihai Morar, QA (:MihaiMorar) from comment #43)
> Garrett, are there any additional tests required for testing this feature on
> it's enought to confirm that the fixed bugs from "Depends On" section are
> verified?

All of the fixed bugs from "depends on" should have accompanying tests. Most of these are in content/base/test/csp. There are some tests related to Web Console logging in browser/devtools/webconsole/test.
Depends on: 933413
Depends on: 935690
Depends on: 938652
Depends on: 942345
Depends on: 925004
Depends on: 915824
Depends on: 951457
Depends on: 587377
Depends on: 957980
Depends on: 963668
Depends on: 605900
Depends on: 965273
Depends on: 883975
Depends on: 965727
Depends on: 984808
Depends on: 991972
Depends on: 964276
Depends on: 991474
Depends on: 991468
Depends on: 994782
Depends on: 994872
Depends on: 1014545
Depends on: 1027833
Depends on: 1027868
Depends on: 993477
Depends on: 1011841
Duplicate of this bug: 361915
No longer blocks: 390910
Duplicate of this bug: 390910
Depends on: 1048048
Depends on: 1037768
Depends on: 1100630
Depends on: 1106775
Depends on: 878608
Depends on: 1265316
No longer blocks: 1024557
Depends on: 1387807
Depends on: 1389874
Depends on: 1441514
Depends on: 1479516
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
No assignee, updating the status.
Depends on: 1631737
Depends on: 1632840
You need to log in before you can comment on or make changes to this bug.