1770888 - (CVE-2022-34473) Sanitizer fragment-only check for <svg:use href=...> bypassed by xlink:href

Status: VERIFIED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Armin Ebert [:arminius]]

The Sanitizer API restricts the value of an SVG <use href="..."> attribute to fragment URLs by default, so that nodes can't be imported from different documents.

However this check doesn't currently consider xlink:href, although that's a valid fallback used by SVGUseElement::Href().

Testcase

new Sanitizer().sanitize(new DOMParser().parseFromString(`
<svg>
    <use href="#foo"/>
    <use href="http://example/#bar"/>
    <use xlink:href="http://example/#baz"/>
</svg>
`, 'text/html')).firstChild.outerHTML

Expected result:

<svg>
    <use href="#foo"></use>
    <use></use>
    <use></use>
</svg>

Actual result:

<svg>
    <use href="#foo"></use>
    <use></use>
    <use xlink:href="http://example/#baz"></use>
</svg>

AFAIU, this happens because the fragment-only check in nsTreeSanitizer::SanitizeAttributes() is only done for attributes without a set namespace. So a patch might be just something like:

diff --git a/dom/base/nsTreeSanitizer.cpp b/dom/base/nsTreeSanitizer.cpp
--- a/dom/base/nsTreeSanitizer.cpp
+++ b/dom/base/nsTreeSanitizer.cpp
@@ -1262,7 +1262,8 @@ void nsTreeSanitizer::SanitizeAttributes
       // else not allowed
     } else if (aAllowed.mXLink && kNameSpaceID_XLink == attrNs) {
       if (nsGkAtoms::href == attrLocal) {
-        if (SanitizeURL(aElement, attrNs, attrLocal)) {
+        bool fragmentOnly = aElement->IsSVGElement(nsGkAtoms::use);
+        if (SanitizeURL(aElement, attrNs, attrLocal, fragmentOnly)) {
           // in case the attribute removal shuffled the attribute order, start
           // the loop again.
           --ac;

The fragment-only check was introduced in https://hg.mozilla.org/mozilla-central/rev/37819efa513e53663bad1ebbb47035a8eff16e24 which references bug 1757210, so I'm linking to that one (it's view-restricted, so just guessing).

NB: This has probably already been discussed, but I was just wondering if, given that xlink: attributes are considered deprecated in the docs, the sanitizer could just convert xlink:href into a native href, and drop support for XLink otherwise, so that it doesn't need to be handled as a special case at every turn.

Comment 1

[Armin Ebert [:arminius]]

(Turns out I can't set a "See Also:" to an access-restricted bug ID.)

Comment 2

[Frederik Braun [:freddy]]

That was dumb. Thanks for reporting. I'll have a patch soon.

Comment 3

[Frederik Braun [:freddy]]

Thanks for the patch suggestion, this is spot on!

NB: This has probably already been discussed, but I was just wondering if, given that xlink: attributes are considered deprecated in the docs, the sanitizer could just convert xlink:href into a native href, and drop support for XLink otherwise, so that it doesn't need to be handled as a special case at every turn.

Indeed, this discussion was part of bug 1757210. Originally, I wanted to disallow all of <use> , but we need that in our internal usage for the Firefox frontend, as it is being used for theming. For the specific use of the web-exposed Sanitizer API, I can imagine removing all of svg's <use> and maybe even all of xlink.

Comment 4

[Frederik Braun [:freddy]]

Attachment: Bug 1770888 - r?hsivonen

Comment 5

[Frederik Braun [:freddy]]

Attachment: Bug 1770888 - tests r?hsivonen

Depends on D145081

Comment 6

[Frederik Braun [:freddy]]

Copying sec-rating from bug 1757210.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

r=hsivonen
https://hg.mozilla.org/integration/autoland/rev/3a8b1d90a8be817ec4e6914de60c6844c69f76dc
https://hg.mozilla.org/mozilla-central/rev/3a8b1d90a8be

Comment 8

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 9

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Reproduced the issue on Firefox 102.0a1 (2022-05-23) under macOS 12.4 by using the TC from Comment 0 and some help from Freddy.

The issue is fixed on Firefox 103.0a1 (2022-06-23) and Firefox 102.0. Tests were performed on macOS 12.4, Ubuntu 22.04 and Windows 10.

Comment 10

[Frederik Braun [:freddy]]

Attachment: Bug 1770888 - tests r?hsivonen

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

tests r=hsivonen
https://hg.mozilla.org/integration/autoland/rev/d82e1c65b1dd69d5d0aa8cefe1094b1ad859f7bf
https://hg.mozilla.org/mozilla-central/rev/d82e1c65b1dd

fix prettier lint failure on a CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/eab8134f7f1a9cf6e9c976706e1a2a0a5ff8eb6a
https://hg.mozilla.org/mozilla-central/rev/eab8134f7f1a