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

Fallback policy support #63

Closed
koto opened this issue Jun 20, 2018 · 6 comments
Closed

Fallback policy support #63

koto opened this issue Jun 20, 2018 · 6 comments

Comments

@koto
Copy link
Member

koto commented Jun 20, 2018

Fallback policy is a single, exposed TT policy in the realm that gets called implicitly, when the DOM sink is used with a string. Example:

TrustedTypes.createPolicy('fallback', (p) => {
  p.createHTML = (s) => reallyConservativeSanitizer(s);
  ...
  p.expose = true;
});

// legacy code, e.g. in a widget
domElement.innerHTML = 'a string'; // will call 'fallback' policy underneath.

Fallback policies fit well into the generic TT design, but it's unclear yet whether we should support them.

Pro:

  • Substantially facilitates integration with existing codebase, especially when non-controlled code is used in a website (widgets, 3rd party libraries). For example, Google Analytics inserts a tracking image, and it's hard to expect Analytics to change.
  • Trivial to polyfill (it was introduced in Added fallback policy support. #46, now disabled by default)

Con:

  • Strong incentive to make the fallback policy liberal. It has to support all possible, functionally valid DOM interactions of all the runtime dependencies. In practice reallyConservativeSanitizer might break most applications, and the easiest solution is to make the fallback policy a no-op (s) => s. This might become the equivalent of unsafe-inline - easy to adopt, but not improving the security posture.
  • As all exposed policies, it's harder to reason about its security, as its usage is spread throughout the whole application code. A liberal policy might not introduce vulnerabilities, but one can't tell that by looking at the policy alone.
  • No mechanism to control the usage of this policy (it has to be exposed)
  • Difficult to implement in the browsers (strings are blocked at the IDL level); @mikewest suggests this can be a userland implementation.
@koto
Copy link
Member Author

koto commented Jun 29, 2018

The fallback behavior might ease adoption significantly. This may be implemented either as a static configuration (e.g. a set of CSP keywords promoting certain strings to typed values), or fully dynamic, with a policy. One example where this helps is #65 wih url-allow-http keyword.

@arturjanc
Copy link

This feature seems like one of the more exciting parts of Trusted Types -- I really like it. The main benefit is giving developers a path to deploying TT without requiring them to refactor all widgets and libraries to use Types for DOM assignments; this provides an escape hatch not only for the common case of using third-party widgets, but also in many situations where the application is composed of different first-party modules, some of which may be more difficult to update than others.

Developers could incrementally refactor their applications to use TT in the most important parts of the app (e.g. rendering of client-side HTML templates), while using a fallback policy for other string assignments and potentially implementing reporting to identify the next most important candidates for refactoring to TT. It would also help with a large subset of legacy static HTML sites which use dangerous DOM APIs, but where the values are more likely to contain strings which could be sanitized without user-visible impact.

Basically, this is a great feature in its own right and it fits in very well with Trusted Types. @mikewest WDYT?

(FWIW it seems somewhat cleaner to me to do this dynamically in a policy & userland implementation than with static keywords because I imagine a declarative approach might not be expressive enough here.)

@mikewest
Copy link
Member

I think I'm worried about the performance impact of adding more conditional hooks to every DOM API. We're already seeing TT show up in performance graphs, just because some of these APIs are widely used enough to make any additional comparisons expensive.

If it will help adoption as much as y'all think it will, then it sounds like it would be worth prototyping to figure out what the cost would be, and whether we can pay it.

@arturjanc
Copy link

It agree it's is an important concern, but can you clarify which aspect of TT performance you'd be worried about in this context? I expect that there will be runtime impact associated with running the userland sanitizer, but unlike the conditional hooks on every DOM API, the hit here would be confined to pages with have TT enabled and perform string assignments to DOM APIs.

Very naively, I see this as:

// innerHTML setter
if (TT.enabled()) {
  if (typeof(arguments[0]) == "TrustedHTML") { /* Do the usual thing */ } 
  else if (typeof(arguments[0]) == "String") {
    // This is the new part: 
    if (TT.hasPolicy('fallback')) { /* Run the fallback policy on the argument */)
    // done.
    throw; 
  }
}

... in which case the comparison would only happen on pages which opt into TT without having fully switched to types, and which are presumably okay with the performance hit if we document it well enough. Or am I taking crazy pills? ;-)

@mikewest
Copy link
Member

You're not wrong about the complexity, but simply adding the types already shows up on some performance graphs (we've already had to revert and rework one patch due to regressions). DOM APIs like innerHTML are hot paths, and adding complexity to them is expensive.

I'm not saying we can't do it. I'm saying we need to be careful, measure the impact, and make a decision once we know the cost.

@koto
Copy link
Member Author

koto commented Jan 16, 2019

Closing for now - this is implemented in polyfill and Chrome. The policy name that's applied as a fallback is default.

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

No branches or pull requests

3 participants