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

Remove support for data: URL in SVGUseElement #901

Merged
merged 2 commits into from
Jan 10, 2023
Merged

Remove support for data: URL in SVGUseElement #901

merged 2 commits into from
Jan 10, 2023

Conversation

shhnjk
Copy link
Member

@shhnjk shhnjk commented Dec 9, 2022

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
Copy link

w3cbot commented Dec 9, 2022

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

@shhnjk
Copy link
Member Author

shhnjk commented Dec 9, 2022

@caribouW3, could you PTAL? Thanks!

@caribouW3
Copy link
Member

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
Copy link
Member Author

shhnjk commented Dec 12, 2022

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

@mozfreddyb
Copy link

@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.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 14, 2022
This CL adds WPT for URLs supported in SVGUseElement.
See: w3c/svgwg#901

But: 1300195
Change-Id: Ieab699fc167b254c65b92ed811756404dcc7c575
@shhnjk
Copy link
Member Author

shhnjk commented Dec 14, 2022

@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 web-platform-tests/wpt#37511 🙂

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 14, 2022
This CL adds WPT for URLs supported in SVGUseElement.
See: w3c/svgwg#901

But: 1300195
Change-Id: Ieab699fc167b254c65b92ed811756404dcc7c575
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 16, 2022
This CL adds WPT for URLs supported in SVGUseElement.
See: w3c/svgwg#901

But: 1300195
Change-Id: Ieab699fc167b254c65b92ed811756404dcc7c575
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 16, 2022
This CL adds WPT for URLs supported in SVGUseElement.
See: w3c/svgwg#901

But: 1300195
Change-Id: Ieab699fc167b254c65b92ed811756404dcc7c575
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 16, 2022
This CL adds WPT for URLs supported in SVGUseElement.
See: w3c/svgwg#901

But: 1300195
Change-Id: Ieab699fc167b254c65b92ed811756404dcc7c575
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 17, 2022
This CL adds WPT for URLs supported in SVGUseElement.
See: w3c/svgwg#901

But: 1300195
Change-Id: Ieab699fc167b254c65b92ed811756404dcc7c575
@shhnjk
Copy link
Member Author

shhnjk commented Dec 19, 2022

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 #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?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 19, 2022
This CL adds a runtime enabled feature and WPT for removal of data: URLs
in in SVGUseElement.
See: w3c/svgwg#901

But: 1300195
Change-Id: Ieab699fc167b254c65b92ed811756404dcc7c575
@SebastianZ
Copy link

SebastianZ commented Jan 8, 2023

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
Copy link
Member Author

shhnjk commented Jan 9, 2023

@SebastianZ, that was discussed in #707 (comment), 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 #901 (comment) 🙂

@caribouW3
Copy link
Member

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

@caribouW3 caribouW3 merged commit 4842530 into w3c:main Jan 10, 2023
@shhnjk
Copy link
Member Author

shhnjk commented Jan 10, 2023

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)!

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 10, 2023
This CL adds a runtime enabled feature and WPT for removal of data: URLs
in in SVGUseElement.
See: w3c/svgwg#901

But: 1300195
Change-Id: Ieab699fc167b254c65b92ed811756404dcc7c575
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4104249
Reviewed-by: Mike West <[email protected]>
Commit-Queue: Jun Kokatsu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1090945}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 10, 2023
This CL adds a runtime enabled feature and WPT for removal of data: URLs
in in SVGUseElement.
See: w3c/svgwg#901

But: 1300195
Change-Id: Ieab699fc167b254c65b92ed811756404dcc7c575
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4104249
Reviewed-by: Mike West <[email protected]>
Commit-Queue: Jun Kokatsu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1090945}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 10, 2023
…UseElement"

This reverts commit 23fdc0eb00c8597c2c74faee91b4a6a4e2296f3c.

Reason for revert: Not landing immediately, I want to do a manual try on GPU bots to confirm if this is the cause of an SVG test failure. See crbug.com/1406342

Original change's description:
> Add a runtime enabled feature for removal of data: URL in SVGUseElement
>
> This CL adds a runtime enabled feature and WPT for removal of data: URLs
> in in SVGUseElement.
> See: w3c/svgwg#901
>
> But: 1300195
> Change-Id: Ieab699fc167b254c65b92ed811756404dcc7c575
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4104249
> Reviewed-by: Mike West <[email protected]>
> Commit-Queue: Jun Kokatsu <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1090945}

Change-Id: I09bed766cad4afcf99e526af82d0736c39190de9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 19, 2023
…l of data: URL in SVGUseElement, a=testonly

Automatic update from web-platform-tests
Add a runtime enabled feature for removal of data: URL in SVGUseElement

This CL adds a runtime enabled feature and WPT for removal of data: URLs
in in SVGUseElement.
See: w3c/svgwg#901

But: 1300195
Change-Id: Ieab699fc167b254c65b92ed811756404dcc7c575
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4104249
Reviewed-by: Mike West <[email protected]>
Commit-Queue: Jun Kokatsu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1090945}

--

wpt-commits: fe30ab22c3088be330f87cb77ce5d584e6823e21
wpt-pr: 37511
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Jan 20, 2023
…l of data: URL in SVGUseElement, a=testonly

Automatic update from web-platform-tests
Add a runtime enabled feature for removal of data: URL in SVGUseElement

This CL adds a runtime enabled feature and WPT for removal of data: URLs
in in SVGUseElement.
See: w3c/svgwg#901

But: 1300195
Change-Id: Ieab699fc167b254c65b92ed811756404dcc7c575
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4104249
Reviewed-by: Mike West <[email protected]>
Commit-Queue: Jun Kokatsu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1090945}

--

wpt-commits: fe30ab22c3088be330f87cb77ce5d584e6823e21
wpt-pr: 37511
@SebastianZ
Copy link

@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

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

Successfully merging this pull request may close these issues.

7 participants