Enable strict CSP headers to sandbox against any possible XSS

[cjdelisle]

Modern browsers allow sending Content-Security-Policy headers and will prevent eval() or inline <script> tags as well as all of the variants of onload, onerror etc. which is a formidable barrier to XSS security breach.

This is an improvement suggestion which consists of removing any inline <script>, eval() or on* javascript (move these to remote loaded <script> and event handlers). Then begin sending CSP headers which (for script) allow 'self' and the urls of any content delivery networks which are used.

The benefit is peace of mind because modern browsers will disallow execution of XSS attack script so even if a vulnerability is discovered, the result the the vast majority of users is just an error in the console. Support: http://caniuse.com/#feat=contentsecuritypolicy

[lampholder]

This is P3 because it's not on our immediate roadmap, but it seems like a solid idea.

I don't have a real idea for the level of effort or return on investment for Riot Web, though - I'll poke some people for some input.

[btittelbach]

You've probably all read the articles where people are now successfully looking into finding remote code execution bugs in Electron apps. https://statuscode.ch/2017/11/from-markdown-to-rce-in-atom/

Would be good to know riot.im is prepared for that.

[ara4n]

We use CSP already in the media repository to stop content uploaded stuff containing XSS. Riot/Web itself isn't served with a CSP however, and unsure how Electron is configured. Bumping priority accordingly to investigate.

[ara4n]

marking as critical until proven otherwise

[rugk]

BTW to make sure each user of Riot uses it, I highly suggest to (also) use the in-HTML version of your CSP. (It's just a meta tag, so that is a nice enhancement)

I'll look into creating a CSP, which works with the current version. As always be aware, that CSPs are just additional security features, they can mitigate the attack, but they do not replace a real fix of an XSS attack or so, should one be found.

[rugk]

BTW it is not critical, but it is a very good (and highly suggested!) security enhancement.

[rugk]

E.g. a good step would be to do not use inline JS (i.e. don't include JS in HTML code). I think Riot works quite good with that, it just shows two errors in the console, but these seem to work:

Content Security Policy: Die Einstellungen der Seite haben das Laden einer Ressource auf self blockiert ("script-src 'self' 'unsafe-eval'"). Source: (function (ERROR) { const V8_STACK_....

And, of course, the window.vector_indexeddb_worker_script = 'bundles/070e2827d7275f44a591/indexeddb-worker.js'; which is included in script tags in the index.html.

[rugk]

Same with inline CSS. If you disable that it just breaks the whole styling.

The good news is, it works with a relatively strong JS prevention (script-src 'self')
The bad news, is, it uses object so I have to enable object-src. (#6165)

BTW a CSP checker can be found at https://csp-evaluator.withgoogle.com/.

[rugk]

Here is a suggestion for a CSP, which works for the current version:

Content-Security-Policy: "default-src 'none'; style-src 'self' 'unsafe-inline'; script-src 'self'; img-src 'self' data:; connect-src *; font-src 'self'; media-src 'self'; child-src https://scalar.vector.im usercontent.riot.im blob:; form-action 'self'; object-src 'self'"

It only allows https://scalar.vector.im as the integrations server, but that can be adjusted. To limit the connections to one Matrix/Identity server, you would have to adjust connect-src. object-src is allowed because of #6165

[cjdelisle]

IMO applying this rule goes a long way toward preventing XSS.

[ghost]

'unsafe-eval' seems to be needed too, without it CSP reports: call to eval() or related function blocked by CSP. 1 bundle.js:80

Another error CSP reports is this:
Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src host 'unsafe-eval'”). Source: window.vector_indexeddb.... 1 host:46

Any idea how to fix this error?

This is the header with unsafe-eval enabled and only self hosted Identity server enabled that I use:
Content-Security-Policy: "default-src 'none'; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-eval'; img-src 'self' data:; connect-src 'self'; font-src 'self'; media-src 'self'; child-src https://usercontent.riot.im blob:; form-action 'self'; object-src 'self'"

[rugk]

However, that block does not result in problems, as far as I see. So maybe only something unimportant is blocked.

[ghost]

I think it would be a good idea not to block parts of riot-web's code. Plus, I wonder why it's blocked in the first place. Scripts from that host should be allowed according to my CSP.

[rugk]

Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src host 'unsafe-eval'”). Source: window.vector_indexeddb.... 1 host:46

This here is a message about blocking inline JS (not recommend but could be allowed with 'unsafe-inline'). The one from the indes.html. It#s really only a single line – and this line does not seem to be important, because it just seems to work without it.

Of course, I agree, it would be better if that is moved in a JS file and so not blocked by the CSP.

[t3chguy]

I'm not sure that can be moved to js trivially because webpack. As I understand it that line is important so that indexeddb operations happen in a worker. Indexeddb is needed for things like e2e.

[rugk]

No, e2e and such stuff worked for me without that line. Maybe it saved the database somehow else/used some fallback, or so…

[t3chguy]

Post the console of the app starting up, it'll say if it used a fallback storage

[rugk]

Did never look at that exactly, but that also does not matter.

[t3chguy]

If its using fallback storage then it DOES matter as the fallback has race conditions due to being synchronous and localStorage backed which causes e2e to fail in certain circumstances

[rugk]

So finally the JS should just be moved to a single file or so. Can't that hard… it's one line, after all.

[t3chguy]

Except half of that line isn't JavaScript and is instead a build tool generator placeholder for webpack.
<%= htmlWebpackPlugin.files.js[i] %>
and as you can see, its part of the htmlWebpackPlugin which suggests it'll only work in a html file

[rugk]

Ah, that's bad then.

[indolering]

In my tests everything worked without unsafe-eval. Anyway, it's of course only one example. Feel free to add whatever you want and remove parts to get stricter and stricter by time.

It sounds like it won't be mainlined without that piece of the codebase working. Could you add a build step that moves the inline script to an external file? Again, the important piece here is to get a working CSP into the codebase so we can improve things incrementally, let's not make this a blocking issue.

[rugk]

Ah, so the Riot devs should add it, sure… as you've mentioned how this can be done before.

[indolering]

Ah, so the Riot devs should add it, sure… as you've mentioned how this can be done before.

I'm basically asking if you will make the pull request or do I have to?

[rugk]

No, I'll not do a PR.

[Nadrieril]

Is there a recommended CSP string I can safely use when selfhosting riot ? And should this be documented somewhere ?

[rugk]

@Nadrieril I've added mine/one in here.

It should preferably not be documented, but actually tweaked (if needed) and merged into the code.

[Nadrieril]

Ok, thanks ! That'd be very cool indeed

[indolering]

It should preferably not be documented, but actually tweaked (if needed) and merged into the code.

I would document and unit test each rule* to make sure it hasn't been borked! Declarative specs count as code 😄.

*This is technically unit testing and might require a service like Sauce Labs (which has free accounts for F/OSS projects).

[indolering]

@rugk Did you ever get around to that PR?

[rugk]

As explained earlier no. Especially, because I don't know it is what the riot devs want. (and how strict it should be, testing etc.)

[532910]

https://observatory.mozilla.org/analyze/riot.im
Grade F

[ara4n]

riot.im/app now has a CSP on it. however, we have script-src: unsafe-inline due to the webpack issue mentioned above - need to fix this before we can close it up (and add it to riot-web as an meta tag)

[aaronraimist]

For the record the current CSP is, as of ^ is

default-src 'none';
style-src 'self' 'unsafe-inline';
script-src 'self' 'unsafe-eval' 'unsafe-inline' https://www.recaptcha.net https://www.gstatic.com;
img-src * blob: data:;
connect-src *;
font-src 'self';
media-src * blob: data:;
child-src * blob: data:;
form-action 'self';
object-src 'self';
manifest-src 'self'

[ara4n]

...and we need to add the CSP as a meta tag or similar so that Electron picks it up.

[aaronraimist]

Related: #11610

[stoically]

Had a quick glance why unsafe-eval currently (v1.5.8) is required and found 18 instances in the final bundle. It seems there are 13 instances of new Function which function as fallback if the environment isn't an actual browser, so for those dropping unsafe-eval would probably be fine (in the browser not sure about desktop). Then there are 5 instances of eval which come from Modernizer trying to detect es5, not sure what's the impact of that not working.

fwiw, I'm using riot web (Firefox) without unsafe-eval for a while now and haven't noticed any problems - and the only CSP related error I saw so far is the Modernizer one at startup.

[jryans]

In addition to embedding the CSP as a meta tag in the app, we'd also like to remove unsafe-inline from script-src.

[t3chguy]

Splitting off a new issue to track getting rid of unsafe-eval which is currently required for WASM.