[Feature/Bug] Improve <animate> sanitisation

SelfMadeSystem commented 1 year ago

This issue proposes a bug/feature which will improve <animate> sanitisation.

I am uncertain whether to classify this as a bug or a new feature, so I leave the option up to the maintainers.

Background & Context

The <animate> tag is used frequently in SVG animations. It can also animate certain attributes that CSS can't animate, such as offset on <stop> elements.

Proposition

I propose to instead limit what values the attributeName can have. For example, href or xlink:href make no sense to be animated. They should never be animated. However, other attributes such as d, width, fill, offset, or flood-color absolutely have the right to be animated.

I think this would be a much more sensible way of preventing XSS while keeping the features that make SVG great.

Examples

<svg>
  <a xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="?">
    <circle r="400"></circle>
    <animate attributeName="xlink:href" begin="0" from="javascript:alert(1)" to="&" />
  </a> 
</svg>

Should turn into

<svg>
  <a xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="?">
    <circle r="400"></circle>
    <animate attributeName="" begin="0" from="javascript:alert(1)" to="&" />
  </a> 
</svg>

or something similar idk

but...

<svg viewBox="0 0 10 10" width="8em" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
  <defs>
    <linearGradient id="myGradient" gradientTransform="rotate(90)">
      <stop offset="0" stop-color="gold">
        <animate attributeName="offset" values="0;1;0" dur="2s" repeatCount="indefinite" />
      </stop>
      <stop offset="1" stop-color="red" />
    </linearGradient>
  </defs>

  <!-- using my linear gradient -->
  <circle cx="5" cy="5" r="4" fill="url('#myGradient')" />
</svg>

should be left as-is

(note that the example above cannot be converted to a CSS animation as offset is not a CSS-animatable attribute)

Notes:

I am not a cyber-security analyst (however I hope to be), so please make sure my ideas make sense from a security standpoint.

Please give feedback.

I'll try to help with an implementation of something like this when I have time.

I also have similar issues with <use> and <foreignObject>, however I'll make new issues for those respectively when I can come up with good ideas. <use> especially is used (🙃) everywhere in SVG files.

cure53 commented 1 year ago

Heya, thanks for submitting this issue.

I am not sure if I agree that <animate> is a frequently used element in SVG and, given that it is deprecated in SVG 2.0, it will also likely not become more frequent in the future. The <animate> element is a weird leftover from VML when it got merged with PGML to create SVG, no I am hesitant to give it more spotlight by default :)

Together with the <set> element, it somewhat forms the group of fringe elements that, while being rarely used and clumsy in terms of implementation, also cause security risks by, as you mentioned, animating href and alike.

I would recommend to simply use a hook to add more fine-grained handling of those kinds of elements, given the dark past of XSS via SVG caused by <animate>, <set>, <use>, <foreignObject> and the likes will make it very unlikely for us to actually change core library behavior.

References: https://discuss.httparchive.org/t/use-of-html-elements/1438 https://svgwg.org/specs/animations/#AnimateElement

cure53 commented 1 year ago

Closing this for now as no core changes are anticipated and all requested logic can be implemented using a hook.

cure53 / DOMPurify