1806964 - Restrict SVG `<use>` to prevent usage of `data:` URLs

Status: RESOLVED FIXED

(Core :: SVG, task)

Description

[Frederik Braun [:freddy]]

The original discussion was about removing data: support, based on these bits:

• There is some XSS risk with poor filters
• Webkit does not support data: URLs in <use> and Blink intends to remove support
• Chrome usage data suggests this is used in about 0.0056% of page load in Chrome.

The spec also says that this is only intended to work for same-origin URLs and opens a door to potential future use of cross-origin resources.
However, the discussion above seems to close that discussion for good and fully restrict to same-origin only.

I therefore suggest, we align with other browsers and make it fully "same-origin only". This should also ensure we do not follow cross-origin redirects and such. We're working with the authors to build fully solid tests in wpt, which will help us achieve this in an interoperable fashion.

References:

• Standards position https://github.com/mozilla/standards-positions/issues/718
• SVG WG discussion https://github.com/w3c/svgwg/pull/901
• Upcoming web platform tests https://github.com/web-platform-tests/wpt/pull/37511

Comment 1

[Frederik Braun [:freddy]]

I noticed that Bug 1754522 has a patch which looks highly relevant to resolving this one here.

Comment 2

[Frederik Braun [:freddy]]

Google is doing to ship this November 2023. https://developer.chrome.com/blog/migrate-way-from-data-urls-in-svg-use/

Comment 3

[Daniel Holbert [:dholbert]]

Thanks, good to know.

I guess that leaves us on-our-own, since https://github.com/mozilla/standards-positions/issues/718#issuecomment-1403709611 notes that Safari already rejects (never allowed?) data URIs in <use>, apparently.

I did a quick grep for these in our codebase to look at impact on our tests, frontend code, etc. I only found a few, pretty trivial to fix up/remove:

• One reftest testing that the data URIs do work in use:
  https://searchfox.org/mozilla-central/rev/87ba454e5c68ff77dff9acb9d7b0b51d6df12d11/layout/reftests/svg/use-extref-dataURI-01.svg#11

• One WPT test (which we currently fail) testing the opposite, i.e. asserting that data URIs do not work:
  https://searchfox.org/mozilla-central/rev/87ba454e5c68ff77dff9acb9d7b0b51d6df12d11/testing/web-platform/tests/svg/struct/reftests/use-data-url.tentative.svg#6

• Two crashtests with <use> pointing at a data URI:
  https://searchfox.org/mozilla-central/rev/87ba454e5c68ff77dff9acb9d7b0b51d6df12d11/layout/svg/crashtests/732836-1.svg#4
  https://searchfox.org/mozilla-central/rev/87ba454e5c68ff77dff9acb9d7b0b51d6df12d11/dom/base/crashtests/658845-1.svg#2

Comment 4

[Tom Schuster]

The Sanitizer API might end up depending on this being implemented.

Comment 5

[Tom Schuster]

Attachment: Bug 1806964 - Restrict SVG <use> to prevent usage of data: URLs. r?longsonr!

Comment 6

[Robert Longson [:longsonr]]

Should probably be behind a pref so that enterprise sites can enable it if they wish to do so.

Comment 7

[Daniel Holbert [:dholbert]]

Agreed.

We may also want to enable that pref for the benefit of the reftest that I referenced in comment 3 (which will keep that test passing, and which will help validate that we don't inadvertently break that configuration without noticing).

And we should also toggle the pref for the crashtests that I linked at the end of comment 3, too.

Comment 8

[Robert Longson [:longsonr]]

What we also need is a testcase along these lines...

1. a <use> element that points to a red <rect> (or some other valid content)

2. we call setAttribute to set that use element's href to a data URI

two thing need to happen

1. we don't render the data URI

2. we don't keep rendering the rect either.

I hope calling Unlink() would be enough to fix that but we should check that it does.

Comment 9

[Tom Schuster]

Thank you both. I think I incorporated your suggestions in my updated patch.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f0a7131d0fd
Restrict SVG <use> to prevent usage of data: URLs. r=longsonr

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/43341 for changes under testing/web-platform/tests

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/3f0a7131d0fd

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 14

[Cristian Tuns]

Backed out for causing bug 1866911 (and duplicates)

• Backout link

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/43409 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 17

[Robert Longson [:longsonr]]

Fortunately I think you can reland this pretty much as originally written. The actual fix to the regression was elsewhere in SVGUseElement

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efcdf995dbc7
Restrict SVG <use> to prevent usage of data: URLs. r=longsonr

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/43473 for changes under testing/web-platform/tests

Comment 20

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/efcdf995dbc7

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 22

[Brian Thomas Smith [bsmth]]

Documentation updates for this can be tracked in the following GitHub issue: https://github.com/mdn/content/issues/31111