Add CSP nonce-source support

dev
1

While moving all inline scripts externally would take a tremendous amount of effort, as far as I can see, adding nonce-source support would be relatively trivial.

nonce-source works by adding a nonce attribute to any <script> tags you want to run, which is a base64 string randomly generated on each page load. This nonce is also added to the CSP header, and only scripts with the correct nonce set are allowed to run.

This means inline scripts can be whitelisted without the need to whitelist all inline scripts with unsafe-inline, and external scripts can be included without the need to whitelist them explicitly in the header (which is useful for themes loading scripts from an external CDN).

More info:

To achieve this in Discourse we could generate the nonce on every page load in ApplicationController, add it to request.env so we can insert it where we want, and set the appropriate CSP header (or a custom header so CSP headers can be written in the nginx config).

I believe this addresses the problems identified before:

This, along with the other .html.erb partials containing inline scripts could simply include the nonce attribute within their script tags.

app/helpers/application_helper#theme_lookup seems to be the method used for inserting themesā€™ head_tags. It could be extended to parse the html to be inserted, and add the nonce attribute to any script tags.

This could sit behind a site preference, so sites relying on plugins which donā€™t update to work with nonces donā€™t break them.

From the previous CSP feedback, and my own grepping of places <script> tags appear in the codebase, I canā€™t see what else would have to be done to make Discourse compatible with nonce-source.

Is there anything Iā€™ve missed? Would a PR be welcome to implement this?

I havenā€™t yet thought through how this could be tested to ensure <script> tags lacking a nonce arenā€™t added in future.

5 Likes
2

I caught up with Leo out of band, but wanted to capture my understanding in my own words to help with clarity. First, I think this is a good step for making it easier for Discourse instance owners to strengthen their CSP policies, which would be awesome.

Pain Point / Problem:

Discourse implementation owners that wish to deploy CSP for their instance have to add ā€˜unsafe-inlineā€™ to their CSP policies to get it to work. This addition nearly invalidates the value of having a CSP policy if an XSS vector was to present itself in that context.

Hereā€™s my interpretation of thisā€¦

1.) (PROPOSAL FROM LEO) All inline script content would contain nonce-source (this eliminates the need for using ā€˜unsafe-inlineā€™ in a CSP policy to support core Discourse functions and gives the CSP policy meaning/value for inline XSS contexts)

2.) (ANOTHER THING TO CONSIDER - May be out of scope of Leoā€™s proposal) Upstream would need to consider what the default preference would be for external script content loads (thereā€™s always a few and might be subject to change, and maybe that gives people heartburn). My suggestion would be to be permissive for external loads in upstream by default and have some security guidance that suggests how someone could dial it in and be very explicit about their external resources and fully lock down the CSP policy.

5 Likes
3

I do not know, I am not saying pr-welcome quite yet, but do a local proof of concept in a branch to prove that this will actually work.

2 Likes
4

Sure, done in: https://github.com/LeoMcA/discourse/tree/csp-nonce-source

The code is pretty straightforward, most of the changes are just adding the nonce attribute to all the tags: https://github.com/discourse/discourse/compare/master...LeoMcA:csp-nonce-source

Iā€™ve spun up a temporary instance to show the CSP working in practice: https://discourse-csp-nonce.mozilla.community/

(which gets an A from Mozilla Observatory! Mozilla Observatory)

Open the developer console to see the CSP in action:

Screenshot%20from%202018-07-30%2017-11-29
Screenshot%20from%202018-07-30%2017-11-29837Ɨ173 17.6 KB

Just for this proof of concept Iā€™m not adding the nonce attribute to themeā€™s body_tag to show how this greatly reduces the risk of XSS.

7 Likes
5

@xrav3nz when you get started this is one of the first things I would like you to look at.

What I want to be EXTRA DUPER careful about here is

  • Any possible plugin / theme impact
  • Any performance impact
  • Any issues with partial caching

I would like to get this insurance policy in, but we are going to have to take the time we take.

5 Likes
8

Excellent!

Iā€™m currently in the process of cleaning up my code, fixing all the tests I broke, and adding some tests of my own. You can follow along in this branch: https://github.com/discourse/discourse/compare/master...LeoMcA:csp

Thereā€™s a few seemingly nutty things going on, Iā€™ll explain myself once Iā€™m done.

Hereā€™s what will roughly be left to do after Iā€™ve finished this current work (which I hope to do this week):

  • Decide upon a default CSP policy
    • Iā€™ll consult with our resident CSP expert within Mozilla
  • Get nginx to fail safe with a restrictive CSP when no CSP is set by rails
  • Write comprehensive documentation for admins wanting to secure their instance as much as possible
    • The default Iā€™m working on right now should ā€œjust workā€ for most instances, but thereā€™s always edgecases upon edgecases

@sam, if you want this placed behind a preference to begin with, so we can see what sort of real world performance impact itā€™ll have (by testing it on Mozilla Discourse), thatā€™s fine by me :slight_smile:

4 Likes
9

I got the :broom: out and cleaned up my proof of concept:

https://github.com/discourse/discourse/pull/6282

Iā€™ve left a number of comments on the PR to explain some of the things Iā€™ve done, but one thing I want to call out here is the (current) default CSP of:

script-src 'unsafe-eval' 'nonce-%{nonce}' %{host}/mini-profiler-resources/ %{host}/service-worker.js %{cdn}/javascripts/;

Hereā€™s my explanation for each of the declarations:

  • 'unsafe-eval' - needed to execute ember.js
  • 'nonce-%{nonce}' - needed to the vast majority of scripts on the page
  • %{host}/mini-profiler-resources/ - needed to execute the mini profiler, it exists outside of Discourse so we canā€™t nonce it
  • %{host}/service-worker.js - again something we canā€™t nonce
  • %{cdn}/javscripts/ - needed to load some scripts ace.js loads itself, which is behaviour other libraries might copy

If youā€™ve read up on CSP, you might be wondering ā€œwhy @LeoMcA, why havenā€™t you used ā€˜selfā€™ and ā€˜strict-dynamicā€™ to make that policy shorter, simpler and sweeter?ā€.

The tl;dr is because Safari is a massive pain in the :peach:.

On not including 'self'

By default usersā€™ uploads will be accessible from the same host as the discourse site itself, and so they will be white-listed by any CSP including 'self'. Unfortunately what this means is that I as an attacker can upload a script file masquerading as another, allowed, file type for upload (like a pdf). If I were to point to it from the src of a <script> tag, a browser is liable to ignore the MIME type sent by the server, and ā€œsniffā€ out a more appropriate type, executing the script.

In this instance, 'self' essentially makes the CSP useless. An XSS vulnerability could just be exploited by pointing to a script the attacker has uploaded to the discourse instance, instead of adding inline code.

Thankfully thereā€™s a header to prevent MIME type sniffing X-Content-Type-Options.

But Safari is the only major browser not to support it.

On not including 'strict-dynamic'

'scrict-dynamic' allows any script loaded by a nonced script to be loaded, which is very helpful. Unfortunately both Edge and Safari donā€™t support it.

9 Likes
10

Are they slated to support it? We could wait until that happens if it simplifies things.

11

My ire may have been slightly misdirected at Safari, as from some googling it seems like some versions of Safari do support it.

The real problem is X-Content-Type-Options isnā€™t a standardised header (it was introduced by Microsoft in IE 8), so even among those browsers that do support it, its supported in different ways. Its best that it be set to nosniff, but it shouldnā€™t be relied upon, and other mitigations (like a very specific CSP policy) should be put in place.

12

Given that we are now a lot bigger than what we were 2 years ago, and can afford resources here, I would prefer to do this ā€œthe proper wayā€ ā„¢.

This would involve:

  • Changing the theme system so it pulls out scripts and ships them as static payloads instead of inline (which is great overall cause themes can ship more JS with less impact)

  • Changing the ā€œdynamicallyā€ generated JavaScript so we simply render data attributes on an element and then fish it out in a static script.

It is a biggish task but I feel it is manageable now and @xrav3nz can take it (and coordinate with @Osama the theme changes needed)

I much prefer this to the nonce approach cause we donā€™t need code to generate nonces and dynamically add nonces which is just busywork and it is clearly not at clean as a standard ā€œno inline JSā€ approach which is also far easier to explain and maintain longer term.

15 Likes
13

Cool, that sounds sensible to me.

This approach means thereā€™s much less CSP ā€œspecial sauceā€ going on, so my experience getting knee deep in the stuff is less valuable, but @xrav3nz and @Osama if you want to run anything past an extra pair of eyes, please feel free. :slight_smile:

8 Likes
14

closing this now that we have proper CSP merged in thanks to @xrav3nz and are inching centimeters from turning on full on CSP in meta!

9 Likes