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. (https://github.com/vector-im/riot-web/issues/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 https://github.com/vector-im/riot-web/issues/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]

I'll look into creating a CSP, which works with the current version.

Apply the CSP that you have come up with to prevent further technical debt and break this ticket up into further refinements.

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.

AFAIK CSP is supposed to be the fundamental fix for XSS attacks. If you lockdown all of your scripts using CSP, then an attacker has to find a browser bug to get around the CSP filter. I was pretty ... dismissive of Riot's commitment to security when I found out it didn't use CSP.

[indolering]

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

Just add a build step to push all inline JavaScript to external files. WebPack apparently supports CSP nonces natively.

[rugk]

AFAIK CSP is supposed to be the fundamental fix for XSS attacks. If you lockdown all of your scripts using CSP, then an attacker has to find a browser bug to get around the CSP filter.

E.g. there are still older browsers that do not support CSP. Additionally, when an XSS exploit is found, maybe an attacker can also find a way to circumvent your CSP. You just can never be 100% sure that your CSP mitigates (note I do not say "prevents" here deliberately) all XSS attacks. Especially as you cannot disallow everything and you just need to allow something for your application to work. Depending on your application this can be more or (better) less, but it may never be 100% "XSS-proof". At least, you cannot easily judge it. That said, one thing is for sure: A good CSP likely prevents many XSS attacks.

[cjdelisle]

@rugk I disagree, IMO a good CSP policy is a water-tight solution to XSS attacks, except on old browsers which probably have known vulnerabilities anyway. Furthermore, a good CSP policy makes XSS attacks much less interesting because they only target the 1% of users who refuse to upgrade their Windows XP machine, so attackers may never bother weaponizing them. Finally, a CSP policy can be configured to report violations to a server, so any attempted attack will be detected as soon as it is seen by a modern browser. Unsanitized html injection should still be considered a bug, but with CSP it is absolutely not something to panic over.

[rugk]

All you say is true, sure. Except that it is no "water-tight solution", because of one simple thing:

IMO a good CSP policy is a water-tight solution to XSS attacks,

How do you define "good"? What is the threshold at which a policy is not good anymore?

And yes, you can say you mean these with unsafe, but that may not be enough. Because, sure, preventing simple HTML injection is easy with CSPs if done properly, but there are many more ways to theoretically and - if a clever attacker/security researcher discovers it - practically exploit it. See this Stackexchange answer and this here (or real-life CSP bypasses even for very string CSPs like these with nounce) for details.

So yes, you may not need to panic - that does not help anyway, in most cases -, but XSS is a big topic. As such, you still need to treat any XSS vulnerability as a vulnerability - because it is one. A CSP is one layer of defense, in this case likely the last one, but that must not prevent you from fixing the underlying vulnerabilities, even if they are migrated by a CSP.

[indolering]

This has been a fun discussion on the state of XSS prevention, but can we all agree that CSP is super important and move on to implementing this?

@rugk, can you fix your CSP policy by adding unsafe-eval and get it mainlined? Then we can start new tickets targeting each bad policy and prevent further security regressions.

[rugk]

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.

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