search

pi-hole / web

Pi-hole Dashboard for stats and more
https://pi-hole.net
Other
2.04k stars 558 forks source link

Prevent DOM XSS using Trusted Types #3114

Closed orazioedoardo closed 1 month ago

orazioedoardo commented 2 months ago

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?:

Trusted Types provides a way to make DOM APIs more secure by requiring dangerous APIs like innerHTML either to be removed and refactored away (ultimate goal) or forcing them to pass through sanitization before being used.

How does this PR accomplish the above?:

First, it removes uses of innerHTML where feasible, that is, in Pi-hole written scripts. Currently only in charts.js (though there doesn’t seem to be room for attack in here). Second, to address vendor scripts, a Trusted Type policy is created to automatically pass uses of innerHTML through DOMPurify's sanitization. The policy being named "default" makes it apply without having to change the code, as opposed to a specific policy which would need to be used like this: element.innerHTML = myPolicy.createHTML(someHTML).

Vendor scripts:

Link documentation PRs if any are needed to support this PR:

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.
DL6ER commented 2 months ago

What is the actual attack vector you're trying to protecting against here? This PR, honestly, adds quite a bit of complexity: not only that a lot of new code is added, also the newly added external scripts need to be maintained, incl. updating the new vendor scripts and possibly changing even more code in the future if there are major releases (possibly breaking the API) of the vendor scripts.

We should be sure that it is really worth doing this and shouldn't do it just because we can to protect against something which may be very hypothetical or something that may only become an issue in some distant future due to a change we don't foresee right now.

orazioedoardo commented 2 months ago

What is the actual attack vector you're trying to protecting against here?

DOM-based XSS, I recommend checking this out, this and this on how it works.

we can to protect against something which may be very hypothetical or something that may only become an issue in some distant future due to a change we don't foresee right now

It's mostly it actually, Trusted Types is intended to be used as a tool to surface to the developer uses of unsafe web APIs for them to remove them in place of safe APIs. The goal is steering towards not introducing potential vulnerabilities in the first place, even if currently there aren't, which I wrote above. It's what https://github.com/pi-hole/web/pull/3114/commits/b295ab41ed22931d4a36b483528b75600d804122 does, which could be a PR on its own without the concept of Trusted Types.

The Trusted Types thing is really just this file. It instructs the browser to force uses of unsafe APIs that render HTML go through the createHTML method first. In a perfect world where a web application uses no unsafe APIs, you could just send Content-Security-Policy: [...]; require-trusted-types-for 'script'; trusted-types 'none' to eliminate DOM XSS, and close this PR. With Trusted Types, the risk of DOM XSS is reduced to just the security of the policies, in this case the DOMPurify filtering not being bypassable.

This PR, honestly, adds quite a bit of complexity

That said, I don't think it does. Pi-hole itself already didn't use unsafe APIs outside charts.js from what I can see. Other than that the rest is just automatically dealing with vendor scripts not doing the same good job (enable this line and see how many warnings you get), which necessitated adding a default policy since you can't refactor those scripts. Actually I wanted to make it show the warning only on pi-hole scripts which can be refactored but I haven't figured out a way to do it.

the newly added external scripts

It's just the policy file though. You may need to add another policies for the other two instance methods but that's it. Remember the goal is not having to use it (see second point), policies are a compatibility thing where you can't change existing scripts.

updating the new vendor scripts

Yeah you need to keep at least DOMPurify up to date but

possibly changing even more code in the future if there are major releases (possibly breaking the API) of the vendor scripts

So I can't predict the future but I don't think DOMPurify would change the API out of the blue, it's been stable forever basically. Regarding the polyfill, that's based on a web standard so it shouldn't break like that, but it's also optional and can be removed by making a small change to do browser feature detection in the policy. You still would need to update the policy, not the other scripts.

If you appreciate the concept but don't feel like to enforce it, you could deliver Content-Security-Policy-Report-Only: require-trusted-types-for 'script'; trusted-types 'none' instead to only log violations to console and use it as a debugging feature to see which scripts need refactoring.