Skip to content

Add security headers#228

Merged
d00rman merged 1 commit intowikimedia:masterfrom
gwicke:security_headers
Apr 16, 2015
Merged

Add security headers#228
d00rman merged 1 commit intowikimedia:masterfrom
gwicke:security_headers

Conversation

@gwicke
Copy link
Member

@gwicke gwicke commented Apr 9, 2015

In modern browsers these headers help to avoid some of the most common
client-side security issues even if we missed something in our HTML and SVG
sanitization code.

Bug: https://phabricator.wikimedia.org/T95443

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 86.8% when pulling a427d79 on gwicke:security_headers into 42db7c4 on wikimedia:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 86.8% when pulling a427d79 on gwicke:security_headers into 42db7c4 on wikimedia:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 86.8% when pulling a427d79 on gwicke:security_headers into 42db7c4 on wikimedia:master.

@gwicke
Copy link
Member Author

gwicke commented Apr 9, 2015

/cc @d00rman @eevans @catrope @Stype

lib/server.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the same as Content-Security-Policy above, both having the same meaning / effect

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to http://caniuse.com/#search=csp the X-prefixed variant is still needed for IE 10 and 11. So yeah, should have the same value.

@gwicke gwicke force-pushed the security_headers branch from a427d79 to 7117b7a Compare April 9, 2015 15:01
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 86.81% when pulling 7117b7a on gwicke:security_headers into db93ea2 on wikimedia:master.

lib/server.js Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought restbase only returned json that was consumed by the frontend. Why are you allowing media, images, and styles from any domain to be included in the rendering?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RESTBase returns content to be used by any client. For example, I can pull up https://rest.wikimedia.org/en.wikipedia.org/v1/page/html/Cat and see styles & images loaded from upload and bits. We can narrow down the domains in the future, but this will need to be configurable per wiki.

@gwicke gwicke force-pushed the security_headers branch from 7117b7a to 137f5bf Compare April 9, 2015 17:56
@catrope
Copy link

catrope commented Apr 9, 2015

@Stype RESTbase doesn't just return JSON, it also returns HTML.

@gwicke We use iframes for HTML parsing in old browsers: DOMParser support came relatively late in Chrome and is only like 6 months old in Safari. We may consider dropping support for non-DOMParser browsers though. In any case, how we parse the HTML is independent of how we fetch it: we use jQuery XHR in plain-text mode, so as long as the browser supports CORS that should be fine. We should also investigate whether we have actually blacklisted all browsers that don't support cross-domain XHR.

@gwicke
Copy link
Member Author

gwicke commented Apr 9, 2015

@catrope: Yeah, makes sense. I went ahead & enabled X-Frame-Options with SAMEORIGIN policy after seeing that you are not going to be affected by that.

lib/server.js Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're only adding the CSP header to json, you shouldn't have the * modifiers (those are protection in case your json is sniffed to html by the browser).

I think you should default-src 'none' here, then have an else block that still adds these with a more liberal CSP (I would argue we should enumerate the domains instead of *) and without XFO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are only adding these headers to content that is not JSON. Are you aware of any clients that sniff JSON with explicit mime type and nosniff headers as HTML, but respect CSP headers?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did miss that. So for non json content, is there any case not covered by 'self' and *.wikimedia.org for style/img/media?

For the json case, you should still send nosniff and XFO: DENY. That will help with xssi. There are probably not cases where a browser will ignore nosniff but enfore CSP, but I think the question should be why not include them. If there's a valid reason, then leave it off.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for non json content, is there any case not covered by 'self' and *.wikimedia.org for style/img/media?

For our purposes that would probably be okay, but it definitely needs to be configurable. For example, it needs to be different in labs, staging & for all third-party users. That's why I said that we can do this in the next step, rather than right away.

For the json case, you should still send nosniff and XFO: DENY. That will help with xssi. There are probably not cases where a browser will ignore nosniff but enfore CSP, but I think the question should be why not include them. If there's a valid reason, then leave it off.

Do you see a realistic chance that a browser that's recent enough to respect those headers would sniff JSON as HTML, even with the explicit application/json content type? I'm not opposed to sending them (overhead should be small, especially with SPDY), just wonder if it really makes a difference in practice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, could you explain what XFO: DENY will do for JSON?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 86.83% when pulling 137f5bf on gwicke:security_headers into db93ea2 on wikimedia:master.

@gwicke gwicke force-pushed the security_headers branch from 137f5bf to cd5ec89 Compare April 10, 2015 15:41
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 86.83% when pulling cd5ec89 on gwicke:security_headers into db93ea2 on wikimedia:master.

@gwicke gwicke force-pushed the security_headers branch from cd5ec89 to 328532b Compare April 10, 2015 15:47
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 86.83% when pulling 328532b on gwicke:security_headers into db93ea2 on wikimedia:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 86.83% when pulling 328532b on gwicke:security_headers into db93ea2 on wikimedia:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 86.83% when pulling 328532b on gwicke:security_headers into db93ea2 on wikimedia:master.

@gwicke
Copy link
Member Author

gwicke commented Apr 13, 2015

@d00rman @eevans @Stype: Does this look okay to you?

@Stype
Copy link

Stype commented Apr 13, 2015

@gwicke, XFO:DENY for json should be the default. That prevents redressing attacks if there's ever a token or sensitive data served up by the api.

@gwicke
Copy link
Member Author

gwicke commented Apr 13, 2015

FTR, on IRC @Stype brought up the possibility of sensitive information in a JSON iframe being positioned as a 'captcha' that the user needs to enter. So, we should disallow framing in any case.

In modern browsers these headers help to avoid some of the most common
client-side security issues even if we missed something in our HTML and SVG
sanitization code.

Bug: https://phabricator.wikimedia.org/T95443
@gwicke gwicke force-pushed the security_headers branch from 328532b to d8a75f5 Compare April 13, 2015 20:36
@coveralls
Copy link

Coverage Status

Coverage increased (+0.11%) to 86.84% when pulling d8a75f5 on gwicke:security_headers into db93ea2 on wikimedia:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.11%) to 86.84% when pulling d8a75f5 on gwicke:security_headers into db93ea2 on wikimedia:master.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be set to DENY, and then if the response is not a JSON, set it to SAMEORIGIN in that case only?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your reasoning?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your reasoning?

Hm, I've re-read your comment:

FTR, on IRC @Stype brought up the possibility of sensitive information in a JSON iframe being positioned as a 'captcha' that the user needs to enter. So, we should disallow framing in any case.

So now I'm thinking we should have just:

rh[XFO] = DENY;

or am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should we protect against ourselves?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should :) It's what I understood from your comment about the IRC discussion, namely //So, we should disallow framing in any case//

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For third-party sites, yes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that any code that relies on framing a json response and presenting it to the user probably isn't the quality of code that should be running on our site. But if someone has a valid use case, then SAMEORIGIN will mitigate most issues.

@gwicke gwicke changed the title Add security headers in non-JSON responses Add security headers Apr 14, 2015
d00rman pushed a commit that referenced this pull request Apr 16, 2015
@d00rman d00rman merged commit 607b6bc into wikimedia:master Apr 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants