search

w3c / svgwg

SVG Working Group specifications
Other
695 stars 131 forks source link

Remove support for data: URL in SVGUseElement #901

Closed shhnjk closed 1 year ago

shhnjk commented 1 year ago

Motivation

Assigning an attacker controlled string to SVGUseElement.href causes XSS due to data: URLs. This also led to a bypass of Trusted Types in Blink.

Additionally, data: URLs can only trigger script execution in script loaders such as HTMLScriptElement.src or dynamic import. However, SVGUseElement is an exception to this, which also caused a bypass in the Sanitizer API. We believe that this also led to several other bugs in sanitizers and linters missing a check for this special case.

Since Webkit does not support data: URLs in SVGUseElement, Blink is planning to remove support for it. And therefore, we'd like to update the spec.

We have also requested Mozilla's position on this.

Currently, the usage of data: URLs in SVGUseElement is about 0.0056% of page load in Chrome.

w3cbot commented 1 year ago

shhnjk marked as non substantive for IPR from ash-nazg.

shhnjk commented 1 year ago

@caribouW3, could you PTAL? Thanks!

caribouW3 commented 1 year ago

Hi, At a first glance, the proposal to forbid all data: url altogether seems a bit extreme. The current editors draft already clearly says that user agent may take that kind of restriction. If there's a global consensus that all user agent do the same thing, then it makes sense to reflect it in the specification.

shhnjk commented 1 year ago

Thanks! We will wait for Mozilla's response on this then 🙂

mozfreddyb commented 1 year ago

@shhnjk is there a WPT patch accompanying this standards change? It would be ideal if we had cases for cross/same-origin, -site, and same-document URLs too.

shhnjk commented 1 year ago

@shhnjk is there a WPT patch accompanying this standards change? It would be ideal if we had cases for cross/same-origin, -site, and same-document URLs too.

WIP WPT at https://github.com/web-platform-tests/wpt/pull/37511 🙂

shhnjk commented 1 year ago

I think we should essentially require a same origin URL here (and also enforce that for redirects, which would be easy if this was defined in terms of fetch...).

And no longer have "user agents may" or "a future version of". Let's actually settle this.

Thanks! Since https://github.com/w3c/svgwg/issues/707 got closed, I've removed the a future version of bit. However, I think User agents may restrict external resource documents for security reasons bit is still necessary. For example, Blink enforced restrictions on external resource documents such as:

  1. <foreignObject> is not supported.
  2. Event handlers doesn't fire.

@caribouW3, I've got a positive signal from Mozilla and Webkit is likely supportive (pending formal decision until January 6th). Is there anything else I need consensus on to land this change?

SebastianZ commented 1 year ago

Just to provide a use case for cross-origin requests: I recently had the need to re-use an external SVG multiple times within a document and style them individually.

This only works via <use>. Though all images on that site are normally coming from a CDN. And because cross-origin requests are currently not allowed, I had to deliver them via the same domain instead.

If SVG obeyed to CORS, this wouldn't be an issue.

And there could also be some CORS header that controls whether data: URLs are allowed in SVG. So, restricting them by default, yes, but authors would still have a way to opt-in to support them.

Sebastian

shhnjk commented 1 year ago

@SebastianZ, that was discussed in https://github.com/w3c/svgwg/issues/707#issuecomment-1356259989, and there are other alternatives to do this such as CSS Linked Parameters. And you can also fetch cross-origin SVG and get response as Blob, and then create a Blob URL to use it as a resource in SVGUseElement too.

@caribouW3, friendly ping for https://github.com/w3c/svgwg/pull/901#issuecomment-1358195469 🙂

caribouW3 commented 1 year ago

Since it will be supported in webkit, let's merge this. @shhnjk, you can then merge the corresponding wpt PR.

shhnjk commented 1 year ago

Since it will be supported in webkit, let's merge this. @shhnjk, you can then merge the corresponding wpt PR.

Thank you! The WPT PR should be merged shortly (by the bot)!

SebastianZ commented 1 year ago

@SebastianZ, that was discussed in #707 (comment), and there are other alternatives to do this such as CSS Linked Parameters.

Yes, CSS Linked Parameters will eventually solve the use case of applying different styles to a single external SVG resource. And thanks for pointing me to the other issue! (I previously already commented there, as well, but totally forgot about it.)

Sebastian