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

Set CSP header to prevent XSS #17798

Merged
merged 9 commits into from Aug 12, 2021
Merged

Set CSP header to prevent XSS #17798

merged 9 commits into from Aug 12, 2021

Conversation

justinvelluppillai
Copy link
Contributor

@justinvelluppillai justinvelluppillai commented Jul 20, 2021

Description:

Fixes #17773 by setting a Content-Security-Policy which is fairly loose at this stage.

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@Findus23
Copy link
Member

I think we should only be using Content-Security-Policy-Report-Only for at least one or two releases as I'm sure there are a lot of people with weird setups where the CSP will break in fascinating ways.
I still think it is worth creating a secure one for Matomo as it will make Matomo a lot more secure, but also provide an option to fall back to Report-Only or disable it, for setups where using one is not possible.

@tsteur
Copy link
Member

tsteur commented Jul 21, 2021

👍 be good indeed to have such an option to disable it, or using report-only. And could indeed start with report-only. Maybe in dev mode we would always from beginning use the real one so it's easier to make things compatible? might not make sense though just some thoughts. I suppose this would give some people more time to adjust their setup if needed. They might not notice these reports but at least they might notice it in the changelog maybe?

@justinvelluppillai
Copy link
Contributor Author

We could have a setting to choose between enabled, disabled, or report only mode, defaulting to disabled or report only.

@justinvelluppillai
Copy link
Contributor Author

I've added a couple of settings, one to enable/disable and one to specify a report-uri. CSP is disabled by default for now, specifying a report-uri will switch it to Content-Security-Policy-Report-Only

@justinvelluppillai justinvelluppillai marked this pull request as ready for review July 26, 2021 00:36
@justinvelluppillai justinvelluppillai added the Needs Review PRs that need a code review label Jul 26, 2021
@tsteur
Copy link
Member

tsteur commented Jul 26, 2021

👍 @justinvelluppillai it seems report-uri is deprecated see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/report-uri ? Could maybe remove this feature and then have only one setting for the three different modes (disabled, report, enabled)?

Or seems it might be replaced by https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/report-to ? I don't know how commonly used it is. If it's not commonly used that often someone could maybe add this feature as a plugin.

@Findus23
Copy link
Member

While report-uri seems to be deprecated, it is the only parameter that is actually supported by browsers:
https://caniuse.com/mdn-http_headers_csp_content-security-policy_report-uri

report-to is not supported by any non-chrome browser: https://caniuse.com/mdn-http_headers_report-to

That said, Content-Security-Policy-Report-Only makes also sense without a report-uri (especially during development) as it will log violations in the console.

@justinvelluppillai
Copy link
Contributor Author

Huh, I didn't notice that in my investigations. A couple of possible approaches:

  1. Specify both report-uri and report-to as described here https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/report-uri
  2. Join @Findus23 and @tsteur's solutions: have three modes only and exclude the report-uri/report-to fields and let it log to console.

Both trivial to implement, option 1 provides more possible configurations but I lean towards option 2 for simplicity, if everyone agrees?

@Findus23
Copy link
Member

I think we could either add two settings (one switching between on, report-only and off and one specifying the report-uri (or being empty)) or use three settings (like e.g. in https://github.com/Chocobozzz/PeerTube/blob/develop/config/default.yaml#L151-L154)

@tsteur
Copy link
Member

tsteur commented Jul 27, 2021

I suppose using two settings could work and let us extend it better in the future if we were to decide to add more features (which isn't currently needed).

So something like you mention

csp_enabled = 0/1
csp_report_only = 0/1 . # Only does something if csp is enabled

@tsteur
Copy link
Member

tsteur commented Jul 27, 2021

@justinvelluppillai it seems like the Page Overlay feature doesn't work yet when the URL is from a different page? I just tested it and it won't load the site.

@justinvelluppillai
Copy link
Contributor Author

@tsteur with the default settings we've chosen (report-only) it should still work as normal but report some issues. I can modify the Page Overlay feature to use a different (or no) CSP. I will take a look at getting the Page Overlay feature running locally then see what's best.

@tsteur
Copy link
Member

tsteur commented Jul 29, 2021

fyi I tested with in report mode (not report-only).

@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2021

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Aug 6, 2021
@sgiehl sgiehl removed the Stale The label used by the Close Stale Issues action label Aug 6, 2021
@tsteur tsteur added this to the 4.5.0 milestone Aug 9, 2021
@justinvelluppillai
Copy link
Contributor Author

@sgiehl it'd be great if you could give this a check over and if all ok we can merge as it won't have too much impact until we enable it.

@sgiehl
Copy link
Member

sgiehl commented Aug 9, 2021

@justinvelluppillai Sure. Feel free to simply use the github feature to request for reviews in the future if you need one ;-)

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Left a couple of minor comments. Btw. might also be good to merge in the latest changes from 4.x-dev to see if tests are passing. Currently the UI tests are showing various failures, that actually look unrelated.

tests/PHPUnit/Unit/SecurityPolicyTest.php Outdated Show resolved Hide resolved
tests/PHPUnit/Unit/SecurityPolicyTest.php Outdated Show resolved Hide resolved
core/View/SecurityPolicy.php Outdated Show resolved Hide resolved
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

The code looks good now 👍

@sgiehl sgiehl merged commit ca8e320 into 4.x-dev Aug 12, 2021
@sgiehl sgiehl deleted the 17773-content-security-policy branch August 12, 2021 12:35
@justinvelluppillai justinvelluppillai added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Matomo should be setting content security policy to prevent some XSS
4 participants