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

Handling of dependent types #6

Closed
xtofian opened this issue Sep 23, 2017 · 13 comments
Closed

Handling of dependent types #6

xtofian opened this issue Sep 23, 2017 · 13 comments

Comments

@xtofian
Copy link

xtofian commented Sep 23, 2017

There are attributes where the required security type contract of the assigned value depends on the value of another attribute. Notably, the <link> element's href attribute requires a type that represents a URL that references a trustworthy resource (TrustedResourceUrl in Closure; similar to TrustedScriptURL in this proposal), if the element's rel attribute has a value of import or stylesheet. For other values of rel (e.g., author) the href attribute is interpreted as a plain hyperlink, and hence a weaker contract suffices ("following the link / navigating to the URL does not result in any undesirable side effects such as script execution").

This is a rather unfortunate design but we're presumably stuck with it.

In particular, we need to account for the possibility that the attribute that the type depends on (rel) is changed from a value that requires the weaker contract to one that requires the stronger contract after the dependently-typed attribute (href) itself.

In Closure, this is accounted for by a typed wrapper (goog.dom.safe.setLinkHrefAndRel) that sets both attributes at the same time, and dynamically enforces the appropriate type contract, combined with disallowing direct assignment to either attribute (rel or href) in application source.

In a native implementation of typed setters such a combined setter is likely undesirable. However, the issue could be addressed by changing the behavior of the rel attribute setter to clear the href attribute's value on assignment to the rel attribute (or perhaps preferably, to throw an exception if rel is assigned when href already has a non-empty value). The setter for the href attribute can then dynamically enforce the appropriate type contract depending on the rel attribute's actual value.

@xtofian
Copy link
Author

xtofian commented Sep 23, 2017

Another example is the srcdoc attribute of <iframe> elements: If the element's sandbox attribute contains allow-same-origin, then the value assigned to srcdoc must be HTML markup that's safe to evaluate in the same origin. Otherwise, a weaker contract is sufficient.

@xtofian
Copy link
Author

xtofian commented Jun 12, 2018

Another variant: The contents of <script> are treated as JavaScript if the type attribute is "module" or a JS MIME type, or omitted. If it's some other value, the contents are an uninterpreted data block with application-defined semantics.

Somewhat aside, it's not obvious what type should be required for the contents (or URL) of a <script> element with type="application/something". Perhaps it's best to disallow such an assignment altogether. It would seem there are many better alternatives (e.g. CORS).

@koto
Copy link
Member

koto commented Jun 15, 2018

We might be able to ignore <link rel=import> in TT. HTML imports are definitely going away. There are discussions on how to make a module system for HTML (WICG/webcomponents#645) and the currently popular solution seems to leverage ES6 modules, and to be able to import HTML from Javascript code (e.g. like this).

What would be problematic is that we couldn't require TT for the HTML module import path:

import doc from './my-html-module.html' as HTMLModule;

We can't require TrustedScriptURL for my-html-module, as all modules are loaded before JS executes. That said, it should not introduce problems, as the module name can not be a variable (it's a compile-time-constant equivalent). The browser might make an informed decision and enforce the policies for the imported module.

A competing solution of including HTML from HTML markup seems like it doesn't get enough traction, although there's still possibility this will be possible via another element.

@xtofian
Copy link
Author

xtofian commented Jun 19, 2018

Re: HTML module import:

It seems fine to not require TrustedScriptURL for the module import path, since it's entirely determined at load time, i.e. should not be subject to injection vulnerabilities. For applications that are deployed via some form of build-time assembly, security reviewers can check at build time that there are, for instance, no imports from third-party sources.

There is https://github.com/tc39/proposal-dynamic-import to consider. import(path) would require TrustedScriptURL for path?

@koto
Copy link
Member

koto commented Jun 20, 2018

For vanilla module import paths we don't even have a way of using TTs (they cannot be expressions). For dynamic imports we should require TTs, but that's probably difficult to have, as it requires some changes from TC39, or some support in the spec for the runtime to enforce additional rules on the import path (it's probably required, as the runtime has to resolve the paths somehow - for browser they are URLs, for node - some magic identifiers).

@koto
Copy link
Member

koto commented Jun 20, 2018

It seems like dynamic imports are already implemented in Safari & Chrome. From the polyfill, there's nothing we can do (we can't hook into import), and it seems like they can only be guarded from CSP script-src w3c/webappsec-csp#243.

Given that this is a string->code vector (while non-DOM), it's not clear to me whether we should make it in scope for TT enforcement, or rather leave it to base CSP script-src. What makes it problematic is that dynamic import is inherently "violating" nonced CSP (it's an implicit strict-dynamic equivalent), so any nonce-based CSP not has to rely on whitelists again, with all their problems.

cc @arturjanc

@koto
Copy link
Member

koto commented Jun 20, 2018

It turns out actually, CSP script-src doesn't apply at all to dynamic imports :(

@xtofian
Copy link
Author

xtofian commented Jun 20, 2018 via email

@koto
Copy link
Member

koto commented May 1, 2019

We departed a little bit from the original issue (into module loading). IIUC, the only elements right now that have different contracts based on other attribute values are:

  • link.href (with rel as a distinguisher)
  • iframe.srcdoc (with sandbox as a distinguisher)
  • script.src (with type as a distinguisher)
  • script.text (with type as a distinguisher).

The most strict (for DOM XSS) link.href contract is when rel=import, however this is deprecated and definitely going away (removal for Chrome planned for June 2019). All other values seem to match TrustedURL.

For others, I think we might simply choose the most restrictive contract, and as such TrustedHTML for iframe.srcdoc, TrustedScriptURL for script.src and TrustedScript for script.text (especially given that new powerful features like import maps tend to be controlled from non-JS script nodes).

This approach is simple, and fails close. You have to create a type is a value can be used in a risky way, not when we are sure it will be used as such. In general this is always the case with TT - it's possible that the typed object is not added to the DOM at all, or is added to an inert DOM etc.

@xtofian
Copy link
Author

xtofian commented May 18, 2019

One potential downside with this approach is that it requires application code to assign a stronger contract, which confers a higher level of trust, to values that don't actually merit that contract.  This makes it imperative that the surrounding code ensures that the resulting TrustedTyped is in fact only assigned to the sink in that restricted context, and does not "escape" into the rest of the program, where it could be assigned to a sink with the assumption that it's actually safe in the sinks context (which it isn't). 

For instance, to assign an arbitrary string to iframe.srcdoc of a sandboxed iframe, it has to be converted to TrustedHTML without sanitization or escaping.  One would have to be very careful that the resulting TrustedHTML doesn't "escape" and get assigned to another element's innerHTML.

This was the main reasoning behind the Google/Closure APIs that set, e.g., both the rel and the href attribute at the same time, which dodges this issue.

@koto
Copy link
Member

koto commented May 27, 2019

A prospective solution to that might be what @briansmith proposed in #152 (comment), which is to outline the allowed sinks when producing the types. This allows one to create subtypes, only valid for certain sinks, but on the other hand creates type instances that are not interchangeable, which might lead to problems e.g. with bugs surfacing only at runtime. I think we should strive for a system that lets the runtime bugs be discoverable as early as possible, and that leverage the type system to allow for static analysis and refactorings.

Brainstorming a bit: perhaps instead we could guard it at runtime in a different fashion? Each type object carries the name of the policy that created it (in polyfill, Chrome doesn't implement that yet). Perhaps authors might be able to configure some sinks to only accept objects from given policies? Statically, in a header, or dynamically via registering a callback that will always vet a value based on its policy name? My rough preference would be to leverage existing mechanism like changing the property setters, but I'm not sure if they would not get the already stringified value now. However, it does start to look like the complexity of such solution is too big, for a little gain.

One extra related type interdependency is on (iframe.src + iframe.name) and (a.href + a.target). Right now the issue does not exist, as iframe.src and a.href require the same type, but if this changed, this would be a bypass, or a backwards incompatibility.

@koto
Copy link
Member

koto commented May 27, 2019

I've confirmed that the property setters do not get the stringified arguments, so it seems adding additional restrictions to the sinks could work like that:

https://jsbin.com/gefoxaquda/edit

@koto
Copy link
Member

koto commented Aug 8, 2019

Closing; The impact of the issue is quite narrow now, post- HTML import and TrustedURL deprecation (#204). I think only the sandbox iframe srcdoc would be the potentially affected sink, and it seems easy to workaround in JavaScript - either by writing explicit, or implicit sink wrappers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants