-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add CSP for securedrop.org #431
Conversation
securedrop/settings/base.py
Outdated
CSP_DEFAULT_SRC = ("'self'",) | ||
CSP_SCRIPT_SRC = ( | ||
"'self'", | ||
'http://ajax.googleapis.com/ajax/libs/jquery/2.2.4/jquery.min.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harrislapiroff Can we bundle up these assets and serve directly? I'd prefer to omit the external call entirely, and drop the whitelist here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we can do that. This should only be used by the debug toolbar, but I think it's reasonable to want to have the CSP active even on dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll need someone to tell me if there are license concerns bundling this in our repo though when we eventually open source it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked myself that jQuery is under MIT, so I believe this inclusion should be permissible regardless of how we ultimately license this repo. If for any reason we didn't want to include the file wholesale in the repo we could also add it as an npm dependency, but that seemed like a lot of overhead for this. I've added the file on its own in #439.
I'd very much like to implement the CSP in the nginx config, which would get us a single declaration that works on all the Django/Wagtail servers. Setting at the app level will require duplication in each website repo. Open to that, since as @emkll points out, it's essentially defense in depth. |
@conorsch I have two non-blocking concerns/questions about implementing this at the nginx level:
|
Add django-csp to requirements and middleware Whitelisted inline scripts by hash and style to deny inline scripts and style. Excluded /admin path for CSP as it was making very heavy use of inline JS. unsafe-eval is required for client/common/js/common.js:645 and /client/tor/js/torEntry.js:89. We should consider fixing this, as it would allow XSS should user-supplied code finds it's way to an eval method.
Absolutely. For now, you can trust that at least CI will catch CSP violations, via the dedicated Molecule "ci" scenario. As we push toward a more container-centric workflow, we can adapt the testing procedures to make sure we provide feedback as close as possible to the time of codechange. I'm optimistic that the app-level integration presented here will provide this extremely early.
Ah, if you mean at the nginx level, yes, I suspect it would. Certainly not at the app level, though! (See changes.) The app-level changes will require more maintenance, but with substantially more control over the policy directives, which seems like a fine trade-off. @emkll will be available to consult on the CSP for all our sites, so lean heavily to get backup on decisions if you're ever unsure.
Cross that bridge when we come to it. There's a compelling argument for more dynamic viz in both STN and PFT, but we don't have any work earmarked for that yet. Even when we do get to that point, we'll be glad of app-level integration for CSP. |
No longer needed since
I'm having a wacky time trying to figure out why we're getting all those |
I still think that my theory is possibly correct, because I see |
@conorsch I'm trying to understand the state of the PR as you see it. Do you think this will be ready to merge as an interim solution? I notice that the PR contains significant hashing of inline CSS, which seems inappropriate for the nginx config, so if this isn't merged as-is, it sounds like we'd have to split cross-site CSP rules from per-site CSP rules? We're very close to launch, so I'm a little wary of keeping scope contained. |
Didn't get to full review of this today, so a few off-the-cuff notes.
Yes, certainly. The changes are thin on docs. For example, how were the hashes for the assets obtained? Surely they were copy/pasted out of the web console during local development, but I don't see that procedure documented anywhere. When, not if, these break, we'll need to address that problem so more team members are comfortable with maintenance procedures here. Some sort of dev-oriented tooling to check verify these settings are correct would be ideal. We don't have functional tests interacting with the website UI, so it'll be difficult to stay on top of any changes over time, as we munge the script and asset files. Still, I'm willing to put up with that friction as a candidate for ironing out the CSP workflow for inclusion across our other Wagtail sites. Right now the failing tests need to be resolved—haven't spent time with those yet, will investigate starting tomorrow. |
Passing now. Two notes about the state of this PR:
|
It occurs to me that I should make a note on why tests are passing now. I think we have an inconsistent test failure somewhere. At a cursory glance it looks like it might be a result of certain model factories (which generate database objects with random content) sometimes generating objects that don't satisfy unique field constraints. |
Update: I'm able to get trigger test failures when messing with some of the hardcoded hashes, but not all. In all cases, the CSP violation will be logged to the browser console, but that's not checked in testing, and would require human eyes to catch it. I'll spend a moment this afternoon trying to cobble together barebones testing prior to merge here, just to give us confidence about not deploying breaking changes. Absent that, we'll need to be diligent about verifying changes locally when any assets are modified via PR. That sounds ripe for mistakes. We'll also need to open follow-up issues based on the next steps @emkll outlines in the OP. |
That I'm willing to put up with for now—the debug toolbar itself still displays fine, just the logo is missing. Fine. There's been substantial work on DDT to support CSP, e.g. django-commons/django-debug-toolbar@8b29127, so I'm optimistic we can iron out any kinks. Oddly I'm not seeing any CSP violations logged regarding DDT, so not fixing now. From the above, sounds like a
For these and the other conditions @emkll notes in OP, I'll file follow-up issues. For now, I'm inclined to deploy what we have against staging and monitoring for changes. We'll need to verify no console errors during review whenever assets change. Ideally we can test for that, but hit my timebox on investigating that today, so will file a follow-up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's toss this at staging and bang on the hood. @harrislapiroff, will wait for your +1 before merge and deploy.
I'm OK with merging this as is, given the ticket you opened 👍 |
Adds an initial CSP to securedrop.org website using django-csp to requirements and middleware.
CSP is mostly a defense-in-depth solution, and shouldn't break functionality. We can slowly iterate on this policy:
unsafe-inline
JavaScript.unsafe-eval
is required for client/common/js/common.js:645 and /client/tor/js/torEntry.js:89. We should consider fixing this, as it would be another big win.We should also investigate using a
report-uri
(to which CSP violations are reported to) to both detect errors in the policy, and be aware of potential attack vectors (obviously while preserving user privacy and in line as the currently level of logging)