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

Introduce content security policy (CSP) #29545

Merged
merged 12 commits into from
Feb 1, 2019
Merged

Introduce content security policy (CSP) #29545

merged 12 commits into from
Feb 1, 2019

Conversation

epixa
Copy link
Contributor

@epixa epixa commented Jan 29, 2019

To kick things off, a rudimentary CSP implementation only allows dynamically loading new JavaScript if it includes an associated nonce that is generated on every load of the app.

Over time, we will add more and more restrictions to our default Content Security Policy, but for now we start with a set of restrictions that allow Kibana to operate as it stands today.

How this works

Any time the Kibana UI is requested, the server will return a content-security-policy header that describes some basic restrictions for what kind of scripts can execute. This policy is configurable via kibana.yml, though we recommend that the defaults be preserved under all but the most exceptional of circumstances.

While the policy "template" remains the same when loading an app, the policy header itself will vary from request to request because it includes a randomly generated nonce, and only <script> tags that use that same nonce will be allowed to execute. This is wired up through our bootstrapping logic and webpack, so developers shouldn't need to worry about explicitly including the nonce anywhere.

JavaScript inside HTML attributes such as href will never function. If an injected <script> tag were to somehow be rendered, it wouldn't have the necessary nonce and the browser would refuse to execute it.

Limitations

CSP is part of our defense in layers strategy, but it's not perfect. All data and user input must still be sanitized and escaped before rendering.

Internet Explorer 11 will ignore our content-security-policy header entirely. There's nothing we can do about this particular behavior. Please don't use IE11.

There are tons of third party dependencies out there that require more relaxed CSP rules or no CSP at all. If we encounter them in practice, we must either contribute the proper CSP compatibility upstream to that dependency or we can't use it.

Testing

CSP logic is encapsulated in the server/csp module, which is owned by the security team. Any changes to this logic, including to the default CSP rules will need to be approved by them.

The goal of our automated testing is to verify that CSP is being applied properly end to end.

Unit tests verify the default CSP rule template, the behavior for generating a random nonce, and behavior for converting the rules (with nonce) into a string suitable for the HTTP header.e

Integration tests verify that Kibana serves a content-security-policy header that does not allow unsafe-inline.

All functional/webdriver tests are using Kibana with the default CSP rules, so they verify that Kibana works as expected. We do not test how browsers enforce the CSP policies as that's an exercise best served by the browser projects themselves.

Notes for future CSP work

  • The GIS app relies on mapbox-gl-js, which requires the worker-src and child-src allowances in this initial set of CSP rules. The GIS team is going to work on contributing the necessary fixes upstream to remove this allowance. Also, technically child-src is deprecated in favor of worker-src, but we still need to support it for safari. Whenever we choose to add restrictions for img-src, we'll need certain allowances to support mapbox as well.
  • We currently allow unsafe-eval primarily because angular.js requires it out of the box. There may be a few other places in the code that require it as well (e.g. vega), so we will need to work toward addressing all of them. Angular has an opt-in feature called ng-csp that we should be able to use to remove the eval requirement there.

Closes #2873

@epixa epixa self-assigned this Jan 29, 2019
@epixa epixa added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Jan 29, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security

@epixa
Copy link
Contributor Author

epixa commented Jan 29, 2019

@joshbressers

@epixa
Copy link
Contributor Author

epixa commented Jan 29, 2019

@alexbrasetvik

@epixa

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

To kick things off, a rudimentary CSP implementation only allows
dynamically loading new JavaScript if it includes an associated nonce
that is generated on every load of the app.

A more sophisticated content security policy is necessary, particularly
one that bans eval for scripts, but one step at a time.
@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@epixa

This comment has been minimized.

@thomasneirynck
Copy link
Contributor

verified the Maps-app is working with this policy.

@legrego
Copy link
Member

legrego commented Jan 31, 2019

Your proposal makes sense to me. It's flexible enough to support arbitrary rules, so we don't have to keep a list of allowed rules up-to-date. I think the tradeoff is ease of configuration/readability with the nested quotes, but I'm not terribly concerned about that since I imagine most installations won't need to touch these settings.

Although... in terms of leniency, this might allow someone to silently misconfigure their CSP with a typo. I know ES has been working towards reducing their leniency, do you think it's worthwhile to do the same here?

I'm curious how the strict flag works -- how will we block legacy browsers when this is set?

@epixa
Copy link
Contributor Author

epixa commented Jan 31, 2019

@legrego Your point on leniency makes a lot of sense, but I think we have to be lenient here due to emerging standards. The reality is that the CSP spec does evolve, and it's entirely reasonable that a user would expect to be able to update an older Kibana install with a modern CSP policy. I could see the argument either way, but for now I'm inclined to go with future proofing until we learn more about how CSP is used.

As for csp.strict, the checking would be fairly rudimentary but effective. The goal isn't to catch every possible misbehaving CSP behavior in a browser but rather to catch browsers that don't enforce even the bare minimum set of requirements (which would be IE11 and very old versions of our other supported browsers). So before any script files are loaded, the app entry point html would attempt to execute a "naked" inline script that sets a variable on window. It then executes a properly-nonced inline script that checks for the existence of that variable and bails out of loading Kibana if it is found.

Don't worry too much about the details of csp.strict at this point since I intend to handle that in a separate PR.

@elasticmachine

This comment has been minimized.

@epixa epixa changed the title [wip] csp: nonce and unsafe-eval for scripts Introduce content security policy Jan 31, 2019
@epixa epixa changed the title Introduce content security policy Introduce content security policy (CSP) Jan 31, 2019
@epixa epixa requested a review from a team January 31, 2019 19:31
@epixa epixa added the review label Jan 31, 2019
@epixa epixa removed their assignment Jan 31, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

packages/kbn-interpreter/package.json Show resolved Hide resolved
src/server/csp/index.ts Show resolved Hide resolved
src/server/csp/index.ts Show resolved Hide resolved
src/server/csp/index.ts Show resolved Hide resolved
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Other than our discussions above about the set of default rules, this is looking great. Tested on chrome/ff/ie

docs/setup/settings.asciidoc Outdated Show resolved Hide resolved
src/server/csp/index.test.ts Show resolved Hide resolved
Co-Authored-By: epixa <court@epixa.com>
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @epixa!

@epixa epixa merged commit 7a87f03 into elastic:master Feb 1, 2019
@epixa epixa deleted the csp2 branch February 1, 2019 22:11
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@epixa epixa added the Feature:Security/CSP Platform Security - Content Security Policy label Feb 4, 2019
epixa added a commit that referenced this pull request Feb 11, 2019
* csp: nonce and unsafe-eval for scripts

To kick things off, a rudimentary CSP implementation only allows
dynamically loading new JavaScript if it includes an associated nonce
that is generated on every load of the app.

A more sophisticated content security policy is necessary, particularly
one that bans eval for scripts, but one step at a time.

* img-src is not necessary if the goal is not to restrict

* configurable CSP owned by security team

* smoke test

* remove x-content-security-policy

* document csp.rules

* fix tsconfig for test

* switch integration test back to regular js

* stop looking for tsconfig in test

* grrr, linting errors not caught by precommit

* docs: people -> you for consistency sake

Co-Authored-By: epixa <court@epixa.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Security/CSP Platform Security - Content Security Policy release_note:enhancement review Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v6.7.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Content Security Policy
5 participants