493857 - (CSP) Implement Content Security Policy

Status: NEW

(Core :: Security, task, P1)

Description

[Brandon Sterne (:bsterne)]

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/

Comment 1

[Alex Vincent [:WeirdAl]]

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

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP data structures & policy methods

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.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Attachment: unit tests for the CSPUtils.jsm objects

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.

Comment 4

[Brandon Sterne (:bsterne)]

(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.).

Comment 5

[Daniel Veditz [:dveditz]]

(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.

Comment 7

[Brandon Sterne (:bsterne)]

Attachment: CSP work in progress

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.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Work in Progress (v2)

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

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Work in Progress (v3)

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.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Attachment: Incremental upgrade from v3

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)

Comment 11

[Brandon Sterne (:bsterne)]

Attachment: no event listeners for event-handling attributes

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

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Work in Progress (v3.6)

Updated patch to fix some URI scheme parsing issues.

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Work in Progress (v4)

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.

Comment 14

[Benjamin Smedberg]

If this goes in 1.9.2 at all I think it needs to block the alpha.

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Work in Progress (v4.1)

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.

Comment 16

[Reed Loden [:reed]]

Comment on attachment 389817 [details] [diff] [review]
CSP Work in Progress (v4.1)

>+ * The Initial Developer of the Original Code is
>+ *   Sid Stamm <[e-mail]>

No, the initial developer is MoCo or MoFo. You should just list yourself under contributors.

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Work in Progress (v5)

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)

Comment 18

[Benjamin Smedberg]

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.

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Work in Progress (v5.1)

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.

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Work in Progress (v5.1 - repaired)

Oops, previous attachment for v5.1 was incomplete.  This attachment fixes that.

Comment 21

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Work in Progress (v5.2)

- 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

Comment 22

[Brandon Sterne (:bsterne)]

Attachment: Redirect handling PoC - applies to v5.2

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.

Comment 23

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Work in Progress (v5.3)

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

Comment 24

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 25

[Cork]

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.

Comment 26

[Sid Stamm [:geekboy or :sstamm]]

Related bug in webkit:
https://bugs.webkit.org/show_bug.cgi?id=30081

Comment 27

[Brandon Sterne (:bsterne)]

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.

Comment 28

[Eric Shepherd [:sheppy]]

We have initial documentation here:

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

What more is needed?

Comment 29

[Eric Shepherd [:sheppy]]

Lots of work done during the doc sprint today on CSP documentation. Would someone like to review it?

https://developer.mozilla.org/en/Security/CSP/Introducing_Content_Security_Policy
https://developer.mozilla.org/en/Security/CSP/Default_CSP_restrictions
https://developer.mozilla.org/en/Security/CSP/CSP_policy_directives
https://developer.mozilla.org/en/Security/CSP/Using_Content_Security_Policy

I'll be writing about CSP reports tomorrow.

Comment 30

[Brandon Sterne (:bsterne)]

Awesome, sheppy.  I'll review the docs tomorrow.

Comment 31

[Eric Shepherd [:sheppy]]

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.

Comment 32

[Mihai Morar, (:MihaiMorar)]

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.

Comment 33

[Mihai Morar, (:MihaiMorar)]

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.

Comment 34

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 35

[Mihai Morar, (:MihaiMorar)]

Thanks Sid. If there are any test plan or additional test cases required please let me know.

Comment 36

[Mihai Morar, (:MihaiMorar)]

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?

Comment 37

[Sid Stamm [:geekboy or :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?

Comment 38

[Garrett Robinson [: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.

Comment 39

[Florian Bender]

(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?

Comment 40

[Ian Melven :imelven]

(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.

Comment 41

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 42

[Mihai Morar, (:MihaiMorar)]

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

Comment 43

[Mihai Morar, (:MihaiMorar)]

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?

Comment 44

[Garrett Robinson [:grobinson]]

(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.

Comment 47

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 48

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 49

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.