cure53 / DOMPurify

DOMPurify - a DOM-only, super-fast, uber-tolerant XSS sanitizer for HTML, MathML and SVG. DOMPurify works with a secure default, but offers a lot of configurability and hooks. Demo:
https://cure53.de/purify
Other
13.61k stars 695 forks source link

Potential for XSS exploit through data uri #911

Closed leopiel closed 6 months ago

leopiel commented 6 months ago

This issue proposes a [bug, feature] which...

Background & Context

There are certain html tags for which data uris are allowed (here). Similar issue was already reported here, but it was said it's not an XSS attack vector.

Example javascript:

<img src="data:text/html;base64,PHNjcmlwdD53aW5kb3cubG9jYXRpb24ucmVwbGFjZSgiaHR0cHM6Ly93d3cuZ29vZ2xlLmNvbSIpOzwvc2NyaXB0Pg==">

Once the html element containing the data-uri is opened in a new tab, the javascript attached in the uri will be triggered in the browser.

There is more info about this self-contained attack vector in here. In addition it can be used to direct users to phishing links.

Bug

Input

<img src="data:text/html;base64,PjxpbWcgc3JjPXh4eHh4IG9uZXJyb3I9YWxlcnQoMSk+">

Given output

<img src="data:text/html;base64,PjxpbWcgc3JjPXh4eHh4IG9uZXJyb3I9YWxlcnQoMSk+">

Expected output

<img>

Feature

I'd suggest adding an option to not allow having such data uris in any of html tags that are at the moment whitelisted in here

cure53 commented 6 months ago

Heya, thanks for reporting those. We don't think that anyone needs to be worried about Data URIs anymore, also the behavior can already be controlled:

https://github.com/cure53/DOMPurify?tab=readme-ov-file#control-behavior-relating-to-uri-values

I think we should be good here without any fixes, wdyt?

leopiel commented 6 months ago

Thank you very much for the answer.

With the mentioned attributes in the provided link it's only possible to extend the default behaviour, not limit/disable it.

Our specific issue is actually that phishing links can be hidden inside the data urls and since we are dealing with emails, it's important for us to avoid this. For this purpose, we would like to not allow data urls in the HTML sanitized with the library.

We could technically achieve the desired behaviour via the afterSanitizeAttributes hook, but it's quite cumbersome and not very clean code-wise to remove all the cases allowed within the library, thus we I believe that we and maybe also others would benefit from an option within the library for disabling the data urls for all the tags mentioned in here.

cure53 commented 6 months ago

Our specific issue is actually that phishing links can be hidden inside the data urls and since we are dealing with emails, it's important for us to avoid this. For this purpose, we would like to not allow data urls in the HTML sanitized with the library.

How would a phishing URL sent via Data URI differ for the same being done via HTTPS?

we I believe that we and maybe also others would benefit from an option within the library for disabling the data urls for all the tags mentioned in here.

That is fair, but our threat model doesn't contain Phishing protection and it can already be done using a hook. So I am hesitant to update the core library.

leopiel commented 6 months ago

How would a phishing URL sent via Data URI differ for the same being done via HTTPS?

A script such as <script>window.location.replace("https://www.google.com");</script> could be passed in base64 format:

<img src="data:text/html;base64,PHNjcmlwdD53aW5kb3cubG9jYXRpb24ucmVwbGFjZSgiaHR0cHM6Ly93d3cuZ29vZ2xlLmNvbSIpOzwvc2NyaXB0Pg==">

Once the image is opened in a new tab the script is triggered and user is being directed to the provided link.

That is fair, but our threat model doesn't contain Phishing protection and it can already be done using a hook. So I am hesitant to update the core library.

Maybe some more context: if such link is passed in the email body, the email is automatically put to spam folder and a warning is shown in Gmail so they are considering this as dangerous: image

If you still think it's not the responsibility of the library, we can go with using the hook for now and if more users have the problem it could be reconsidered.

cure53 commented 6 months ago

I still think it is not the responsibility of the library by all means and won't be until we change our threat model :slightly_smiling_face:

However, all you need is pretty much already there, here for example:

https://github.com/cure53/DOMPurify/blob/main/demos/hooks-scheme-allowlist.html

That demo hook pretty much already does what you need, as in:

  1. Hook into afterSanitizeAttributes
  2. Grab href, xlink:href, action, you might need to add src
  3. Parse them properly and check the scheme against an allow-list
  4. Throw them out if they are not allowed

My thinking is, that with very few modifications of that demo, you can already achieve your goal in a reliable fashion.

leopiel commented 6 months ago

Thanks a lot for the discussion and fast responses. Will go with this approach for now 👍

cure53 commented 6 months ago

Most welcome :slightly_smiling_face: