Scrubber removes crucial SVG 1.1 elements

[stanhu]

The current sanitizer strips out crucial SVG 1.1 elements (e.g. style) and doesn't take into account the full mapping of allowed attributes and elements. I wrote a custom Loofah scrubber here that does this:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3401/diffs

Is there interest in making this part of the Loofah package?

[flavorjones]

Hi,

Thanks for opening this issue. I'm open to discussing this.

I need to better understand what you're suggesting. Can you please fully explain the document type (preferably with an example) and the attributes you're using? You haven't provided much information here, and the merge request you linked to is a bit hard to follow.

[stanhu]

Yes, take a look at this SVG file, as taken from this issue.

https://gitlab.com/gitlab-com/gitlab-artwork/raw/master/wordmark/stacked_wm.svg

If you run this file through Loofah, the resulting image is a big black blob, nothing like the original. Why? The default Loofah scrubber only has style whitelisted as an attribute, not an element, while the SVG 1.1 spec says that it can be a full-fledged element. There are numerous other examples that illustrate the same issue for different reasons.

Now we could simply just add all the SVG 1.1 elements into the whitelist and be done with it. But since the spec clearly specifies which attributes are allowed for which elements, I went a step further and made the Loofah scrubber obey those rules. That's what the aforementioned merge request does.

[stanhu]

@flavorjones, can you take a look here?

[Hampei]

I walked into this as well, but if you use inline svg, the style element is indeed a risk, since it can affect anything on the page.
We moved to using data-urls for svg and just added the style element to the list of safe elements.

Loofah::HTML5::WhiteList::SVG_ELEMENTS.add 'style'
Loofah::HTML5::WhiteList::ALLOWED_ELEMENTS.add 'style'
Loofah::HTML5::WhiteList::ALLOWED_ELEMENTS_WITH_LIBXML2.add 'style'

[flavorjones]

Noting here for posterity that this would be addressed by #155 if we used DOMPurify's allowlists.

[GeoffTidey]

Had a similar issue with a SVG that had a style including 'stroke-miterlimit'

Loofah::HTML5::SafeList::ALLOWED_SVG_PROPERTIES.add 'stroke-miterlimit'

has fixed it for me