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

Allow future extensions to the API without breaking compatibility #241

Closed
koto opened this issue Nov 18, 2019 · 20 comments
Closed

Allow future extensions to the API without breaking compatibility #241

koto opened this issue Nov 18, 2019 · 20 comments

Comments

@koto
Copy link
Member

@koto koto commented Nov 18, 2019

Pulling out a comment from mozilla/standards-positions#20 (comment).

The scope of the API will be re-reviewed to ensure it's forward compatible with adding more primitive enforcement points.

The gist of the issue is that while TT cover DOM XSS now, we should allow ourselves to extend its approach to other security problems (e.g. data exfiltration or keyboard sniffing via CSS #104). The API itself should also be backwards-compatible - e.g. implementing new types in the browser should not break existing websites that use TT for DOM XSS only. Citing @otherdaniel :

if we imagine a new TrustedStyle type to be added, just like current types are, then every TT-enabled website would instantly shoot itself in the foot because en empty createStyle callback in the policy would block all style assignments... I do think this needs to be fixed.

  • One idea on how this could be achieved is opting in to each enfrocement area (e.g. DOM XSS, CSS) separately - for example declaratively via a CSP keyword i.e. #66.

  • Another is to make the (default) policy createXYZ permissive by default (one would have to explicitly set createXYZ value to null or a custom function to get the failing close behavior), so for example:

    trustedTypes.createPolicy('default', {
     createHTML: null,
     createScript: null,
     createScriptURL: null
    })

    would blockDOMXSSy types, but allow CSS, whereas

    trustedTypes.createPolicy('default', {})

    allows all current and future types. The problem is that it fails open.

@koto

This comment has been minimized.

Copy link
Member Author

@koto koto commented Nov 18, 2019

/ cc @jonathanKingston @otherdaniel @xtofian from previous discussions.

@otherdaniel

This comment has been minimized.

Copy link
Contributor

@otherdaniel otherdaniel commented Nov 18, 2019

The problem is that it fails open. True, but: I don't see how we can be future-proof without some degree of "fail-open". Would it make sense to distinguish between "fail-open" for existing sink types (which is IMHO undesired), and "fail-open" for future sink-types that the current code-base didn't yet know about (which is IMHO unavoidable)?

Maybe we could have a syntax for unknown sinks (like: { createScript: ..., createOther: ... }) that a developer could use to really definitely opt-out of all future enhancements.

That said, I think that whole avenue is a gigantic foot-cannon: Trying to opt-out of future, yet-unspecified behaviours sounds like something that you'd want, but that in practice will probably have a an overwhelming likelihood of having undesired side-effects. I strongly suspect that anything that blocks yet-unspecified, future sink-types is likely going to be more hassle than it's worth.

@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Nov 21, 2019

The problem is that it fails open.

True, but: I don't see how we can be future-proof without some degree of "fail-open".

I think I agree with @otherdaniel here. It seems reasonable to me for the default behavior to be permissive by default. That is, a default policy with undefined interpreted as equivalent to a passthrough seems like a fine way of providing ourselves with room for future extension that won't break sites. We could allow developers to tighten it either via the syntax Daniel described above, or a 'strict-whatever' keyword, or etc. I don't think many would, but developers who value future compatibility less than security could certainly do so.

Given that the story we're telling here is one of auditing and review, it seems reasonable to fail open in the absence of an explicit declaration, given that that "failure" would itself be reviewed.

for example declaratively via a CSP keyword

This also fails open, doesn't it? If we're going to fail open either way, I prefer @otherdaniel's approach. :)

@koto

This comment has been minimized.

Copy link
Member Author

@koto koto commented Nov 21, 2019

I would prefer if there would be a CSP keyword to lock down the setting. The problem with using only the default policy for that mechanism is that it would have to be part of the JS, called once and before any DOM sinks are called. Default policy was initially meant to relax the enforcement, not tighten it (just like other policies). And since CSP is processed before the JS kicks in, that seems to match well to what we want. CSP defines limits for your application code, JS adheres to those restrictions.

@koto

This comment has been minimized.

Copy link
Member Author

@koto koto commented Nov 21, 2019

How about if we defined it like so:

trusted-types policy names and potential future rules that guard policy creation go here; 
script-src 'trusted-types';
style-src 'trusted-types';
object-src 'trusted-types';

That limits the APIs that we have to guard to existing CSP buckets. And there's no good place for TrustedHTML, although this can be defined as: if any directive has a 'trusted-types' keyword, TrustedHTML is required, as it has the capability of creating any type indirectly.

@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Nov 22, 2019

TL;DR: How about trusted-types names and stuff; require-trusted-types-for 'script'?

Default policy was initially meant to relax the enforcement, not tighten it

This is a fair point. I might be overindexing on the default policy's role in easing deployment, and the assumption that such a policy would exist. It certainly true that we want to support pages that don't define such a policy as well. Hrm.

Let's look at the CSP integration again:

How about if we defined it like so:

trusted-types policy names and potential future rules that guard policy creation go here; 
script-src 'trusted-types';
style-src 'trusted-types';
object-src 'trusted-types';

That limits the APIs that we have to guard to existing CSP buckets.

My initial reaction to that is that it might be confusing, as it also sets expectations that connect-src 'trusted-types' and img-src 'trusted-types' would be available and meaningful. It's also a bit strange to put a tightening mechanism in a keyword, when other keywords generally loosen a policy's requirements.

And there's no good place for TrustedHTML ...

I think I'd like to tie enforcement requirements to the semantic meaning of the relevant sinks, as opposed to the policy used to construct a type that fits that sink. The core of what TT is doing today seems to be management of script execution. TrustedHTML exists not because we care deeply about the ability to inject arbitrary HTML into a page, but because that injection can cause script execution, and unexpected script execution is bad.

If we go this route, it seems like we want a page's policy to do two things: define the list of policy names available on a given page, and define the scope for which those policies are required. The former seems reasonably placed in the trusted-types directive. I'm not thrilled with tying the our current behavior for the latter to script-src and/or object-src, as I don't think there's any case in which it would be sane for us to allow developers to opt into the one but not the other, as the semantics of both are similar: they cause script execution.

I think it might be better to add a require-trusted-types-for 'script' directive (similar to the require-sri-for directive we almost shipped) in order to define the mechanism's scope. I'd also suggest that we default its value to 'script', which would be defined to encompass the sinks we currently cover. If we add new sinks in the future, we can decide whether they're really semantically the same as the others, and require enforcement along with the others, or whether they're a new category of thing (stylesheets, etc).

I'm not super-happy with that mechanism from an aesthetic perspective, as it seems like when we introduce 'style', we'll just require folks to type require-trusted-types-for 'script' 'style' forever. But maybe that's not a terrible thing.

@koto

This comment has been minimized.

Copy link
Member Author

@koto koto commented Nov 22, 2019

That LGTM for all the reasons you mentioned (it has a slight advantage that it simplifies the configuration for what is likely to be the most common setting i.e. DOM XSS prevention).

Unless anyone feels strongly opposed, I'd go along and spec it like this. Going once, going twice, ...

@koto

This comment has been minimized.

Copy link
Member Author

@koto koto commented Nov 22, 2019

To clarify - would the existence of trusted-types directive imply the behavior of require-trusted-types-for 'script' if this directive is not present? If so, then we'd likely need require-trusted-types-for 'none' or an empty directive value to opt-out of this.

Additionally html would also be relevant for styles, so I guess TrustedHTML will be required anytime a respective sink can be triggered via HTML. i.e. require-trusted-types-for 'script' and require-trusted-types-for 'style' imply TrustedHTML is guarded as well.

@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Nov 22, 2019

To clarify - would the existence of trusted-types directive imply the behavior of require-trusted-types-for 'script' if this directive is not present?

That's what I'd intended, yes.

If so, then we'd likely need require-trusted-types-for 'none' or an empty directive value to opt-out of this.

Why? Is there any case in which someone would have an enforced CSP that restricted the names of policies available on a given page, but wouldn't also want to enforce TT requirements for sinks? Isn't that behavior what Content-Security-Policy-Report-Only is for?

Additionally html would also be relevant for styles, so I guess TrustedHTML will be required anytime a respective sink can be triggered via HTML.

Yes. In this model, sinks that require TrustedHTML would be enforced any time any category of trusted type is enforced.

@otherdaniel

This comment has been minimized.

Copy link
Contributor

@otherdaniel otherdaniel commented Nov 25, 2019

Just so I get this... There'd be
require-trusted-types-for 'script';
which is effectively a constant one adds to the CSP. Absence of that constant is considered to imply presence of the same.

  • Is there any situation - in version 1 - where require-trusted-types-for (presence; absence; or any values) would make any difference?
  • What about invalid values? require-trusted-types-for 'potatosalad' ?
  • If TT is being extended, which is what we're discussing here, then require-trusted-types-for would gain support for several values, but the value implied by absence would continue to be 'script', right?
@koto

This comment has been minimized.

Copy link
Member Author

@koto koto commented Nov 25, 2019

Just so I get this... There'd be
require-trusted-types-for 'script';
which is effectively a constant one adds to the CSP. Absence of that constant is considered to imply presence of the same.

  • Is there any situation - in version 1 - where require-trusted-types-for (presence; absence; or any values) would make any difference?
    I can imagine require-trusted-types-for 'none' value switchng off the enforcement? (that is, assuming there are no other CSPs that re-enable it). I think an empty directive should not imply 'none' (also to solve the below)
  • What about invalid values? require-trusted-types-for 'potatosalad' ?

Invalid keywords should be ignored I think, so in this case you end up with empty directive that implies 'script'.

  • If TT is being extended, which is what we're discussing here, then require-trusted-types-for would gain support for several values, but the value implied by absence would continue to be 'script', right?

Correct.

@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Nov 26, 2019

I can imagine require-trusted-types-for 'none' value switchng off the enforcement? (that is, assuming there are no other CSPs that re-enable it).

Yes, though I'm not sure why we'd offer that option.

I think an empty directive should not imply 'none' (also to solve the below) ... Invalid keywords should be ignored I think, so in this case you end up with empty directive that implies 'script'.

Neither of these are consistent with CSP. That is, script-src; does not imply "Load all the script!" but "Don't load script." If we add something like require-trusted-types-for, then I'd suggest that we implement it's parsing in a manner similar to other directives: invalid keywords are ignored, valid keywords are accepted, and the list of valid types provided is a complete representation of the scope.

What I'm suggesting above is that we treat the presence of the trusted-type as implying require-trusted-types-for 'script' unless that directive is specified. As @otherdaniel notes, we'd keep that implication even if Trusted Types grew new types in the future.

I'm also comfortable with leaving nothing to the imagination, and requiring the boilerplate require-trusted-types-for 'script' to be included in every policy. That's more clear, and ensures that forward compatibility is maintained explicitly. It's also superfluous until and unless we extend the mechanism, which is unfortunate.

@koto

This comment has been minimized.

Copy link
Member Author

@koto koto commented Nov 26, 2019

@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Nov 26, 2019

Neither of these are consistent with CSP. That is, script-src; does not imply "Load all the script!" but "Don't load script."

That's because adding to most other directives relaxes the setting. *-for directives do the opposite, which might explain a different logic here. It's just a though.

That's a reasonable point. Perhaps require-trusted-types-for 'not-a-thing' does mean that we wouldn't enforce anything. Interesting!

If we run with that interpretation, then I think we need to make people type require-trusted-types-for 'script'. It's a bit of boilerplate, but it's not the worst thing we'll have asked people to type. This means that most developers would end up with a policy containing: trusted-types name1 name2; require-trusted-types-for 'script'.

@jonathanKingston, @annevk: It would be quite helpful for y'all to weigh in from Mozilla's perspective around the require-trusted-types-for suggestion that we seem to be coalescing around. :)

@koto

This comment has been minimized.

Copy link
Member Author

@koto koto commented Nov 28, 2019

Actually, it seems like the common setting might be require-trusted-types-for 'script' only with no trusted-types - which seems OK from the authors perspective. trusted-types only controls the allowed policy name whitelist - and the lack of it should imply that auhors do not wish to restrict which policies are created - which might be a case for a lot of integrations.

With that logic, we could even remove trusted-types * from the spec, as the absence of the directive has the same meaning.

@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Nov 28, 2019

I like that way of looking at it. require-trusted-types-for 'script' for everyone!

Should we then rename trusted-types to something like trusted-types-policy-names?

@otherdaniel

This comment has been minimized.

Copy link
Contributor

@otherdaniel otherdaniel commented Nov 28, 2019

Makes sense to me.

Names: I'd prefer trusted-types-policies over trusted-type-policy-names, for brevity.

I take this is orthogonal to 'allow-duplicates' for the names, right?

@koto

This comment has been minimized.

Copy link
Member Author

@koto koto commented Nov 28, 2019

I'm indifferent re: the naming. trusted-types, while not precise, makes more sense if we decide to put keywords that define how types (and not policies) behave - e.g. 'disallow-cross-document' etc. We could also just add a new directive then though. 'trusted-types-policies' gives us more flexibility in case there is a better mechanism to control policy creation than names.

So I guess the decision here is rather if we aim for more directives in the future, or more keywords. Either seems fine.

I think this is orthogonal to 'allow-duplicates', i.e. there should be a keword to control that - and not in require-trusted-types-for.

@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Dec 2, 2019

So I guess the decision here is rather if we aim for more directives in the future, or more keywords. Either seems fine.

I think either trusted-types or trusted-types-policies would be reasonable. Flip a coin?

I think this is orthogonal to 'allow-duplicates', i.e. there should be a keword to control that - and not in require-trusted-types-for.

'allow-duplicates' seems quite related to the policy names, so it would fit reasonably in trusted-types-policies (or whatever).

@koto

This comment has been minimized.

Copy link
Member Author

@koto koto commented Dec 2, 2019

I have a slight preference to leave it under trusted-types, it's simpler. trusted-types-policies would have to be singular I think, like rest of directives. I'll spec it like so, renaming later is easy.

koto added a commit to koto/trusted-types that referenced this issue Dec 2, 2019
@koto koto closed this in 49bff23 Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.