FTL icon indicating copy to clipboard operation
FTL copied to clipboard

Improve Content Security Policy and Referrer Policy (v6)

Open orazioedoardo opened this issue 1 year ago • 2 comments

Thank you for your contribution to the Pi-hole Community!

Please read the comments below to help us consider your Pull Request.

We are all volunteers and completing the process outlined will help us review your commits quicker.

Please make sure you

  1. Base your code and PRs against the repositories developmental branch.
  2. Sign Off all commits as we enforce the DCO for all contributions
  3. Sign all your commits as they must have verified signatures
  4. File a pull request for any change that requires changes to our documentation at our documentation repo

What does this PR aim to accomplish?:

Requires https://github.com/pi-hole/web/pull/3104, https://github.com/pi-hole/web/pull/3113 and https://github.com/pi-hole/web/pull/3114. Current CSP allows everything from / to the same origin, though not all that CSP covers is required for Pi-hole. Moreover, default-src does not cover base-uri, form-action, frame-ancestors. Not setting these means allowing them.

How does this PR accomplish the above?:

This PR strengthen the policy by disallowing everything by default (possibly also future directives falling under default-src), then allowing just what's needed. It also removes unsafe-inline from script-src as from my testing it looks like it's not needed and having it significantly weakens XSS mitigation.

On the topic of XSS mitigations, script-src 'self' is not even the best option as it allows every script from the same origin, not just what's requested by the current page. Also it can be potentially bypassed via user uploaded files (not applicable) or JSONP endpoints, for which I'm not sure whether you use them or not.


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

  • [X] I have read the above and my PR is ready for review. Check this box to confirm

orazioedoardo avatar Aug 11 '24 07:08 orazioedoardo

Do we really need to use such a narrow CSP policy? Not only that it creates some ugliness (https://github.com/pi-hole/web/pull/3113), it also creates a lot of complexity (https://github.com/pi-hole/web/pull/3114) in other places.

To me, a much more basic policy like:

Content-Security-Policy: default-src 'self'; frame-ancestors 'self'; form-action 'self';

already seems to offer a lot for no added complexity:

  • All resources are hosted by the same domain of the document.
  • There are no inlines or evals for scripts and style resources.
  • There is no need for other websites to frame the website.
  • There are no form-submissions to external websites.

DL6ER avatar Aug 20 '24 17:08 DL6ER

Do we really need to use such a narrow CSP policy?

I believe it makes sense to default deny everything (just a security best practice after all) considering the effort to adopt the policy is pretty small:

Not only that it creates some ugliness (https://github.com/pi-hole/web/pull/3113)

default-src 'self' only benefits Firefox because of https://github.com/pi-hole/web/pull/3113, but that's a bug because default-src shouldn't control anything on its own and they are planning to address it. So it should be a temporary measure until it hopefully falls under img-src like other browsers do.

it also creates a lot of complexity (https://github.com/pi-hole/web/pull/3114) in other places

To clarify, https://github.com/pi-hole/web/pull/3114 is for the require-trusted-types-for 'script' directive, which enforces Trusted Types on Chromium browsers, but not required for the rest of CSP. I added it here because it's delivered via CSP and again, it was quite easy due to the way Pi-hole is built (I explain more on Trusted Types in the relevant PR).

To me, a much more basic policy like:

This policy misses base-uri 'none' or base-uri 'self' by the way. It's not included in default-src and missing it leaves a potential bypass venue to script-src 'self' by injecting a base tag. EDIT: I'm not sure base-uri also changes the meaning of self but pi-hole doesn't use <base> element, so better safe than sorry.

You are also not using iframes even to pi-hole itself in v6 so no need for self on frame-ancestors and as of now unsafe-inline on style-src is required.

orazioedoardo avatar Aug 20 '24 19:08 orazioedoardo

So should I rebase this and related pull requests against the development branch or you don't plan on merging the changes?

orazioedoardo avatar Sep 15 '24 09:09 orazioedoardo