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

Consider dropping declarative scope for subresource loading. #580

Closed
koto opened this issue May 12, 2020 · 12 comments
Closed

Consider dropping declarative scope for subresource loading. #580

koto opened this issue May 12, 2020 · 12 comments
Labels
security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response.

Comments

@koto
Copy link

koto commented May 12, 2020

https://github.com/WICG/webpackage/blob/master/explainers/subresource-loading.md declares how, essentially, adding a link tag will change how, and if, some scripts on the page are being loaded.

Adding that primitive to the platform makes it harder to protect existing applications from XSS, as it bypasses existing methods that guard script loading (and likely server-side XSS mitigations like XSS filters are unaware of those new vectors as well).

For example, right now there's no requirement to have a CSP nonce in <link rel="webbundle">, and modifying the bundle scope in javascript does not require a Trusted Type, and yet adding that element may, IIUC, cause arbitrary scripts to get loaded, or prevent them from being loaded.

New mechanisms that alter script loads should be programmatic, and not declarative.

/ cc @arturjanc @mikewest @lweichselbaum

@jyasskin jyasskin added the security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response. label May 21, 2020
@jyasskin
Copy link
Member

I was thinking of a <link rel="webbundle"> as just providing a different way to retrieve a script, whose execution, XSS resistance, integrity, CSP, etc. are provided by the <script> tag that actually executes the script. Any script provided by a bundle would still need to be authoritative for the URL that loads it, or else the script would fall back to a network fetch. So, I'm struggling to think of how an attacker who can insert HTML tags would be able to use a new <link rel="webbundle"> to run script they couldn't run by inserting a `<script src="..."> tag.

There's a suggestion in https://github.com/WICG/webpackage/blob/master/explainers/subresource-loading.md#link-based-api that a <link rel="webbundle"> with an overbroad scope could prevent scripts from loading, and this may be a good argument that we should instead go to the network.

I would appreciate more opinions from security folks. :)

@koto
Copy link
Author

koto commented May 25, 2020

I was thinking of a <link rel="webbundle"> as just providing a different way to retrieve a script, whose execution, XSS resistance, integrity, CSP, etc. are provided by the <script> tag that actually executes the script. Any script provided by a bundle would still need to be authoritative for the URL that loads it, or else the script would fall back to a network fetch. So, I'm struggling to think of how an attacker who can insert HTML tags would be able to use a new <link rel="webbundle"> to run script they couldn't run by inserting a `<script src="..."> tag.

The problem here is that this introduces another declarative way of altering the script loads like <base> or import maps. Whoever can control these elements in the DOM gets to decide how and if scripts are loaded -- and in a way that's not obvious. This is an important capability, and I argue that we should not be creating it - especially if it doesn't require already controlling the JS code that executes.

It's not very clear that, for example this code:

const foo = document.createElement('link')
foo.rel = userparams.rel
foo.href = '//legitimate.example/' + userparams.bar;
document.head.appendChild(foo)

is a gadget that used to be OK, but now can alter how the scripts are loaded on the page. This is surprising to code reviewers, introduces new attack surface that existing security tools don't cover -- and might lead to security bugs.

It's not expected for link elements to interfere with script loads. The last remaining, bad examples of this pattern were link rel=serviceworker and <link rel="import">; both deprecated (after having led to numerous XSSes, and filter bypasses - even in Chrome). I argue we should not add novel ways of how scripts are loaded (directly, or indirectly) and instead rely on primitives that already exist and security consequences of which are well understood. Even adding <base> tag, has led to numerous bugs, HTML sanitizer bypasses etc.

On the other hand, import maps use a <script> element, because one would expect that control over <script> nodes gives total control over the application. Perhaps a <script> based API should be used instead of <link>.

An even better solution would be to require Javascript execution to alter script loading via bundles - either partially, for example like outlined in https://github.com/WICG/webpackage/blob/master/explainers/subresource-loading.md#no-declarative-scope, or fully, by only allowing to fetch a webbundle from Javascript. That gives guarantee that a limited HTML injection (e.g. if the attacker can only insert certain tags, as others are filtered out) cannot lead to a security issue via webbundles.

There's a suggestion in https://github.com/WICG/webpackage/blob/master/explainers/subresource-loading.md#link-based-api that a <link rel="webbundle"> with an overbroad scope could prevent scripts from loading, and this may be a good argument that we should instead go to the network.

That is one way of how the code that is executed in the document is altered by web bundles - this one without a need to access to any signing keys. Being able to disable selective script loads is problematic, as some scripts might perform additional security checks, workaround bugs, polyfill Web APIs etc. - and in general it gives the attacker another lever to trigger xs-leaks.

@jeffkaufman
Copy link

Thinking about how bundles could be used, I think it's possible that they don't need to be loadable declaratively. In any case where you might want to reference a web bundle in the HTML which would contain a resource which you are authoritative for, you could instead serve a web bundle as the top level page. So perhaps we would not be losing any important functionality if we made these be only loadable through JavaScript?

@jyasskin
Copy link
Member

It looks like adding a <link rel=preload> element with an incorrect integrity attribute can also prevent script from loading, so folks do have to be at least a bit careful with gadgets that let folks add <link> elements.

However, I'm gradually getting more worried about sites that:

  1. Let users upload content to an origin.
  2. Serve actual scripts from the same path or a subpath of the user-provided content. (Ew, but might not already be exploitable?)
  3. Add the application/web-bundle mime type.
  4. Have the gadget that lets an attacker add that arbitrary <link>.

No matter whether the <link> can define its scope, there's at least a race condition that would let the attacker replace script. I'd been planning to restrict the scope to the bundle's path or a subpath, and require a header like Service-Worker-Allowed to loosen that restriction, but because <link> headers are currently safer to inject than script, I'm now thinking we may want to require a response header to make the response authoritative at all.

@koto
Copy link
Author

koto commented Aug 25, 2020

Don't these issue go away if there's no <link rel="webbundle">?

@jyasskin
Copy link
Member

jyasskin commented Aug 27, 2020

They would, but we'd wind up with a preloading interface that's very different from what HTML has used in the past. I'll ask the webperf mailing list for opinions.

@terjanq
Copy link

terjanq commented Dec 8, 2020

Bumping this up, did we achieve any consensus on this? @koto @jyasskin

webbundle seems indeed very similar to rel=import, although it also seems much more restrictive than the import. This could potentially cause breakages of the CSP, which might need some compromises (loopholes) to be able to load the bundle in the first place.

But strictly from the security perspective, we've also had (still have actually) a similar mechanism app cache manifest, which had a similar permission system and still was widely abused (https://www.slideshare.net/fransrosen/attacking-modern-web-technologies).

I am more leaning towards @koto statement that this is just too risky, even with these strict conditions because it was proven in the past that these restrictions can often be fulfilled.

  1. Let users upload content to an origin.
  2. Serve actual scripts from the same path or a subpath of the user-provided content. (Ew, but might not already be exploitable?)
  3. Add the application/web-bundle mime type.
  4. Have the gadget that lets an attacker add that arbitrary <link>.

@terjanq
Copy link

terjanq commented Dec 8, 2020

I was thinking more about the issue and I thought about a potential solution. <script> tags by default already implement type attribute and we could potentially require every script to have type="bundle" to be able to access the resource from the bundle. That would require changing both <link> and <script> in that case. Not sure what to do with <link rel=stylesheet href=xxx> since these should probably be also protected against being controlled by an attacker.

What do you think? Are <script> and <link> tags the only concern for bypassing the HTML sanitizers?

@littledan
Copy link
Contributor

I'm a little worried about disabling header-based resource hints when it comes to bundle-based subresource loading. Wouldn't it be most useful if we had a serialization which worked for both HTML and HTTP headers, so that the fetch to the bundle could run ASAP? We could do this by using Link: headers but disabling the <link> tag and using a separate <script> tag format for that case, for example.

@jyasskin
Copy link
Member

jyasskin commented Dec 8, 2020

We haven't found consensus on this yet. I believe the path-based restrictions alluded to here and here convinced @koto that this wasn't a blocking issue, so the plan was to do an origin trial with <link> tags and re-evaluate from there.

The webperf list was friendly to using <script> or <base> tags for declarative bundle declarations inside the document, which seems fine to me. The list didn't discuss how to do the same in response headers, but Link seems fine there?

<link rel=stylesheet href=url_inside_bundle> seems like not a new risk if the site already allows the attacker to inject arbitrary <link> tags. 😉 But if we attach bundles using <script type=a_bundle_is_declared_here> or similar, this is protected too.

@terjanq
Copy link

terjanq commented Dec 8, 2020

<link rel=stylesheet href=url_inside_bundle> seems like not a new risk if the site already allows the attacker to inject arbitrary <link> tags. 😉 But if we attach bundles using <script type=a_bundle_is_declared_here> or similar, this is protected too.

I know sanitizers that allow <link> elements because these are usually not harfmul, e.g. <link rel="icon"> is commonly allowed. They usually block stylesheet, import, etc. but rest seems to be allowed. So I would argue that this is something to solve.

@hayatoito
Copy link
Collaborator

hayatoito commented Sep 16, 2021

We are switching to <script>-based API. See issue #670 for the progress.
Probably we can close this issue. Let me close this.

Although the subject of this issue is "Consider dropping declarative scope for subresource loading" and <script>-based API is still declarative one, it seems we can close this issue with <script>-based API, given the discussion here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response.
Projects
None yet
Development

No branches or pull requests

6 participants