-
Notifications
You must be signed in to change notification settings - Fork 74
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
Expose information on status of TrustedTypes enforcement #36
Comments
To understand your use case correctly, you'd like to know if any enforcement is enabled (i.e. string assignment to XSS sinks throws)? Given that it seems like custom runtime-defined policies are also planned (e.g. #32), it would still be possible to have an insecure configuration of the enforcement. That would be OK though, as it's the application defining that policy. |
Coming back to this, I'm not sure if we should encourage checking for enforcement. Given that TTs are backwards compatible when used with DOM sinks, it might be better if the libraries check for support alone. That encourages the libraries to use the types, as soon as they are available. For example, Angular or jQuery might accept a TT-typed input without sanitization, while still performing runtime sanitization if a string was passed. The check then becomes a simple
Enforcement is controlled by the response header (or some polyfilled equivalent), but if TTs are supported, you should use them, even if enforcement is currently disabled. Otherwise, the application takes different code paths in Dead code eliminationThis still enables dead code elimination, by providing a different externs file (without TT) at compilation time. The following safety check: if (platform.HAS_TRUSTEDTYPES && !window.trustedtypes.isEnabled()) {
// fail noisily and decisively; prevent further initialization of the app;
} is technically not needed - the application would functionally break if it tries to use the TT API in a non-supporting browser. Does this make sense? |
I see your point about not encouraging diverging code paths depending on whether TT are enforced. However, I do think we need a way for application startup code to reliably tell whether enforcement is in place, especially if fallback policies are in use: A fallback policy allows application code to rely on its presence; e.g. However, this code is unsafe if TT is not enforced (e.g. due to a CSP header misconfiguration). And, in a browser that supports TT, the app would not functionally break, but be potentially vulnerable. Thus, authors of apps that are written with the assumption of TT being enforced (with fallback) would want to add a check in their bootstrap code. |
If I understand correctly, what would be needed is not You can check for the existence of the fallback policy like this: 'TrustedTypes' in window && TrustedTypes.getPolicyNames().includes('fallback') But you're right - if TTs are not enforced, that policy would never be called, leaving the application potentially vulnerable. On the other hand, if a library is already TT-aware, why not just start using the fallback policy explicitly regardless if it's enforced? const useFallback = 'TrustedTypes' in window && TrustedTypes.getPolicyNames().includes('fallback');
...
node.innerHTML = useFallback ? TrustedTypes.getExposedPolicy('fallback').createHTML(foo) : foo Of course we can make the API a bit simpler for using the fallback policy (e.g. |
FWIW, one could presumably check whether or not any policy is enabled as follows, right?
|
Yes.
…On Wed, Jun 20, 2018, 22:51 xtofian ***@***.***> wrote:
FWIW, one could presumably check whether or not any policy is enabled as
follows, right?
'TrustedTypes' in window && TrustedTypes.getPolicyNames().length > 0
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#36 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAH0q7wNdjyGz-HBmpj0R81JtUBRaqEsks5t-rXCgaJpZM4Q4bvo>
.
|
I'm not sure
makes all that much sense.
has exactly the same effect as
So the It might be preferable to encourage a pattern for library authors such that
I.e. something like
|
The difference is I agree completely with the second part, because it's using TTs explicitly, and leaves the option for the authors to control the behavior of the libraries. |
But doesn't the `node.innerHTML = useFallback ? ... : foo` also become
vulnerable if policies are not enforced? If they're not enforced,
`useFallback` is false (because if polcies are not enforced, there
definitely is no fallback either). And then the RHS just becomes `foo`.
(I feel like I'm missing something important :)
But in any case, I'd tent to agree that fewer knobs are better.
…On Thu, Jun 21, 2018 at 4:28 AM Krzysztof Kotowicz ***@***.***> wrote:
has exactly the same effect as node.innerHTML = foo;
The difference is node.innerHTML = foo leaves you vulnerable if the
policies are not enforced. Since this is a shorter form, it encourages a
bad pattern for the libraries i.e. ignoring types alltogether and relying
on the CSP to enforce the security. That's why I'm not so sure anymore that
the fallback policy is the best solution, at least in the current
definition of it. IMHO branching on *enforcement* (instead of TT support)
introduces yet another knob that the authors might get wrong.
I agree completely with the second part, because it's using TTs
explicitly, and leaves the option for the authors to control the behavior
of the libraries.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#36 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AED2jspYAKdN7sFh2ww7TRHeWlQKlNfyks5t-4NAgaJpZM4Q4bvo>
.
|
Oh, I assumed useFallback is true iff fallback policy is defined (like in #36 (comment)). I assume there is no way (short of In such model, the statement with the branch is only vulnerable in one condition: legacy browser & no polyfill. Even if policy is not enforced, the library uses the (defined and available) policy, sanitizing the value before it reaches the sink. |
Ah, now I get it. Ok, that makes sense. In practice, one would wrap the RHS into a function to avoid the clutter all over the app. Perhaps a reference implementation of TrustedTypes builders could provide this as a helper? |
BTW, if fallback policies are enabled, That seems a little clunky and brittle, since whether or not this works depends on what the fallback policy actually does.
|
I see that a few things have changed since this conversation, however I would like to add my point of view. I think fundamentally there are three different cases when we talk about TT in applications:
All of these categories represent a state which an application or library might want to handle differently. For example, if existing sanitizing libraries like DOMPurify would like to integrate TT, they would make their sanitizing functions return trusted values instead of strings. However, what they cannot assume that they should return trusted values because of TT being available in the global object, because user might want to rely on the value being a string (and invoke This implies that there has to be some way to tell, whether the application wants to use TT or not. I strongly think this value should only be
It would be very surprising if step 3) could cause a different behavior than step 2). I also don't think this will create a hard times for security reviewers. Of course instead of code like: if ('trustedTypes' in window) {
...
} else {
...
} the code would look like: if ('trustedTypes' in window && window.trustedTypes.shouldUseThem) {
...
} else {
...
} However, if we make sure the @koto does this make sense? |
Yes. Now let's bikeshed about the flag name :) |
I've previously stated: However I forgot that users could do: try {
document.createElement('div').innerHTML = 'string'
// we are in report-only mode
} catch (e) {
// we are in enforcing mode
} This is a huge anti-pattern as this will also log a violation and I think we shouldn't motivate use case like this and we should just let developers know whether to use Trusted Types or not (a.k.a boolean field). EDIT: @koto pointed out that might not work because |
Has this discussion reached consensus? I'm presently looking into implementing this. My proposal would be a read-only attribute isRequiredForScript, since that's what we ended up calling the CSP directive that enables TT. I also think this should be true for both enforcing and report-only (since report-only quite clearly wants the app to use TT; otherwise the reports would be pretty useless). |
That sounds good, thanks for that! Alternatively I also think it should return |
I don't think this feature is a good idea. Returning true in both report-only and enforcing mode just moves the issue earlier. Enabling report-only mode should be a no-op, adding this feature means that now enabling report-only mode can break you application. |
Ah, I see. So, if report-only should be a no-op so that you can safely monitor DOM usage, then we can't inform the app whether report-only is enabled. The initial use cases are all about code, especially third-party libraries, that might usefully adapt some of their behaviour based on TT enabled/disabled. These seem to be contradictory, so I guess it's a matter of weighing the use cases against each other? |
The application, in general, can almost always find out whether it runs under CSP, and what its mode is (
Yes, modulo the above.
I think it is, and I see the value of adding such a flag. But, on the other hand, application breaking functionally when |
Having discussed this, I feel that if we want to expose the API, we probably want to do this at the CSP level (our chosen delivery mechanism), rather then solving it for Trusted Types only. Some previous work in this area: |
Something like
window.trustedtypes.isEnabled(): bool
.This would be useful in a number of scenarios:
Frameworks that support strict contextual escaping and have their own notion of types (e.g. Angular, or Polymer with polymer-resin) may want to turn off their own mediation of assignments to DOM properties (e.g. Angular's DomSanitizer) and rely on the platform-provided mediation.
Existing libraries might have to suppress (legacy) functionality that's incompatible with TrustedTypes. E.g. jquery seems to be doing some HTML markup manipulation, which is potentially prone to security bugs and presumably should be disabled in a TrustedTypes-enforced app. The library could check at run-time whether or not TrustedTypes are enforced and alter behavior accordingly.
In scenarios where JS code is compiled and optimized, it's desirable to elide framework code that implements features provided by the platform. For instance, polymer-resin implements mediation (i.e., context-aware sanitization) of values flowing into DOM XSS sinks via Polymer templates. It does so by hooking into Polymer to interpose a context-aware value sanitizer into template data bindings. The sanitizer consists of a fair bit of code and metadata to determine how a value should be sanitized based on the destination context. With TrustedTypes enforced by the browser, most of this code should be unnecessary, and it'd be desirable to not ship it to browsers.
In a compiled setting, this can be accomplished via some form of preprocessing; e.g. using Closure compiler's
@define
mechanism combined with dead-code elimination. I.e., we might define:and then condition framework code on this flag. E.g. polymer-resin would essentially have two implementations distinguished by this flag, a full one (for
!HAS_TRUSTEDTYPES
) and a light-weight one that assumes TrustedTypes. When compiled with--define='platform.HAS_TRUSTEDTYPES=true'
, the bulk of the polymer-resin implementation would be compiled away.This however raises a configuration risk: If the web server were to ship the
HAS_TRUSTEDTYPES
JS code into a context where TrustedTypes are not actually enabled (e.g. b/c the header was not sent), the client is in an insecure configuration.To guard against this, we can include a safety check somewhere in the initialization code, along the lines of,
The text was updated successfully, but these errors were encountered: