Add security headers to minimize XSS damage #292

xPaw · 2016-05-01T17:28:04Z

This disallows execution of scripts from other domains, inline scripts and use of eval. Also adds X-Content-Type-Options: nosniff on all requests.

astorije · 2016-05-03T04:24:08Z

@xPaw, I rebased this to merge conflicts :-)
Will review very soon, tomorrow hopefully (I need to check a thing or two before).

astorije · 2016-05-05T05:10:22Z

src/server.js

@@ -91,6 +97,7 @@ function index(req, res, next) {
 			config
 		);
 		var template = _.template(file);
+		res.setHeader("Content-Security-Policy", "default-src *; style-src * 'unsafe-inline'; script-src 'self'; reflected-xss block");


I really like this policy! A few minor comments/questions:

Content-Security-Policy is defined on page-by-page basis. Any reason why you put it here instead of inside your allRequests function?

* is the default value for everything so we don't need to specify it for default-src. Was there a specific reason why you did so?

I'm not sure I fully understand reflected-xss block: is it to enforce Chrome's filter against executing code that exists in the URL (for example) onto all browsers that would not enforce it by default?

It's possible that some or all of the following directives could be added, provided that I'm not entirely wrong:

Any reason against connect-src 'self'; considering how the server connects to the client (at least currently)?

I think we could set child-src 'none'; object-src 'none'; as well, as I don't think we need framed content or Flash content or etc.

We can set form-action 'self'; as well, or maybe even form-action 'none'; as none of the forms get actually submitted.

We might be able to set frame-ancestors 'none'; but maybe that could restrict usage for some users? (Thinking of the integrations like Bytesized or Cozy or whatever, maybe someone does crazy stuff to display their apps?)

We can set media-src 'self' as we don't embed videos or similar (but we do load one sound from within the app)

I'm not sure there's any need to send CSP on all requests (all other requests besides index.html would be static content).

I'm not sure I fully understand reflected-xss block

reflected-xss : Instructs a user agent to activate or deactivate any heuristics used to filter or block reflected cross-site scripting attacks, equivalent to the effects of the non-standard X-XSS-Protection header.

I've also didn't set extra hardened directives because I don't know what we would want to allow for eventual plugins to do.

* is the default value for everything so we don't need to specify it for default-src

Explicitness. We could eventually set it to self instead, and configure other directives as needed.

I'm not sure there's any need to send CSP on all requests (all other requests besides index.html would be static content).

Yeah OK, we'll adjust later if necessary.
I know that we essentially only send index and then the websocket takes over, but I'm thinking that other fallbacks from Socket.io might be worth "policying". But again, I don't think Socket.io would send this policy even if we set if with all requests (because it does go through Express).

reflected-xss : Instructs a user agent to activate or deactivate any heuristics used to filter or block reflected cross-site scripting attacks, equivalent to the effects of the non-standard X-XSS-Protection header.

OK so basically it's unnecessary in Chrome because filters are already enabled, but I don't know for other browsers.

I've also didn't set extra hardened directives because I don't know what we would want to allow for eventual plugins to do.

Agreed, but what about hardening at the moment and adapting when packages are a reality (or start to be one)?
After all, it's not set in stone, and some directives of the policy might not change anyway (especially connect-src and form-action, maybe child-src).

Explicitness. We could eventually set it to self instead, and configure other directives as needed.

Good point, to keep in mind.

astorije · 2016-05-06T03:24:38Z

I'm essentially 👍 but I would prefer if we harden things as suggested in the comments before merging this PR. Again, nothing set in stone, totally OK to re-evaluate when plugins show up, but in the meantime, there is no need to keep the doors open.

astorije · 2016-05-06T03:28:54Z

For the record, I had to catch up on the details of CSP before reviewing this, and if someone is looking for a good intro, http://www.html5rocks.com/en/tutorials/security/content-security-policy/ is really good.

xPaw · 2016-05-06T08:09:30Z

I've added child-src 'none'; object-src 'none'; form-action 'none'; referrer no-referrer; and removed reflected-xss.

xPaw · 2016-05-06T08:10:00Z

client/js/libs/handlebars/parse.js

@@ -14,7 +14,7 @@ function uri(text) {
 			return url;
 		}
 		var split = url.split("<");
-		url = "<a href='" + split[0].replace(/^www/, "//www") + "' target='_blank'>" + split[0] + "</a>";
+		url = "<a href='" + split[0].replace(/^www/, "//www") + "' target='_blank' rel='noopener'>" + split[0] + "</a>";


https://mathiasbynens.github.io/rel-noopener/

Thought it would be a good idea to include along with referrer no-referrer

Great read! And great idea!

maxpoulin64 · 2016-05-08T03:14:29Z

Looks all good to me, 👍

@astorije did you still want to hold the merge?

astorije · 2016-05-08T04:42:01Z

Nop, let's do this!

Add security headers to minimize XSS damage

xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label May 1, 2016

astorije self-assigned this May 1, 2016

astorije force-pushed the xpaw/csp branch from 019c29c to 32ca930 Compare May 3, 2016 04:23

astorije added the Type: Security Security concern or PRs that must be reviewed with extra care regarding security. label May 4, 2016

astorije reviewed May 5, 2016
View reviewed changes

astorije added the second review needed label May 6, 2016

Add security headers to minimize XSS damage

d143542

xPaw force-pushed the xpaw/csp branch from 32ca930 to d143542 Compare May 6, 2016 08:09

xPaw reviewed May 6, 2016
View reviewed changes

astorije merged commit c7fb388 into master May 8, 2016

astorije deleted the xpaw/csp branch May 8, 2016 04:42

astorije removed the second review needed label May 8, 2016

astorije added this to the ★ Next Release milestone May 15, 2016

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017

Merge pull request thelounge#292 from thelounge/xpaw/csp

c031f0b

Add security headers to minimize XSS damage

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add security headers to minimize XSS damage #292

Add security headers to minimize XSS damage #292

xPaw commented May 1, 2016

astorije commented May 3, 2016

astorije May 5, 2016 •

edited

Loading

xPaw May 5, 2016

astorije May 6, 2016

astorije commented May 6, 2016

astorije commented May 6, 2016

xPaw commented May 6, 2016

xPaw May 6, 2016

astorije May 6, 2016

maxpoulin64 commented May 8, 2016

astorije commented May 8, 2016

Add security headers to minimize XSS damage #292

Add security headers to minimize XSS damage #292

Conversation

xPaw commented May 1, 2016

astorije commented May 3, 2016

astorije May 5, 2016 • edited Loading

Choose a reason for hiding this comment

xPaw May 5, 2016

Choose a reason for hiding this comment

astorije May 6, 2016

Choose a reason for hiding this comment

astorije commented May 6, 2016

astorije commented May 6, 2016

xPaw commented May 6, 2016

xPaw May 6, 2016

Choose a reason for hiding this comment

astorije May 6, 2016

Choose a reason for hiding this comment

maxpoulin64 commented May 8, 2016

astorije commented May 8, 2016

astorije May 5, 2016 •

edited

Loading