New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug 948151: Add permissive CSP #5002

Merged
merged 1 commit into from Oct 10, 2018

Conversation

Projects
None yet
4 participants
@jwhitlock
Member

jwhitlock commented Sep 24, 2018

Builds on PR #5001, which can be merged first to get the libraries into the base image.

Add a CSP configuration, based on testing several pages on development. Enabling CSP and other options are configured from the environment, and off by default. The policy is configured in the Django settings, and not configured by the environment. The policy allows inline CSS and JavaScript, so that the site works without modification, but doesn't add as much security as it could.

CKEditor 4.5.10 still requires unsafe-eval, but 4.7 claims to remove this requirement. A decorator adds unsafe-eval only on the editing pages that use CKEditor.

The reporting view is copied from mozilla/bedrock, and it used to forward violation reports to Sentry. There were no tests to copy.

@jwhitlock jwhitlock requested review from jgmize and escattone Sep 24, 2018

@jwhitlock jwhitlock force-pushed the jwhitlock:csp-948151 branch from d0662ac to 2e08bca Sep 24, 2018

bug 948151: Add CSP config, reporting endpoint
Add a CSP configuration, based on testing several pages on development.
Enabling CSP and other options are configured from the environment, and
off by default. The policy is configured in the Django settings, and not
configured by the environment. The policy allows inline CSS and
JavaScript, so that the site works without modification, but doesn't
add as much security as it could.

CKEditor 4.5.10 still requires unsafe-eval, but 4.7 claims to remove
this requirement. A decorator adds unsafe-eval only on the editing
pages that use CKEditor.

The reporting view is copied from mozilla/bedrock, and it used to
forward violation reports to Sentry. There were no tests to copy.

@jwhitlock jwhitlock force-pushed the jwhitlock:csp-948151 branch from 2e08bca to abf6061 Oct 2, 2018

@escattone

I enjoyed reviewing this, as it forced me to dig into and understand the CSP header. I first updated my Docker images (mainly to pickup the new Kuma base image containing the required django-csp package), and then ran this locally with the following added to my .env:

MDN_CONTRIBUTION=True
MDN_CONTRIBUTION_CONFIRMATION_EMAIL=True
CSP_ENABLE_MIDDLEWARE=True
CSP_REPORT_ENABLE=True

I verified the Content-Security-Policy header, and then wanted to test both the new CSP reporting endpoint as well as the need for some of the default CSP settings.

  • I removed 'unsafe-inline' from both CSP_SCRIPT_SRC and CSP_STYLE_SRC, and verified that a POST to the CSP reporting endpoint was made with the proper details
  • I removed data: from CSP_IMG_SRC, and again verified that the proper CSP violation was reported
  • With everything back to normal, I logged in, and then noticed that I was getting a CSP violation for http://i2.wp.com violating the img-src directive (which was triggered by the call to https://secure.gravatar.com -- i2.wp.com is an image CDN). I discovered that this was due to the fact that I was running in non-SSL mode. When I switched to SSL mode by adding COMPOSE_FILE=docker-compose.yml:docker-compose.ssl.yml to my .env file, the CSP violation disappeared.
  • Remaining within SSL mode, I also verified the need for the 'unsafe-eval' directives added to the wiki.edit and wiki.translate endpoints, by removing them and ensuring that the proper CSP violation was reported for each endpoint.

This looks great and worked for me locally. I would like to eventually see all of the CSP reporting data embedded in our Sentry report, but if we decide to do that we can do that in a future PR. I assume the next step is to enable CSP but only in reporting mode, with these settings in an infra PR:

CSP_ENABLE_MIDDLEWARE=True
CSP_REPORT_ENABLE=True
CSP_REPORT_ONLY=True

and then, over the course of a week or so, see if we've missed anything in terms of the individual CSP policy directives via the reports to Sentry?

Thanks @jwhitlock!

# Incomplete CSP report
return HttpResponseBadRequest('Incomplete CSP Report')
client.captureMessage(message='CSP Violation: {}'.format(blocked_uri),

This comment has been minimized.

@escattone

escattone Oct 9, 2018

Member

I was thinking here that if we're not going to send the entire csp_data chunk to Sentry (which I think would be nice), it might be good to at least extract and add the csp_data['csp-report']['violated-directive']? I suppose that in most cases it's obvious from the blocked_uri, so we can wait and see if we need more information in the future as well.

This comment has been minimized.

@ExE-Boss

ExE-Boss Oct 10, 2018

Contributor

I agree with this, as adding the violated directive would make debugging easier.

This comment has been minimized.

@jwhitlock

jwhitlock Oct 10, 2018

Member

@pmac @jgmize this is a copy of the same reporter as used on Bedrock. Is the violated-directive part of the captured data in Sentry? Do you feel you have adequate data to diagnose CSP reports?

This comment has been minimized.

@pmac

pmac Oct 10, 2018

Member

I feel we do, but it could be better. We don't have this enabled in production for fear of DDOSing ourselves. Mostly we see blocked things being injected by addons.

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Oct 10, 2018

With everything back to normal, I logged in, and then noticed that I was getting a CSP violation for http://i2.wp.com violating the img-src directive (which was triggered by the call to https://secure.gravatar.com -- i2.wp.com is an image CDN). I discovered that this was due to the fact that I was running in non-SSL mode. When I switched to SSL mode by adding COMPOSE_FILE=docker-compose.yml:docker-compose.ssl.yml to my .env file, the CSP violation disappeared.

I had similar problems with the gravatar images, but I think that's because the STATIC_URL is set to a relative path /static/ in development, and that turns into http://i2.wp.com/localhost/static/img/avatar.png and an error. I think if STATIC_URL was http://localhost:8000/static/, then both gravatar and CSP would be happy. I snuck a full STATIC_URL in docker-compose.yml into PR #5014. SSL-mode includes a full STATIC_URL to a local file, and I think I had similar good results with that.

I assume the next step is to enable CSP but only in reporting mode, with these settings in an infra PR:
CSP_ENABLE_MIDDLEWARE=True
CSP_REPORT_ENABLE=True
CSP_REPORT_ONLY=True

Yes, that's my thought too, but start with reporting-only on staging, and make sure the Sentry integration works with a manageable number of reports, before we go reporting-only in production and turn on the fire hose. I've also had a decent experience running a similar configuration in local development, we may want to make that a default to catch issues before they get to production, once we have an enforced CSP.

# mitigation option for a flood of violation reports
return HttpResponse()
data = client.get_data_from_request(request)

This comment has been minimized.

@jwhitlock

jwhitlock Oct 10, 2018

Member

I added a breakpoint here. The POST data, including the full report, is captured by client.get_data_from_request(request), and sent with client.captureMessage below.

This comment has been minimized.

@escattone

escattone Oct 10, 2018

Member

Awesome, thanks for checking that!

@escattone

This comment has been minimized.

Member

escattone commented Oct 10, 2018

Good to know about the relative vs full STATIC_URL, thanks @jwhitlock!

@escattone escattone merged commit ab83730 into mozilla:master Oct 10, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (mdn) No manifest changes detected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment