Skip to content
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

5082 - Make content-security-policy more strict #5220

Merged
merged 2 commits into from
Jan 10, 2019
Merged

Conversation

kennsippell
Copy link
Member

Description

I believe this is our most tightly constrained CSP without breaking anything. It requires a few "unsafe" configurations:

  1. imgSrc: data: - "developers SHOULD NOT include either 'unsafe-inline', or data: as valid sources in their policies. Both enable XSS attacks by allowing code to be included directly in the document itself; they are best avoided completely." https://www.w3.org/TR/CSP3/#csp-directives
  2. scriptSrc: unsafe-eval - We use eval() about 88 times and use new Function() about 17 times in inbox.js. "AngularJS makes use of this in the $parse service to provide a 30% increase in the speed of evaluating AngularJS expressions." https://docs.angularjs.org/api/ng/directive/ngCsp
  3. styleSrc: unsafe-inline - Our dependency, angular-ui-bootstrap injects about 8 inline style blocks. Here is an example issue where they fix one of them Angular-ui is throwing csp error on uib-popover angular-ui/bootstrap#5470. The risks from unsafe css are reduced by fully constraining imgSrc (not done here) and by avoiding unsafe-eval (not done here).

#5082

Questions for Reviewer

  1. I'm explicitly allowing the inline script for telemetry window.startupTimes - through a whitelisted hash. Another option would be to move that script into its own file and appcache it. As-is, if the content of this script ever changes, the corresponding hash will need to be updated. If we keep it as a hash, I'm unsure of the best method to keep the hash and script in sync (new test, calculate the hash in api, just pray, etc.).
  2. We may want to whitelist a hash for each of the eight style blocks from angular-ui-bootstrap instead of usingunsafe-inline - I didn't do it because I'm not sure they are always the same for all browsers but they probably are and can follow-up if we close on that direction.

Additional considerations

This change also makes the header larger. Including it on all resources (js, css, application/json, etc.) isn't required so one minor performance tweak we might consider here is setting it for Accept: html requests only.

CSP also supports report-uri which can alert us when the security policy is violated (due to bugs or successful attacks). Might be worth opening an issue to enable this down the road.

Testing

I went through some basic workflows in webapp + tried each actions in admin pages. In general though, this has risks of breaking edge-case functionalities or workflows I don't know about. Best to AT across all browsers as there are apparently some differences.

Review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Announced in Changes.md in plain English. Configuration and user documentation on medic-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in Changes.md.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

Copy link
Contributor

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

Great work!

@garethbowen garethbowen merged commit 1e987df into master Jan 10, 2019
@garethbowen garethbowen deleted the 5082-csp branch January 10, 2019 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants