Skip to content
This repository has been archived by the owner on Mar 14, 2024. It is now read-only.

If DOMParser.parseFromString is an actual unsafe XSS sink... #6890

Closed
franktopel opened this issue Nov 25, 2021 · 18 comments
Closed

If DOMParser.parseFromString is an actual unsafe XSS sink... #6890

franktopel opened this issue Nov 25, 2021 · 18 comments

Comments

@franktopel
Copy link
Contributor

franktopel commented Nov 25, 2021

and [`DOMParser.parseFromString`](https://developer.mozilla.org/docs/Web/API/DOMParser#DOMParser.parseFromString)

... then potentially DOMPurify's approach of sanitizing is by itself a security risk:

    if (NAMESPACE === HTML_NAMESPACE) {
      try {
        doc = new DOMParser().parseFromString(dirtyPayload, PARSER_MEDIA_TYPE);
      } catch (_) {}
    }

Because your article also suggests DOMPurify as a sanitization lib, please clarify.

@koto Could you please take a look?

@cure53
Copy link

cure53 commented Nov 25, 2021

https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#dom-domparser-parsefromstring-dev

Note that script elements are not evaluated during parsing, and the resulting document's encoding will always be UTF-8.

@franktopel
Copy link
Contributor Author

franktopel commented Nov 25, 2021

It's not as if <script> elements are the only bad things that can be contained in dirtyPayload.

@cure53
Copy link

cure53 commented Nov 25, 2021

Then maybe this helps resolving the mystery :) There is no scripting here that could lead to XSS.

Since document does not have a browsing context, scripting is disabled.

https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#dom-domparsersupportedtype-texthtml

@franktopel
Copy link
Contributor Author

franktopel commented Nov 25, 2021

Since document does not have a browsing context, scripting is disabled.

Is the same true for your alternative document creation method?

@koto
Copy link
Contributor

koto commented Nov 25, 2021

DOMParser.parseFromString requires a TrustedHTML, because that function will create a DOM tree, and you could extract nodes from that tree - e.g. ones containing 'onclick' or 'srcdoc` attribute nodes. These would then execute as soon as they are attached to the main document.

At the same time, it's on eof the few ways to actually obtain an inert tree, for the purposes of its sanitization, which is what DOMPurify is doing. You are technically correct, DOMPurify's use of DOMParser is a security risk, because that library may have a buggy sanitization. Trusted Types in this case tell you that this library does a security sensitive operation. Which is expected, sanitizers are supposed to do that, and TT just let you discover which libraries are in fact responsible for HTML sanitization in your application.

In the long run, DOMPurify could use the native sanitizer API in place of DOMParser, and TrustedHTML would not be neccessary, and the security risk (w.r.t. DOM XSS) would be completely gone, as the native browser API instead of brilliant and life-saving, yet complex and potentially buggy logic of DOMPurify would be guaranteeing that scripty payloads are stripped from the tree.

@cure53
Copy link

cure53 commented Nov 25, 2021

Is the same true for your alternative document creation method?

Yes

@franktopel
Copy link
Contributor Author

franktopel commented Nov 25, 2021

DOMParser.parseFromString requires a TrustedHTML

Then that means that DOMPurify itself must declare a policy, and that policy is probably passing the dirty string unaltered.

@cure53
Copy link

cure53 commented Nov 25, 2021

@franktopel
Copy link
Contributor Author

franktopel commented Nov 25, 2021

Has anyone ever investigated attack vectors on elements not in a live document? Or would that be a violation of the specs anyway, if something like that existed?

@cure53
Copy link

cure53 commented Nov 25, 2021

Yes, for many years in various browsers. The last bypass we spotted was Java Applet related - other than that, there are no known issues at this moment I am aware of.

Further, if any browser ships a regression, I presume that lots of tests will go red at the same time, including ours :D

@koto
Copy link
Contributor

koto commented Nov 25, 2021

To add to @cure53, this (TT policy that blesses anything) is practically unavoidable. That's currently the only way to sanitize a string into DOM nodes. The only alternative that would be "safe by construction", and not because of DOMPurify's scrutiny and know-how, would be if DOMPurify shipped its own HTML parser and then created only safe nodes (safe, because TT won't let you create "bad" nodes) out of its emulated DOM tree. Which we know is not the correct way to tackle sanitization, nor efficient nowadays.

IIRC there was also some Safari bug a few years back, where inert doc wasn't inert at all.

@cure53
Copy link

cure53 commented Nov 25, 2021

Oh, haha yeah, the Webkit regression, that was nasty :D

We needed to fix around that in 2017:
https://twitter.com/cure53berlin/status/859655691153362944

@franktopel
Copy link
Contributor Author

Okay, thanks for taking the time and elaborating. Still feels kinda bad to have to rely on browser implementations here, but that's the way it is.

@koto
Copy link
Contributor

koto commented Nov 25, 2021

Yup. Sanitizer API, that's our only way out.

@franktopel
Copy link
Contributor Author

franktopel commented Nov 25, 2021

Would be great if other browser vendors would start implementing trusted types, at all. Safari on MacOS does not even seem to recognize the CSP setting.

image

I hope that won't invalidate the rest of the CSP.

@koto
Copy link
Contributor

koto commented Nov 25, 2021

I agree :) Your voice might help - mozilla/standards-positions#20

@franktopel
Copy link
Contributor Author

franktopel commented Nov 25, 2021

I've at least asked: mozilla/standards-positions#291 (comment)

Following that discussion it seems like Webkit considers "safer" the enemy of "safe".

@franktopel
Copy link
Contributor Author

Closing this as I trust your explanations.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants