Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Trusted Types appendix. #55

Closed
wants to merge 4 commits into from

Conversation

otherdaniel
Copy link
Collaborator

Comment on interactions between Trusted Types and the Sanitizer API, in an appendix.

```js
const my_config = { ... };
const sanitizing_policy = trustedTypes.createPolicy("apolicyname", {
createHTML: new Sanitizer(my_config).bound() });
Copy link

@koto koto Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to refer to the arguments explicitly?
E.g. createHTML(input) {return new Sanitizer(my_config}.bound(input) }. Policies createXYZ functions have variadic arguments after the first one, and with mentioning the argument explicitly we don't have to tie both TT and Sanitizer bound() signature (and are free to, for example, add arguments to bound() later).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was on purpose. Not sure if I'm being too clever here. :)

Minor misunderstanding: bound doesn't take parameters, but the function it retuns does. We do have the exact same problem there, though. E.g., if we were to redefine sanitizeToString to take two parameters that get concatenated, then my current definition would do weird things.

We could define bound() like so: return x => this.sanitizeToString(x);

IMHO, main 'contra' would be 1, that sanitizeToString doesn't really lend itself to extensions, so planning for such a case might not make that much sense, and 2, if it does change, we could change bound() to match. Main 'pro' is IMHO that... it's super easy to do, and additional clarity doesn't hurt.

I think I'll do an update for this case, by being more explicit about what bound() does.

auto-sanitize any HTML assignments.

```js
trustedTypes.createPolicy("default", { createHTML: new Sanitizer().bound() });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue, but the default policy is actually already called with three arguments, so the potential extension of bound() would be complex if there's existing code like that.

auto-sanitize any HTML assignments.

```js
trustedTypes.createPolicy("default", { createHTML: new Sanitizer().bound() });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this doesn't make sense to me. Why can't we let Sanitizer return TrustedHTML? I don't see a reason why developers have to create a policy when Sanitizer is guaranteed to not have dangerous output?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm indifferent here. Since the sanitizer can directly produce nodes that don't XSS, many applications won't need a TrustedHTML type (nodes also let you avoid the parser-roundtrip). For those that do need strings, creating a policy is relatively simple (and we can improve that even, e.g. trustedTypes.createPolicy('name', sanitizerInstance) or sanitizerInstance.createPolicy('name')).

OTOH, we do have trustedTypes.emptyHTML because it's useful. And it's not terrible if Sanitizer produced TrustedHTML. Having trusted-types 'none'; require-trusted-types-for 'script' guaranteeing no DOM XSS is very compelling.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we require policy creation for Sanitizer API, then it becomes really hard for people to roll out the Perfect Types. Because any dependency could be using innerHTML and that'll break Perfect Types (because policy creation is required). On the other hand, if we allow Sanitizer to product TrustedHTML without policy creation (I'm okay with 'sanitizer' keyword), they will help reduce the breakage.

index.bs Outdated
However, it may not always be in the developers interest to allow direct
creation of HTML from any Sanitizer instance, especially if the Trusted Type
policies intended for use with the document are meant to block content beyond
direct script applications. To accomodate this case, Trusted Types' CSP
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this would have the effect we're seeking for. Applications that want to put stringer-than-XSS restrictions on their DOM, already have an issue with regular DOM APIs. Guarding the sanitizer input with TT doesn't buy them much, as the problem they face is different - how to make sure that all access to the DOM is limited. This pattern would suggest they go through TT(custompolicy)->Sanitizer(customconfig)->DOM, but the the easier access pattern, and probably a source of vulns is to do strings->DOM.

The challenge in enforcing such restrictions is to make sure you never skip calling a sanitizer, and the input type parameter doesn't give you that.

There's also a secondary concern that TrustedHTML already communicates that your data is ready for DOM, even more than strings, and that might give a suggestion not to sanitize.

I think the sanitizer always accept a string on input, the data is always suspicious, that's what the sanitizer is for.

@otherdaniel
Copy link
Collaborator Author

Okay, so no consensus here just yet.

I suspect we're discussing different use cases in disguise. We should support all of them.

I think the first is, XSS-safe applications with minimal ceremony. I think both Koto and Jun are arguing for this. In this view, a Sanitizer that is default-.safe for XSS should siimply be available everywhere, and should be able to produce TrustedScript. Then the app (& dependencies) could just use them, without much planning and infrastructure, and one could enable Trusted Type to complete enforcement.

I think the second is to use Trusted Types for app-specific restrictions. Say, you have custom elements that have "interesting" behaviours attached to them. Or you want to ensure that only specific styling is used (e.g. <em> and <strong> allowed, but no style= attributes). Or you want to prevent id= attributes, because elsewhere in your app you rely on ids only being available in certain cases. Trusted Types wasn't primarily built for this, but you could (partially) use it. "Partially" because dependencies can create whatever HTML they want by using the Node APIs, but you can prevent them from passing through user input with them - e.g. in a widget library, which wouldn't filter them because to the library itself those are harmless.

I'm assuming both use cases are legitimate... not sure if everyone agrees.

I'll add sanitizeToTrustedHTML back, because I think that supports the first use case better, and it doesn't hinder anything in the second.

@koto
Copy link

koto commented Jan 29, 2021

I think the second is to use Trusted Types for app-specific restrictions. Say, you have custom elements that have "interesting" behaviours attached to them. Or you want to ensure that only specific styling is used (e.g. <em> and <strong> allowed, but no style= attributes). Or you want to prevent id= attributes, because elsewhere in your app you rely on ids only being available in certain cases. Trusted Types wasn't primarily built for this, but you could (partially) use it. "Partially" because dependencies can create whatever HTML they want by using the Node APIs, but you can prevent them from passing through user input with them - e.g. in a widget library, which wouldn't filter them because to the library itself those are harmless.

Careful coding already allows you to both protect from XSS and app-restrictions. TT try to make it robust and mistake-resistant for XSS, but would fail for app-restrictions I think. Sanitizer solves a different issue - how to safely process user input (for XSS, but also for app-specific restrictions). You still have to remember to call it, and with the right config, so it doesn't guarantee that all user input will be sanitized. Requiring TrustedHTML on sanitizer input will not solve the app-specific problem either, as there's nothing to guarantee that the TT policy rules would not actually get violated later by the sanitizer config. For example, if your policy for some reason really wants uppercase attribute names, and the sanitizer would normalize the names, technically this is a violation (the output of the sanitizer would not pass the policy). I think if the app-specific rules are to be enforced on user input, than it has to be in a sanitizer config rather than in TT - or, alternatively, make sure not to produce nodes and use a input->sanitizer()->trustedTypesPolicy(customrules)->innerHTML pattern. But again, I don't think TT are a good tool for app-specific DOM limitation, as the raw sinks still are enabled.

I'm assuming both use cases are legitimate... not sure if everyone agrees.

I'll add sanitizeToTrustedHTML back, because I think that supports the first use case better, and it doesn't hinder anything in the second.

@mikewest
Copy link
Member

I've only skimmed through this thread, so I'm sure I'm missing nuance. Please do point it out if so!

At a high level, I see us creating two tools that layer together to form a pretty substantial mitigation for cross-site scripting. The sanitizer API can process a string, and remove content known to the browser to cause code execution (whether user-provided or not). Trusted Types can ensure that strings go through some arbitrary processing function before being "executed" via one or more DOM sinks. Assuming a sanitizer configuration that's appropriate for a given app, it seems clear that they could be composed in either direction with similar practical effect. That is, both of the following code snippets should produce the same result:

// Example 1
const s = new Sanitizer({ config: goes, here: true });
el.replaceChildren(s.sanitize(input));
// Example 2
const s = new Sanitizer({ config: goes, here: true });
const policy = trustedTypes.createPolicy("whatever", { createHTML: i => { return s.sanitizeToString(i); } });
el.innerHTML = policy.createHTML(input);

The former seems initially preferable, given that there's a real risk that the snippets don't produce the same state, as the latter require re-parsing a sanitized string post-sanitization, leading to some risk of mutation-based XSS. So, one potential option is to not integrate TT and the Sanitizer at all, but expect all sanitizer usage on TT-enforced pages to generate DocumentFragments which can be injected into pages wholesale (or to be executed in the context of a policy, which, presumably, is audited).

I'm not sure that this option is sufficient, given Trusted Types' threat model, which (I believe) aims to protect confused-but-not-malicious developers from shooting themselves in the foot while parsing untrusted input. Clever Security Folks™ create policies which are known-good for a given application, and require them to be used by preventing others from being created (via the policy naming constraints). It seems to me that allowing a trivial route around those carefully-constructed policies could be dangerous in the face of custom elements and/or script gadgets that can convert attributes which appear inert to the browser into executable code. Our confused-but-not-malicious developer could do their best to do The Right Thing™, constructing a Sanitizer that doesn't quite cover all the bases, and relying upon it in security-relevant contexts with disastrous results.

A Sanitizer.sanitizeToTrustedHtml method doesn't seem to address this concern; it instead provides a mechanism that combines the risks of re-parsing with the risks of unaudited flow from an untrusted source to the DOM. That doesn't feel like the right integration point.

Two and a half alternative approaches might be interesting to explore:

  1. If Trusted Types are enforced for a given context, break Sanitizer.sanitize. This would leave Sanitizer.sanitizerToString functional, but it would only have a useful effect when executed as part of a TT policy (which, presumably, is audited, thanks to the naming conventions).

  2. Do 1, but create some explicit mechanism of passing a sanitizer configuration into a TT policy. This could also involve teaching Trusted Types to generate fragments, which could provide the best of both worlds. For example:

    const sanitizerConfig = { sanitizer: config, goes: here };
    const policy = trustedTypes.createPolicy("whatever", { sanitizer: sanitizerConfig });`);
    el.innerHTML = policy.createHTML(input);
    // or:
    el.replaceChildren = policy.createFragment(input);
  3. Add some header-based control of sanitizer configurations, so that Clever Security Folks™ can audit them as well.

1 is trivial. 2 appeals to me. 3 is somewhat complicated, and reinvents at least part of the wheel. WDYT?

P.S. I'd note also that the analysis above becomes weirder if Trusted Types ever grows support for style injection points and/or the sanitizer does the same.
P.P.S. Integrating the Sanitizer into Trusted Types (a la 2 above) rather than Trusted Types into the Sanitizer is also a nice way of side-stepping the very real problem that Chromium is still the only implementation of Trusted Types, and that doesn't seem to be in eminent danger of changing (even though it would be marvelous if it did).

@shhnjk
Copy link

shhnjk commented Feb 1, 2021

The former seems initially preferable, given that there's a real risk that the snippets don't produce the same state, as the latter require re-parsing a sanitized string post-sanitization, leading to some risk of mutation-based XSS.

I think Sanitizer API needs to provide a way to defeat mXSS. We can't make mXSSes a developer's problem, because we (browsers) are creating the mutations. So we are in a better position to provide a safe way to avoid mXSS than developers. We are discussing potential solutions at #37.

It seems to me that allowing a trivial route around those carefully-constructed policies could be dangerous in the face of custom elements and/or script gadgets that can convert attributes which appear inert to the browser into executable code.

If the custom elements and/or script gadgets is under a Trusted Types enforcement, Trusted Types should block those. If not, confused-but-not-malicious developers will create a bug which can bypass Trusted Types anyways by using DOM APIs, which will make Trusted Types not much valuable. AFAIK, the only sinks I know of are the Blob API and Dynamic import, in which the APIs itself needs to be fixed.

A Sanitizer.sanitizeToTrustedHtml method doesn't seem to address this concern; it instead provides a mechanism that combines the risks of re-parsing with the risks of unaudited flow from an untrusted source to the DOM. That doesn't feel like the right integration point.

Given my above comments, I think something like Sanitizer.sanitizeToTrustedHtml will be useful. Because what Trusted Types needs to enforce is same or more lax than the Sanitizer API (because Sanitizer API will strip out JS URL where TT doesn't). The risks of re-parsing is something that we need to solve in the Sanitizer API as I mentioned.

the risks of unaudited flow from an untrusted source to the DOM.

If we are talking about the DOM-XSS here, I think that the output coming out of the Sanitizer API with default config needs to be DOM-XSS free (assuming that TT is enforced). In that sense, the output ought to be something that "does not need an audit" in terms of DOM-XSS. If we can come to this point, all web community can try breaking Sanitizer API instead of auditing each places where they use Sanitizer API, which seems like the better place to be.

And with something like Sanitizer.sanitizeToTrustedHtml, security engineers doesn't need to audit places where developers assigns static HTML, by just using Sanitizer.sanitizeToTrustedHtml as well. This is something that will also help adoption of Trusted Types much faster, as Trusted Typing existing code will be as easy as checking for Sanitizer API availability, instead of creating policy and returning static HTML for all places where developers needs to use innerHTML.

Examples:

  1. https://source.chromium.org/chromium/chromium/src/+/master:ui/webui/resources/js/cr/ui/tree.js;l=320-321;drc=56de816e7df5874cc1775439ff5f0a6aabcdfd11
  2. https://source.chromium.org/chromium/chromium/src/+/master:ui/webui/resources/js/cr/ui/bubble.js;l=84-88;drc=6e82ffa4ab779589119ba9cf5d8a5fe6720b2deb
  3. https://source.chromium.org/chromium/chromium/src/+/master:out/Debug/gen/ui/webui/resources/preprocessed/cr_components/omnibox/cr_autocomplete_match_list.js;l=19-24;drc=279ef98c9e6917879e747d483abc2110a242ffd2

Now imagine that more frameworks will support Trusted Types, and these kind of changes to assign static HTML using TT policy would create a noise to all developers and security teams using those frameworks, just to review and be compatible (in case of Perfect Types) static HTML assignments.

I think and hope that the Sanitizer API can help solve this problem.

@mikewest
Copy link
Member

mikewest commented Feb 1, 2021

So we are in a better position to provide a safe way to avoid mXSS than developers. We are discussing potential solutions at #37.

For the sake of argument, let's assume for the moment that the sanitizer completely solves mXSS. In this world, re-parsing is no longer a security concern, but is probably still a somewhat-substantial performance concern. Different in kind, but still interesting to consider.

It seems to me that allowing a trivial route around those carefully-constructed policies could be dangerous in the face of custom elements and/or script gadgets that can convert attributes which appear inert to the browser into executable code.

If the custom elements and/or script gadgets is under a Trusted Types enforcement, Trusted Types should block those. If not, confused-but-not-malicious developers will create a bug which can bypass Trusted Types anyways by using DOM APIs, which will make Trusted Types not much valuable. AFAIK, the only sinks I know of are the Blob API and Dynamic import, in which the APIs itself needs to be fixed.

I think it's useful to examine the path to exploitation, given some unknown-to-the-browser sink. Let's assume something like data-bind; it seems reasonable to assume that a Trusted Types policy would understand the framework executing on the page, and build a mechanism which safely removes these attributes, along with others that might cause script execution. It seems equally reasonable that a well-meaning developer will use new Sanitizer(), assuming it does the right thing, which it likely doesn't.

That seems substantially more straightforward, and therefore more likely, than a developer intentionally working around the type system via DOM APIs.

A Sanitizer.sanitizeToTrustedHtml method doesn't seem to address this concern; it instead provides a mechanism that combines the risks of re-parsing with the risks of unaudited flow from an untrusted source to the DOM. That doesn't feel like the right integration point.

Given my above comments, I think something like Sanitizer.sanitizeToTrustedHtml will be useful. Because what Trusted Types needs to enforce is same or more lax than the Sanitizer API (because Sanitizer API will strip out JS URL where TT doesn't). The risks of re-parsing is something that we need to solve in the Sanitizer API as I mentioned.

  1. Does the Sanitizer API strip out JavaScript URLs? That isn't clear to me from the current spec (@otherdaniel).
  2. This doesn't match my mental model, in which Trusted Types can be assumed to be more strict than the sanitizer, given that it knows things about the application's handling of input above and beyond what the browser would consider executable.
  3. Also, I still think Consider enforcing TT for custom attributes. w3c/trusted-types#288 would be interesting to explore in this context, as it could help the sanitizer be more complete.

the risks of unaudited flow from an untrusted source to the DOM.

If we are talking about the DOM-XSS here, I think that the output coming out of the Sanitizer API with default config needs to be DOM-XSS free (assuming that TT is enforced).

It's unclear to me how this can be the sanitizer's responsibility in a world where framework developers continue to invent new and exciting ways to convert inert text into code. The sanitizer can get rid of things the browser knows will cause code execution. I don't see how we can expect it to get rid of everything else as well (especially in a world where the default configuration can't be overridden).

And with something like Sanitizer.sanitizeToTrustedHtml, security engineers doesn't need to audit places where developers assigns static HTML, by just using Sanitizer.sanitizeToTrustedHtml as well. This is something that will also help adoption of Trusted Types much faster, as Trusted Typing existing code will be as easy as checking for Sanitizer API availability, instead of creating policy and returning static HTML for all places where developers needs to use innerHTML.

Examples:

  1. https://source.chromium.org/chromium/chromium/src/+/master:ui/webui/resources/js/cr/ui/tree.js;l=320-321;drc=56de816e7df5874cc1775439ff5f0a6aabcdfd11
  2. https://source.chromium.org/chromium/chromium/src/+/master:ui/webui/resources/js/cr/ui/bubble.js;l=84-88;drc=6e82ffa4ab779589119ba9cf5d8a5fe6720b2deb
  3. https://source.chromium.org/chromium/chromium/src/+/master:out/Debug/gen/ui/webui/resources/preprocessed/cr_components/omnibox/cr_autocomplete_match_list.js;l=19-24;drc=279ef98c9e6917879e747d483abc2110a242ffd2

If the sanitizer is a reasonable way of ensuring that these static bits of code are indeed reasonable to inject into a page, great! We have two ways of spelling that, one which integrates TT into the Sanitizer (via Sanitizer.sanitizeToTrustedHTML) and one which integrates the Sanitizer into TT (via the TrustedTypePolicyOptions.sanitizer member suggested above). I think we'd agree that they have the same practical effect (assuming mXSS is taken care of).

If we're simply left with this aesthetic choice, my suggestion is that the story around the latter is more clear: Trusted Types enforcement ensures that untrusted strings run through a policy before they're injected into a page. That policy can (should!) be simply defined via a sanitizer configuration, or it can be more complicated and interesting. But it's the policy's responsibility to ensure application-specific rules are enforced, and the Sanitizer is a tool which can be used to perform that enforcement.

Now imagine that more frameworks will support Trusted Types, and these kind of changes to assign static HTML using TT policy would create a noise to all developers and security teams using those frameworks, just to review and be compatible (in case of Perfect Types) static HTML assignments.

What is "Perfect Types"?

@koto
Copy link

koto commented Feb 1, 2021

I think it's useful to examine the path to exploitation, given some unknown-to-the-browser sink. Let's assume something like data-bind; it seems reasonable to assume that a Trusted Types policy would understand the framework executing on the page, and build a mechanism which safely removes these attributes, along with others that might cause script execution. It seems equally reasonable that a well-meaning developer will use new Sanitizer(), assuming it does the right thing, which it likely doesn't.

That seems substantially more straightforward, and therefore more likely, than a developer intentionally working around the type system via DOM APIs.

The issue I see is that the DOM API is intentionally simpler to work with than the policies, so a bypass via DOM does not "feel" like a bypass. For me, the idiomatic way for a developer (or a 3rd party lib) outputs data to the page is DOM+strings. TT policy, with sanitizer or not is only successful is stopping bugs if the natural way doesn't work at all. As such, I'm not convinced that TT could eradicate app-defined XSS vectors. It might be helpful if complemented with other controls (grep for data- or dangerously and review). That said, if we find a solution for w3c/trusted-types#288 in a way that would block the raw DOM too at runtime, I'm sold.

A Sanitizer.sanitizeToTrustedHtml method doesn't seem to address this concern; it instead provides a mechanism that combines the risks of re-parsing with the risks of unaudited flow from an untrusted source to the DOM. That doesn't feel like the right integration point.

Given my above comments, I think something like Sanitizer.sanitizeToTrustedHtml will be useful. Because what Trusted Types needs to enforce is same or more lax than the Sanitizer API (because Sanitizer API will strip out JS URL where TT doesn't). The risks of re-parsing is something that we need to solve in the Sanitizer API as I mentioned.

  1. Does the Sanitizer API strip out JavaScript URLs? That isn't clear to me from the current spec (@otherdaniel).
  2. This doesn't match my mental model, in which Trusted Types can be assumed to be more strict than the sanitizer, given that it knows things about the application's handling of input above and beyond what the browser would consider executable.

A single policy can be made more strict. When limiting policy creation perhaps this could be said about all running policies too (I don't think this would compose well, as you probably would have a single policy with no rules whatsoever for various reasons), but the problem still remains that the raw DOM doesn't care too much about whether sanitizer or TT was used at all for custom sinks. So the imposed limitation holds only on request - if you remember to call a policy or a sanitizer, and ensure not to call the DOM instead. In practice, it's the latter that leads to bugs (after all, we had sanitizers and were grepping code for years) and neither TT nor Sanitizer can help here.

  1. Also, I still think w3c/webappsec-trusted-types#288 would be interesting to explore in this context, as it could help the sanitizer be more complete.

+100.

the risks of unaudited flow from an untrusted source to the DOM.

If we are talking about the DOM-XSS here, I think that the output coming out of the Sanitizer API with default config needs to be DOM-XSS free (assuming that TT is enforced).

It's unclear to me how this can be the sanitizer's responsibility in a world where framework developers continue to invent new and exciting ways to convert inert text into code. The sanitizer can get rid of things the browser knows will cause code execution. I don't see how we can expect it to get rid of everything else as well (especially in a world where the default configuration can't be overridden).

And with something like Sanitizer.sanitizeToTrustedHtml, security engineers doesn't need to audit places where developers assigns static HTML, by just using Sanitizer.sanitizeToTrustedHtml as well. This is something that will also help adoption of Trusted Types much faster, as Trusted Typing existing code will be as easy as checking for Sanitizer API availability, instead of creating policy and returning static HTML for all places where developers needs to use innerHTML.
Examples:

  1. https://source.chromium.org/chromium/chromium/src/+/master:ui/webui/resources/js/cr/ui/tree.js;l=320-321;drc=56de816e7df5874cc1775439ff5f0a6aabcdfd11
  2. https://source.chromium.org/chromium/chromium/src/+/master:ui/webui/resources/js/cr/ui/bubble.js;l=84-88;drc=6e82ffa4ab779589119ba9cf5d8a5fe6720b2deb
  3. https://source.chromium.org/chromium/chromium/src/+/master:out/Debug/gen/ui/webui/resources/preprocessed/cr_components/omnibox/cr_autocomplete_match_list.js;l=19-24;drc=279ef98c9e6917879e747d483abc2110a242ffd2

If the sanitizer is a reasonable way of ensuring that these static bits of code are indeed reasonable to inject into a page, great! We have two ways of spelling that, one which integrates TT into the Sanitizer (via Sanitizer.sanitizeToTrustedHTML) and one which integrates the Sanitizer into TT (via the TrustedTypePolicyOptions.sanitizer member suggested above). I think we'd agree that they have the same practical effect (assuming mXSS is taken care of).

If we're simply left with this aesthetic choice, my suggestion is that the story around the latter is more clear: Trusted Types enforcement ensures that untrusted strings run through a policy before they're injected into a page. That policy can (should!) be simply defined via a sanitizer configuration, or it can be more complicated and interesting. But it's the policy's responsibility to ensure application-specific rules are enforced, and the Sanitizer is a tool which can be used to perform that enforcement.

I think I side with @mikewest here. While it would be ok for the sanitizer instance to return TrustedHTML (because only native sinks matter in TT), using sanitizer within the TT policy (or another way of obtaining policies from sanitizers, but still guarding them as policies) feels more natural. Sanitizer API is meant for filtering out content, TT API is to guard values reaching sinks. I think conflating those two may limit expansion options in the future (we would always have to consider the sanitizer baseline and all possible config options, and whether they are sufficient for TrustedHTML still). We should make it easy to create TT policies from sanitizers, in a way that guardable. For Perfect Types, I think it's best if the sanitization happens directly to nodes, and that pattern encourages it.

Now imagine that more frameworks will support Trusted Types, and these kind of changes to assign static HTML using TT policy would create a noise to all developers and security teams using those frameworks, just to review and be compatible (in case of Perfect Types) static HTML assignments.

What is "Perfect Types"?

require-trusted-types-for 'script'; trusted-types 'none'. A mode in which policies can't be created, and your application is DOM-XSS-secure by construction, without requiring any review.

@mikewest
Copy link
Member

mikewest commented Feb 1, 2021

The issue I see is that the DOM API is intentionally simpler to work with than the policies, so a bypass via DOM does not "feel" like a bypass.

I think we're saying the same thing here?

That said, if we find a solution for w3c/webappsec-trusted-types#288 in a way that would block the raw DOM too at runtime, I'm sold.

This still seems pretty straightforward to me. Define a set of prefixes/suffixes/whatever, write them down somewhere, implement them in the sanitizer API (and maybe teach DOMPurify about them as well), and use those implementations to justify asking folks to start using them.

If the sanitizer is a reasonable way of ensuring that these static bits of code are indeed reasonable to inject into a page, great! We have two ways of spelling that, one which integrates TT into the Sanitizer (via Sanitizer.sanitizeToTrustedHTML) and one which integrates the Sanitizer into TT (via the TrustedTypePolicyOptions.sanitizer member suggested above). I think we'd agree that they have the same practical effect (assuming mXSS is taken care of).
If we're simply left with this aesthetic choice, my suggestion is that the story around the latter is more clear: Trusted Types enforcement ensures that untrusted strings run through a policy before they're injected into a page. That policy can (should!) be simply defined via a sanitizer configuration, or it can be more complicated and interesting. But it's the policy's responsibility to ensure application-specific rules are enforced, and the Sanitizer is a tool which can be used to perform that enforcement.

I think I side with @mikewest here. While it would be ok for the sanitizer instance to return TrustedHTML (because only native sinks matter in TT), using sanitizer within the TT policy (or another way of obtaining policies from sanitizers, but still guarding them as policies) feels more natural. Sanitizer API is meant for filtering out content, TT API is to guard values reaching sinks. I think conflating those two may limit expansion options in the future (we would always have to consider the sanitizer baseline and all possible config options, and whether they are sufficient for TrustedHTML still). We should make it easy to create TT policies from sanitizers, in a way that guardable. For Perfect Types, I think it's best if the sanitization happens directly to nodes, and that pattern encourages it.

If we agree on this, then I think potential differences of opinion in the discussion above don't really matter. So I'll just concede it all for the moment, and agree with you agreeing with me about a potential approach. :)

What is "Perfect Types"?

require-trusted-types-for 'script'; trusted-types 'none'. A mode in which policies can't be created, and your application is DOM-XSS-secure by construction, without requiring any review.

Got it, thanks.

@koto
Copy link

koto commented Feb 1, 2021

That said, if we find a solution for w3c/webappsec-trusted-types#288 in a way that would block the raw DOM too at runtime, I'm sold.

This still seems pretty straightforward to me. Define a set of prefixes/suffixes/whatever, write them down somewhere, implement them in the sanitizer API (and maybe teach DOMPurify about them as well), and use those implementations to justify asking folks to start using them.

Yes, for new sinks it's easy. For existing sinks (e.g. class attribute is special in AngularJS) -- probably impossible, not without significant portability problems (one can't type check programs if class is sometimes a TT sink, and sometimes not). Migrating libraries to use new sinks, however, is probably not terribly easy and needs buy-in :( That said, custom sinks are definitely a separate issue.

If we agree on this,

@shhnjk - wdyt?

@shhnjk
Copy link

shhnjk commented Feb 1, 2021

It seems to me that allowing a trivial route around those carefully-constructed policies could be dangerous in the face of custom elements and/or script gadgets that can convert attributes which appear inert to the browser into executable code.

If the custom elements and/or script gadgets is under a Trusted Types enforcement, Trusted Types should block those. If not, confused-but-not-malicious developers will create a bug which can bypass Trusted Types anyways by using DOM APIs, which will make Trusted Types not much valuable. AFAIK, the only sinks I know of are the Blob API and Dynamic import, in which the APIs itself needs to be fixed.

I think it's useful to examine the path to exploitation, given some unknown-to-the-browser sink. Let's assume something like data-bind; it seems reasonable to assume that a Trusted Types policy would understand the framework executing on the page, and build a mechanism which safely removes these attributes, along with others that might cause script execution. It seems equally reasonable that a well-meaning developer will use new Sanitizer(), assuming it does the right thing, which it likely doesn't.

Yes, guarding sinks in the frameworks is the job of Trusted Types, and not the job of Sanitizer API. This is why it's fine for Sanitizer API to return output which includes data-bind. As long as TT is enforced, as TT will block the assignment when frameworks tries to hit the native sink.

  1. This doesn't match my mental model, in which Trusted Types can be assumed to be more strict than the sanitizer, given that it knows things about the application's handling of input above and beyond what the browser would consider executable.

I said the opposite :) So your mental model is right. Sanitizer API is more strict than TT.

the risks of unaudited flow from an untrusted source to the DOM.

If we are talking about the DOM-XSS here, I think that the output coming out of the Sanitizer API with default config needs to be DOM-XSS free (assuming that TT is enforced).

It's unclear to me how this can be the sanitizer's responsibility in a world where framework developers continue to invent new and exciting ways to convert inert text into code. The sanitizer can get rid of things the browser knows will cause code execution. I don't see how we can expect it to get rid of everything else as well (especially in a world where the default configuration can't be overridden).

As I explained above, Sanitizer API's output with default config should be natively DOM-XSS free. If the document is TT enforced, other framework-level sinks will be handled by TT in the framework level.

If we agree on this,

@shhnjk - wdyt?

I think the change I'm suggesting is difficult to revert back later, so I'm okay with going with you guys' approach :)

But, I would love to see what I suggested in v2 or v3 or whenever we see the demand. I can already see a future where 3rd party libraries will be creating many TT policies, and allowed TT policy list in the header will be as much as what we see today in the allow-list of domains for CSP's script-src. In that world, I see security auditors crying, as they would need to create a grep tool for changes happening in allowed 3rd party TT policies and review changes in those (which are mostly static HTML assignments).

@otherdaniel
Copy link
Collaborator Author

Re-reading all of this... and still trying to form an opinion. I think there is one substantial decision to make; and most of the follow-ons seem to be more about API aesthetics.

  1. Keep Trusted Types and Sanitizer separate. That way, Sanitizer doesn't produce TrustedHTML, and it isn't a TT boundary. Spec-wise, it means I can drop this entire PR/section. (Although we might add some Sanitizer functionality, like the bound() function to make it easier to use within a policy.)

    The main casualty here is the "Perfect Types" use case. Those apps could freely use Sanitizer to produce node trees. But if they do want to sanitizeToString, they'd need to create at least one TT policy in order to use the result in .innerHTML and friends.

  2. Have some integration. In particular, allow Sanitizer instances to directly produce TrustedHTML instances. That would make is easier to write an XSS-free app, and then use TT (without any policies) as an enforcement mechanism, like a shutdown valve, to ensure no code paths have been forgotten. I do think this requires some sort of on/off-switch, so that developers who dislike this can remain in control. How that switch looks like, and what the default would be, is IMHO mostly a matter of API aesthetics.

I think that's the matter of substance. The remainders are largely a matter of taste.

I think # 1 would look like this:

  • This PR gets closed.
  • TT gets an enhancement that makes it easy to create a sanitizing policy.
  • Maybe the bound() function (or some other support for policy creation) would still be beneficial.

While # 2 could look like this:

  • Sanitizer gets a .sanitizeToTrustedHTML(input) method.
  • TT gets on/off switch for Sanitizer.
    • E.g. a CSP keyword: require-trusted-types-for 'script'; trusted-types 'sanitizer' // Allows any Sanitizer as TT source.

Have I correctly captured those options?

@shhnjk
Copy link

shhnjk commented Feb 3, 2021

Yup, overall agreed, and I want # 2, if possible :)

Few comments:

I do think this requires some sort of on/off-switch

I think that's a good idea. But then, we probably want to allow the following foo-policy to be automatically allowed (i.e. foo-policy doesn't need to be present in the trusted-types header) in the document when the 'sanitizer' keyword is present.

const TT_Policy = trustedTypes.createPolicy("foo-policy", {
  createHTML: new Sanitizer() });

Otherwise, frameworks has to have multiple if statements (and policy creation will still be necessary for functions that gets called more than once, unless the document allows 'allow-duplicates').

But if we decided to do this, I do have another idea that might conflict with this, so I would love to discuss about Sanitizer API in some sort of a meeting.

@Sora2455
Copy link

Sora2455 commented Feb 3, 2021

The main problem with option 2 is a lazy dev creating a sanitizer with allowed elements and allowed attributes set to include XSS sinks (like "script" or "onerror") can now completely bypass the protection that TT was supposed to offer.

@shhnjk
Copy link

shhnjk commented Feb 3, 2021

The main problem with option 2 is a lazy dev creating a sanitizer with allowed elements and allowed attributes set to include XSS sinks (like "script" or "onerror") can now completely bypass the protection that TT was supposed to offer.

Yes, that's the point I want to discuss in the meeting. So far, I think @mozfreddyb would never want to allow Sanitizer API to return such an untrusted output. So the concerns goes away. But I think it's useful for frameworks to have sanitizer which can include arbitrary allow-list. So my idea so far is that we should expose another thing called CustomSanitizer API, where it'll allow that, but it has no way to return TrustedHTML without a TT policy.

@shhnjk
Copy link

shhnjk commented Feb 4, 2021

// Example 1
const s = new Sanitizer({ config: goes, here: true });
el.replaceChildren(s.sanitize(input));

BTW @mikewest, if we don't integrate TT with Sanitizer API at all, s.sanitize(input) should throw a TT violation as it's a string to DocumentFragment conversion sink.

@mikewest
Copy link
Member

mikewest commented Feb 4, 2021

Hey, @otherdaniel!

  1. Keep Trusted Types and Sanitizer separate. ...
  2. Have some integration. ...

I mostly agree with this distinction (with the caveat @shhnjk notes above), though I don't think it's merely aesthetic. We're telling developers a story about how they can secure their sites. In that story, my core assumption is that it's possible for the sanitizer to be configured unsafely in the context of a given application. This seems likely even if the sanitizer's configuration options are substantially limited, given the number of frameworks out there on the world wide web.

With that in mind, the story that makes sense to me is something along the following lines:

  1. Trusted Types enforce easily-reviewable processing on untrusted strings before they can be injected into a context that can cause script execution.
  2. A sanitizer is one tool that can perform that processing function, but its configuration is an important security decision.
  3. Encapsulating a well-configured sanitizer in a Trusted Types policy both simplifies the task of reviewing the configuration, and ensures that it's used consistently.

The most practical approach to supporting that story is, IMO, something like the following:

  1. Break Sanitizer.sanitize if Trusted Types is enforced. Until we have more support for that mechanism generally, this would likely live as a monkey-patch in the Trusted Types spec that added the following step at the beginning of the sanitization algorithm. might look something like (though it should handle enforcement/reporting, of course):

    1. If the current global object's cspList contains a policy whose directive set contains a directive with a name of "require-trusted-types-for" and a value which contains 'script', throw a SecurityError.
  2. Document the suggestion that developers could use Trusted Types to enforce the application of their preferred sanitizer configuration:

    const superSafePolicy = trustedTypes.createPolicy("super-safe", {
      createHTML: (input) => {
        const superSafeSanitizer = new Sanitizer({ super: safe, sanitizer: configuration });
        return superSafeSanitizer.sanitizeToString(input);
      }
    });
    el.innerHTML = superSafePolicy.createHTML(maliciousInput);

    And they could rest easy, knowing that the following potential bypass would break:

    const terribleBadNoGoodSanitizer = new Sanitizer({ oh: noes });
    el.appendChild(terribleBadNoGoodSanitizer(maliciousInput));
  3. That's it. In particular, we'd still enforce naming constraints on the policies to prevent an unaudited sanitizer configuration from allowing unsafe strings to flow into executable contexts.

We could, of course, go further and improve the integration for this case, gently pushing developers towards the sanitizer as a good way of addressing their policy needs and avoiding the performance overhead of reparsing at the same time. As a strawman for bikeshedding, let's do that via new sanitizeFragment dictionary entry, and a corresponding createFragment method on the policy:

const superSafePolicy = trustedTypes.createPolicy("super-safe", {
  sanitizeFragment: { super: safe, sanitizer: configuration }
});
el.appendChild = superSafePolicy.createFragment(maliciousInput);

That seems like a pretty good place to end up, and would support the CustomSanitizer mechanism @shhnjk suggested (since we can rely on the policy creator to make good decisions about the configuration.

WDYT?

@mikewest
Copy link
Member

mikewest commented Feb 4, 2021

I missed this bit of @shhnjk's note:

I would love to discuss about Sanitizer API in some sort of a meeting.

This sounds good to me too. @otherdaniel / @lyf, perhaps y'all could move your next sync with @mozfreddyb to a @shhnjk -friendly time? I'd be happy to hop in, as I'm sure would @koto.

@koto
Copy link

koto commented Feb 4, 2021

lazy dev creating a sanitizer with allowed elements and allowed attributes set to include XSS sinks (like "script" or "onerror")

To clarify, the sanitizer API will always remove XSS sinks, that's the baseline and this issue relies on that fact. I don't think there are plans to regress to allow developers to allow onerror and such.

Thanks for all the comments, I feel we're getting somewhere :) Let me put another option out there...

  1. Keep Trusted Types and Sanitizer separate. ...
  2. Have some integration. ...

TT are not a good fit for guarding all strings->DOM transformations currently. I think we should enact boundaries that are strong, focused and consistent. Trusted Types is designed to guard carefully chosen sinks (which might include user-specified sinks in the future), and not to guard DOM node production. The only reason DOM manipulation is guarded is that some of the sinks happen to be in DOM. Further, the only reason TrustedHTML exists is that HTML can contain scripty nodes (that would bypass TrustedScript and TrustedScriptURL restrictions). As such, the current Sanitizer API and current TT could be mutually agnostic, as you could have regular DOM calls that result in the same thing as sanitizer without needing a Trusted Type instance - the sinks TT guards are not touched.

So far it looks like I'm arguing for option 1. But actually, some form of 2. is beneficial, because we might have custom sinks that the Sanitizer API is not aware of. Also, the sanitizer output is made to be written to innerHTML most of the times, and creating a policy wrapper then is a bit of a nuisance.

There's a few invariants I think we should preserve:

  • Sanitizer should be aware of the TT sinks, both the baseline sinks, and the ones users might configure in the future
  • The rules for filtering a value reaching a given sink type should be in a TT policy (an explicit one, or the default one).
  • Sanitizer should have the baseline behavior (no platform-defines XSS).

It suggests that maybe the sanitizer should wrap TT; it could call a provided TT policy for all the sinks that are TT-enforceable if they were not already rejected by the sanitizer, via its baseline or config:

// require-trusted-types-for 'script'; trusted-types 'foo'
trustedTypes.strawman.requireTrustedTypeFor('foo[data-xss-scriptUrl]', TrustedScriptURL);
trustedTypes.strawman.requireTrustedTypeFor('foo[data-foobar]', TrustedFoobar);

const sanitizer = new Sanitizer({
  some: config,
  trustedTypesPolicy: trustedTypes.createPolicy('foo', {
    createScriptURL: (i) => {
       // called by the sanitizer when it encounters foo[data-xss-scriptUrl]
       // not called for script.src as it gets removed by sanitizer itself.
    },
    createFoobar: (i) => {
      // my custom type validation, called on 'foo[data-foobar]'
    }
  }),
})

// Calls policy on all known sinks that were not already removed
// i.e. - only custom sinks.
nodes = sanitizer.sanitize(dirty) 

// Same, the output type doesn't matter, the processing respects TT. 
htmlString = sanitizer.sanitizeToString(dirty).

// Does not require createHTML in the policy, as all the TT sinks went through a policy anyway. 
trustedHTML = sanitizer.sanitizeToTrustedHTML(dirty)

The default policy also works:

// require-trusted-types-for 'script'; trusted-types 'foo'
trustedTypes.strawman.requireTrustedTypeFor('foo[data-xss-scriptUrl]', TrustedScriptURL);
trustedTypes.strawman.requireTrustedTypeFor('foo[data-foobar]', TrustedFoobar);

trustedTypes.createPolicy('default', {
    createScriptURL: (i) => {  ...  },
    createFoobar: (i) => { ... } 
    }
);

const sanitizer = new Sanitizer({
  some: config,
});

// Calls the default policy on all recognized sinks.
trustedHTML = sanitizer.sanitizeToTrustedHTML(dirty)

The advantage is that in the base case (no custom sinks defined in TT) the sanitizer never calls any policy, as all the sinks are already removed:

// require-trusted-types-for 'script'; trusted-types 'none'

// Just works. native sinks are never called, policy is not needed.
document.body.innerHTML = sanitizer.sanitizeToTrustedHTML(dirty)

... but the moment TT configuration gets more strict, you either need to adjust the sanitizer config (to reject the sinks early), provide a policy to the sanitizer (to filter the values for the sinks), or adjust your default policy for a fallback.

@otherdaniel
Copy link
Collaborator Author

[Meeting still needs to happen, of course.]

I've update the proposal, largely by removing stuff. The goal was to put a minimal version of choice # 2 in words, while not blocking any specific use case or future development.

So:

  • sanitizeToTrustedHTML is back.
  • There's an on/off switch in CSP.
  • Since the TT-check (in case of off switch) was apparently rather confusing, the off-switch now actually switches off.
  • I've adapted the explanation a bit to match.

Given where this currently is, this should arguably not go in this spec at all, but rather as a "monkeypatch" in to the Trusted Types one. Made a note of that.

@shhnjk
Copy link

shhnjk commented Feb 6, 2021

There's an on/off switch in CSP.

I think we should expose some way for developers to detect this on/off switch.

@otherdaniel
Copy link
Collaborator Author

otherdaniel commented Feb 8, 2021

I think we should expose some way for developers to detect this on/off switch.

Makes sense.

The potato way of detection is of course to just try-catch new Sanitizer().sanitizeToTrustedHtml("<p>"). But I think a read-only bool property on trustedTypes would make sense. (And would be easy to do.)

@koto
Copy link

koto commented Feb 9, 2021

Re switch, it's complicated :) w3c/trusted-types#36

I have a new sketch on how to make the default easy, to discuss at the meeting: https://gist.github.com/koto/7ad3b636a48a4fa39760dd2b88187162

@shhnjk
Copy link

shhnjk commented Feb 11, 2021

Can we all agree on the following first? That will make things little clear :)

  • Sanitizer API with any config will never return output which can result in an XSS natively (i.e. not considering frameworks' sinks or Script Gadgets)

Thumbs up if you agree, or comment if you disagree :)

@otherdaniel
Copy link
Collaborator Author

Sanitizer API with any config will never return output which can result in an XSS natively (i.e. not considering frameworks' sinks or Script Gadgets)

I agree, and I think this was generally agreed upon already. For quite a while now, the spec is written to have non-overridable built-ins, although we never quite agreed on their exact values.

@otherdaniel
Copy link
Collaborator Author

So... I read, and re-read the various proposals. It doesn't strike me that we're that far apart.

1, "Sanitizer never outputs XSS natively" is agreed.
2, It seems nobody is actually proposing a very "deep" integration.

The main differences I'm seeing is
1, which form the "perfect types" config takes, and
2, whether there's an "opt-out" switch for TrustedHTML-from-Sanitizer-without-policy.

On both items, I guess I still think that giving whoever configured the TT enforcement an option to decide for or against automatic Santiizer-to-TrustedHTML is a good idea.

@shhnjk
Copy link

shhnjk commented Feb 20, 2021

whether there's an "opt-out" switch for TrustedHTML-from-Sanitizer-without-policy.

Most of the options proposed are only opt-ins, and not opt-outs.
After seeing how much JS library needs to care about opt-ins, I don't like the idea of opt-ins for Sanitizer API returning TrustedHTML without policy.

Instead, I think the real options are:

  1. Always allow Sanitizer API to return TrustedHTML (the best options IMO).
  2. Allow developers to opt-out of Sanitizer API returning TrustedHTML without policy (I'm okay with this option if Koto is okay with it).
  3. Never allow Sanitizer API to return TrustedHTML without policy.

As stated above, I'm okay with 1 or 2. But I would love to avoid 3.

@mozfreddyb
Copy link
Collaborator

I have a slight preference, not to return a TrustedHTML from within the Sanitizer.
Ideally, that would happen within the TrustedTypes spec or within a policy implementation.
I think this will make the Sanitizer in more self-contained as a spec and certainly more usable for developers who can't adopt TT (because of site complexities for existing code or lack of browser support).

@koto
Copy link

koto commented Feb 22, 2021

For the reasons outlined in #59, I think the opt-out could be not about whether Sanitizer returns TT, but whether Sanitizer (or Sanitizer with a certain config) can be created in a first place. This detangles the sanitizer creation from the TT logic and decouples the sanitizer from TT policies. #59 proposes some mechanisms for doing that, which allow authors to construct guards that fit your application, including ones that limit Sanitizer to be only used within TT policies.

I also think that, given the above, it's OK to unconditionally make the Sanitizer return TrustedHTML (as authors with custom sinks and an interest in guarding them would have prevented the sanitizer instantiation).

So, to answer @otherdaniel':

  1. which form the "perfect types" config takes, and
  2. whether there's an "opt-out" switch for TrustedHTML-from-Sanitizer-without-policy.

Yes, but on new Sanitizer rather than sanitizer.sanitizeToSomething (and the guards do not have to use TT headers or any of the TT logic)

@shhnjk's options:

  1. Always allow Sanitizer API to return TrustedHTML (the best options IMO).
  2. Allow developers to opt-out of Sanitizer API returning TrustedHTML without policy (I'm okay with this option if Koto is okay with it).
  3. Never allow Sanitizer API to return TrustedHTML without policy.

That would be a combination of 1 and 2 (with every sanitizer you can get TrustedHTML but you can also prevent sanitizer creation - for example by making sure it's only created inside a policy).

@mozfreddyb:

[Removing sanitizeToTrustedHTML] will make the Sanitizer [...] certainly more usable for developers who can't adopt TT

Not quite sure I follow. Developers who can't adopt TT would just not call the function (or would not have it present at all). How is adding a function (that is useful for TT-y developers) makes the Sanitizer less usable for the others? FWIW using TT in parts of your application without yet enforcing TT (e.g. because your dependencies are not yet compliant) is pretty much how you usually use TT, dependencies don't block you from migrating the code you control to TT.

I think we should have a Sanitizer.prototype.sanitizeToTrustedHTML. It can be defined in TT spec, or here, it doesn't matter much. That said, I think #59 should be addressed as well, and if the solution is to build something into the browser (some solutions are user-land JS), it should be part of the Sanitizer API.

@otherdaniel
Copy link
Collaborator Author

It seems like we're not converging. At the risk of making things worse, maybe there's another way to slice our onion... :)

My main pivot here is around the "Perfect Types" use case. As I understand it, at its core it means that the developer writes an XSS-free app, and then uses Trusted Types to lock it down. However, if we rely on .sanitizeToTrustedHTML (or some other flavour of the same thing), then that isn't really the case. That means the developer still has to put in TT-specific code. While that could likely be easily hidden behind one or two function calls that determine whether TT is available and then switch between ...ToString and ...ToTrustedHTML, it still means the code has to be adapted to function unter the TT-lockdown.

With this in mind, how about we structure it like so:

  • The recommended use case is .sanitize (to fragment), and if an app consistently uses this then it can easily and safely enter TT-lockdown by always adding the CSP directive. (If a browser doesn't support TT, then the directive wouldn't do anything, and the app is just as safe.)

  • What if an app cannot be easily written to sanitize to fragments and - at least in some places - requires string handling? In that case, we have two options:

    • Install a default policy that calls a Sanitizer, inside a try-catch. This makes sense if the app developer believes that the remaining strings are already safe by construction. This would be a single call at app startup, and the TT lockdown directive could be safely included. (I think FF does something vaguely similar on their config page.)

    • If the above doesn't work either, then allow easy construction of a sanitizing policy. This could largely be a regular TT policy, but it could also support the Sanitizer methods (except returning TrustedHTML in the string part). This could rely on the regular TT directive, or alternatively have a blanket opt-in. This would be additional work at each (or some) Sanitzer instantiation, but other than that would not require code changes.


In more formal terms:

  • Sanitizer retains .sanitize and .sanitizeToString. No Trusted Types here.
  • Trusted Types gains a new method, .createSanitizingPolicy(String name, SanitizerConfig config).
    • This policy has the well-known TTPolicy methods.
    • This policy also supports the Sanitizer methods, except .sanitizeToString returns TrustedHTML
  • CSP trusted-types directive gains a 'sanitize' keyword to blanket-enable .createSanitizingPolicy for any name.
  • (Maybe also one to disable Sanitizer.sanitize, since from TT perspective - at least from my TT perspective - it is a bypass.)

This would require a reasonably clearly written TT+Sanitizer document, but I don't think that would be too difficult to write.

Advantages I'm seeing:

  • Clear(er) layering. TT sits on top of Sanitizer.
  • A clear, documented path to a zero-effort "Perfect Types" app, with some degree of low-effort flexibility if that path is unsupportable for an existing app.
  • It isn't quite what koto has been asking for in Consider respecting baseline settings for all sanitizers #59, but it strikes as being mostly compatible with it, with .createSanitizingPolicy being a potential segway into a more complete solution.

Any thoughts?

@koto
Copy link

koto commented Feb 22, 2021

"Perfect Types" use case. As I understand it, at its core it means that the developer writes an XSS-free app, and then uses Trusted Types to lock it down. However, if we rely on .sanitizeToTrustedHTML (or some other flavour of the same thing), then that isn't really the case.

Why is that not the case? The XSS free app with Perfect Types cannot have custom sinks, as the data passed to those sinks would eventually need to reach native sinks (some application code takes data-foo attribute and passes it to eval for example), and these are unreachable, because in Perfect Types there are no policies. IOW, in Perfect Types all you have is a Sanitizer, trustedTypes.emptyHTML and other inert type instances. None of them can "trigger" XSS.

Trusted Types gains a new method, .createSanitizingPolicy(String name, SanitizerConfig config)
This policy has the well-known TTPolicy methods.

What would createScript return? Do you have a way for customizing the rules of this TTpolicy? This would be another argument?

This policy also supports the Sanitizer methods, except .sanitizeToString returns TrustedHTML

So it's a union of Sanitizer and TrustedTypePolicy interfaces? You could also have a simpler thing without introducing the union or Sanitizer.sanitizeToTrustedHTML:

trustedTypes.createSanitizingPolicy = function(name, config) {
  const sanitizer = new Sanitizer(config);
  return trustedTypes.createPolicy(name, {
    createHTML: (s) => sanitizer.sanitizeToString(s);
  })
}

(Maybe also one to disable Sanitizer.sanitize, since from TT perspective - at least from my TT perspective - it is a bypass.)

Yeah, that's the one I don't like too much. It sounds like to cater to edge cases (custom sinks) we put roadblocks onto using the sanitizer. I think we would be better off if TT and Sanitizer were widely available, even if that means we're not making it easy to secure applications with custom sinks.

Clear(er) layering. TT sits on top of Sanitizer.

I think that's the core difference. I don't think TT should sit on top of the Sanitizer. There's only two parts of TT that make it relate to Sanitizer. (1) perhaps Sanitizer should create a TrustedHTML, as the HTML is inert (helps Perfect Types), and (2) some applications may use TT as a way to guard dangerous sinks that they define.

(1) is a nice to have. If this ends up impossible, so be it, one can always create a policy for the sanitizer. Not all apps sanitize too, so it's not "killing" Perfect Types for a lot of applications.

I think (2) would be misusing the TT API. Without a significant rewrite, and quite DOM-invasive patching it's not built to guard all DOM operations (including ones that change custom sinks). Customizing the TT guards (e.g. defining which sinks need which type) makes the applications less portable, and libraries difficult to write (can I do Node.className = 'foo' or do I need a TT policy? Should I consult TT every time I touch any attribute?). So I think 2) may be detrimental to TT.

But the more important part is that I think 2) is detrimental to Sanitizer, as it means that sometimes the API is not available at all, unless you reach it through TT, which complicates everything, including polyfillability. And this model (TT on top of sanitizer) only brings some security benefits to authors with custom sinks who use sanitizer and TT.

The facts are - if you have custom sinks, you're going to have a bad time. No platform feature can save you from errors - even TT + sanitizer with their most advanced integration still require you to make sure you're not interacting with raw DOM. But, even without any integration you still get useful primitives. TT make sure raw sinks are guarded, TT policies let you add some extra rules, reviewing sanitizer configs (potentially with #59) makes sure that user data doesn't trigger your custom sinks. You could combine policies with sanitizers if you want, but the API doesn't force you to, because otherwise you're putting an "integration tax" on everyone else.

@otherdaniel
Copy link
Collaborator Author

"Perfect Types" use case. As I understand it, at its core it means that the developer writes an XSS-free app, and then uses Trusted Types to lock it down. However, if we rely on .sanitizeToTrustedHTML (or some other flavour of the same thing), then that isn't really the case.

Why is that not the case? [...]

It's not the case because you can't write an app and then lock it down. What you have to do is write an app that works without TT; then TT-ify it by using .sanitizeToTrustedHTML, and then factor that into a wrapper that will be used depending on whether TT is supported in the current browser or not. And only then can you lock it down.

I'm assuming the goal is to provide an easy (as in: easily explained, and also easy to do) way to write an XSS-safe app, which should surely work in all browsers, and then us TT to lock it down when that is supported. Ideally, one could just add the CSP directive(s) and everything should continue to work.

(I guess we can argue whether "perfect types" is the proper name for this, or whether I'm describing something else and I should make up my own term for it.)


To re-iterate: What I'd like to do is provide "a well-lit path" to XSS-free apps. I don't much care what we call it. Here's a strawman of what I'd like to write in a well-lit-path-to-XSS-free-apps-document:

  1. When handling user data, either assign to .textContent, or sanitize to fragment with .sanitize. Do not pass around strings intended for markup creation.
  2. Add require-trusted-types-for 'script'; trusted-types 'sanitizer' (or 'none', or 'whateverweendupwith')
  3. Lean back and relax.

If you absolutely require string-to-innerHTML assignment, then:

  • If you think your .innerHTML assignments are always safe, then just add default to the CSP declaration and add a default policy like so:
    try { trustedTypes.createPolicy("default", createHTML: s => sanitizer.sanitize(s)); } catch (e) {} in your initialization.

  • If you're not always sure your .innerHTML assignments are safe, but you don't intend any script-y side effects, then create one or more TT policies that call a sanitizer, and use them where appropriate. (Here, examples start getting complicated.)

  • If you're not always sure your .innerHTML string assignment are safe, and if you want script-y side-effects under certain circumstances (e.g. your way of module loading requires it), then you have to take the long road. (TODO: long description of the long road.)

So.. this is a strawman. I'm happy with anything else. But it should have similar (or better) complexity. In particular, the base case should be simple.

@otherdaniel
Copy link
Collaborator Author

Trusted Types gains a new method, .createSanitizingPolicy(String name, SanitizerConfig config)
This policy has the well-known TTPolicy methods.

What would createScript return? Do you have a way for customizing the rules of this TTpolicy? This would be another argument?

I'd assume same behaviour as omitting createScript in a regular policy. (I'm not settled on this, though.)

This policy also supports the Sanitizer methods, except .sanitizeToString returns TrustedHTML

So it's a union of Sanitizer and TrustedTypePolicy interfaces? You could also have a simpler thing without introducing the union or Sanitizer.sanitizeToTrustedHTML:

Yes. My intention is that I could have a construct that - except for initialization - works the same with and without TT. The problem is without TT there are no policies, so I want to save the developer from having to invent some wrapper that will either call a plain sanitizer, or a policy that wraps a sanitizer. So if we have a policy that disguises itself as a sanitizer, we'd be there. (Admittedly, this particular idea is far from elegant. But it was the best I could think of.)

@koto
Copy link

koto commented Feb 22, 2021

Oh, I see. To rephrase how I understand what you meant, .sanitizeToTrustedHTML is problematic, because you need to branch on it (in case it's not supported), so adding it is not "perfect types". Similarly, trustedTypes.emptyHTML is like that. We used PT term in a slightly more narrow meaning, which is to say you can have TT enforcement without creating a single policy.

About the well-lit path for the XSS-free app, you need only TT to achieve it:

  1. the current behavior of require-trusted-types-for 'script'
  2. trusted-types 'none' OR a list of policies + policies security review.

I don't think the sanitizer changes anything here, regardless of the integration model. Even without any integration the sanitizer output cannot cause XSS without the data passing somehow through one of the policies later (the final XSS sink will always be native, and these are guarded). If you can get a TrustedHTML from a sanitizer, that's fine because it won't cause an XSS. Even if you write that tree to the DOM later on, you would have to read some custom attributes (data-foo-i-will-eval) and then push it to eval, but that requires a policy (DOM reads result in strings, some DOM writes require types).

Your code may incorrectly assume that all data-foo-i-will-eval are trusted (and pass it to some s=>s policy), but the security bug results from trusting DOM without validating the value -- and TT doesn't give you a guarantee that all DOM content is trustworthy, only that writing to XSS sinks requires a type.

well-lit path

With the above in mind, the well-lit path is:

  1. When handling user data, either assign to .textContent, or sanitize to fragment with .sanitize. Do not pass around strings intended for markup creation.
  2. Add require-trusted-types-for 'script'; trusted-types 'none'
  3. Lean back and relax.

If you need string-to-innerHTML:

  1. foo.innerHTML = sanitizeToBest(sanitizer, content), where sanitizeToBest can either branch on sanitizeToTrustedHTML existence, or use the policy, depending on what API shape we end up having. Without TT support, this just returns a string. So, pretty much what DOMPurify does already with { RETURN_TRUSTED_TYPE: true }. You could also create a default policy not to rewrite existing code.

With TT innerHTML assignments don't have scripty side effects, unless you created a TT policy that is passed something read from the DOM and that policy is happy to trust that value (i.e. you have script gadgets and just bless them with a TT policy). But that is clearly not something we should encourage or facilitate.

Yes. My intention is that I could have a construct that - except for initialization - works the same with and without TT.

+1, that's important, but there will always be some form of branching for TT and non-TT case. Either based on 'sanitizeToTrustedHTML' in Sanitizer.prototype, typeof trustedTypes !== 'undefined' or any other check. The default policy only offers an illusion that your code does not need to branch.

[...] I want to save the developer from having to invent some wrapper that will either call a plain sanitizer, or a policy that wraps a sanitizer.

I'm not sure I got this. How can you have the code that does not require a wrapper, such that it works both for Chrome+TT+TT headers and Firefox? The simplest thing is branching on sanitizeToTrustedHTML, but I don't see how using a sanitizer-as-policy would make the code more portable.

@shhnjk
Copy link

shhnjk commented Feb 22, 2021

Sorry, because of many comments above, I'm not going to reply all of it and just dump my opinion.

Just create a Sanitizer.sanitizeToHTML method which will either return a string or TrustedHTML.
Condition for Sanitizer.sanitizeToHTML to return a string are:

  1. The browser doesn't support TT.
  2. The site opt-out Sanitizer returning TrustedHTML by keyword (e.g. 'restrict-sanitizer')

This way, JS libraries just have to branch on Sanitizer availability. Because the only place where the app would break is when the site block sanitizer with 'restrict-sanitizer', in which case, the site should have default policy to fallback.

@koto
Copy link

koto commented Feb 23, 2021

Just create a Sanitizer.sanitizeToHTML method which will either return a string or TrustedHTML.

This would be an antipattern I think. Polymorphic return types force callers into making explicit checks for the return value type anyway in order to have any type safety. For example, in TypeScript this would require to type the function like

function sanitizeToHTML(params: any): TrustedHTML | string

which prohibits you from using the value as either type (you can't call .replace() on it, for example, before you make sure it's a string e.g. with an instanceof check). With TypeScript, at least you get the compiler error. Without a compiler, this leads to spectacular production fails. In other words, this helps only in the simplest case of .innerHTML = foo.sanitizeToTrustedHTML, but introduces complication and breakage risks for any case when the return value is not directly assigned to a sink.

@otherdaniel
Copy link
Collaborator Author

Just create a Sanitizer.sanitizeToHTML method which will either return a string or TrustedHTML.

As koto said, we've had customer bugs with this in the Trusted Type launch, where people called e.g. .substr and then had failures when the string turned out to be a trusted string. I guess one could have a designated sanitize-to--trusted-type-or-string method so people at least know what they can expect, but I'm still rather skeptical about this.

@shhnjk
Copy link

shhnjk commented Feb 24, 2021

  1. When handling user data, either assign to .textContent, or sanitize to fragment with .sanitize. Do not pass around strings intended for markup creation.

So are you saying that calling .sanitize shouldn't throw a TT error but we shouldn't create the sanitizeToTrustedHTML? That doesn't make sense to me.

@yoavweiss yoavweiss changed the base branch from master to main March 5, 2021 21:36
@otherdaniel otherdaniel closed this Oct 7, 2022
@otherdaniel otherdaniel deleted the trustedtypes-2 branch October 7, 2022 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants