Add CSP nonce-source support


(Leo McArdle) #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.


(Jonathan Claudius) #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.


(Sam Saffron) #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.


(Leo McArdle) #4

Sure, done in: GitHub - LeoMcA/discourse at csp-nonce-source

The code is pretty straightforward, most of the changes are just adding the nonce attribute to all the tags: Comparing discourse:master...LeoMcA:csp-nonce-source · discourse/discourse · GitHub

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! Observatory by Mozilla)

Open the developer console to see the CSP in action:

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.


(Sam Saffron) #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.


(Leo McArdle) #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: Comparing discourse:master...LeoMcA:csp · discourse/discourse · GitHub

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:


(Leo McArdle) #9

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

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.


(Jeff Atwood) #10

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


(Leo McArdle) #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.


(Sam Saffron) #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.


(Leo McArdle) #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: