w3c / trusted-types

A browser API to prevent DOM-Based Cross Site Scripting in modern web applications.
https://w3c.github.io/trusted-types/dist/spec/
Other
600 stars 70 forks source link

Should all 3 script IDL setters change the associated script text value identically #517

Open lukewarlow opened 4 months ago

lukewarlow commented 4 months ago

A good point came up during code review of an associated webkit patch that the .innerText setter steps and the .textContent/.text setter steps are different, and presumably could result in a different child contents value? If this is the case the current spec (and Chromium implementation) could lead to some script execution errors despite being set via a trusted object and sanctioned IDL attribute?

Question 1: Are these actually identical in behaviour?

Question 2: If they're not should the spec and implementationed be updated to account for the differences?

annevk commented 4 months ago

Oh yeah, innerText is wild: https://software.hixie.ch/utilities/js/live-dom-viewer/?%3Cscript%3E%0Adocument.querySelector(%60script%60).innerText%20%3D%20%60test%5Cntest%60%0A%3C%2Fscript%3E

Can we just not support that?

lukewarlow commented 4 months ago

cc @otherdaniel and @koto curious what your thoughts on this are. I suspect that people might be using this in the wild? Question is can we remove it as a valid IDL sink or do we just document in the spec that it's an oddity and might not always work as expected?

koto commented 4 months ago

[setting script content via innerText] could lead to some script execution errors despite being set via a trusted object

Isn't this the case even without Trusted Types? The spec doesn't provide any indication that the value will be functional for any given sink, only that the value has passed through a policy so there's no DOM XSS risk with assigning it.

Can we just not support that?

We definitely need to cover the DOM XSS sinks with Trusted Types, and script contents being set via innerText is used commonly in applications. TT consciously tries to work with the existing sinks, as opposed to e.g. proposing new ones and disabling old ones.

lukewarlow commented 4 months ago

[setting script content via innerText] could lead to some script execution errors despite being set via a trusted object

Isn't this the case even without Trusted Types? The spec doesn't provide any indication that the value will be functional for any given sink, only that the value has passed through a policy so there's no DOM XSS risk with assigning it.

Specifically I mean trusted types related script execution errors. Because the value may mismatch in which case TT will reject it, which could be unexpected.

Sounds like we should either update the implementations and spec to set the value after it's been set so that it matches. Or just to add a note that explains there may be a mismatch?

annevk commented 4 months ago

Commonly? Can you point to an example? Given what it does with newlines I'd expect all kinds of brokenness.

koto commented 4 months ago

Specifically I mean trusted types related script execution errors. Because the value may mismatch in which case TT will reject it, which could be unexpected.

I might be missing some context. Do you mean that a string would be passed to innerText on a script? Applications do use innerText with script elements intentionally to execute code, which is the only case in which TT would trigger? Can you give an example of what you mean if I misunderstood?

Commonly? Can you point to an example? Given what it does with newlines I'd expect all kinds of brokenness.

That's not easily shareable, but I see it (HTMLElement.innerText TT violations) right now in current violation reports, ~200K distinct violations in last 14 days. The scripts are all one liners. It seems to be just yet another variant of injecting code into applications that some developers use for no particular reason.

annevk commented 4 months ago

Ah, if the scripts are all on a single line they won't run into problems, but I don't think enshrining that makes sense for a standard.

koto commented 4 months ago

Again, I don't think the TT claim that the values reaching the sinks are semantically correct, only that they were checked before reaching the sinks.

annevk commented 4 months ago

Sure, but we also store the given value in some internal slot, no? It seems that would mismatch with the value we end up using, which seems bad.

lukewarlow commented 4 months ago

Again, I don't think the TT claim that the values reaching the sinks are semantically correct, only that they were checked before reaching the sinks.

Yes, but TT also claims that if they've been checked they'll attempt to execute. Which afaict isn't guarunteed for multiline text with innerText. Because we set the internal slot before the processing, and as such the prepareScript function will throw (or at least call the default policy).

data:text/html,<meta http-equiv="Content-Security-Policy" content="require-trusted-types-for 'script'; trusted-types default;"><body><script>const script = document.createElement('script'); script.innerText = TrustedScript.fromLiteral`console.log('line 1');\nconsole.log('line 2');`; document.body.appendChild(script);</script>

If you paste the above data URL into Chrome (with experimental flag for the fromLiteral to work) then it will error rather than execute. That's not an error from .innerText's spec, that's an error from the Trusted Types spec. The fromLiteral isn't relevant it's just the easiest way to write a quick example like this.

lukewarlow commented 2 months ago

This will be fixed by https://github.com/w3c/trusted-types/pull/533 as it avoids the weirdness with innerTexts newline handling (now uses a flag not a duplication of value)