fix($sanitize): don't allow svg animation tags

[rodyhaddad]

After #11124 got merged, a security vulnerability got introduced.
Animation in SVG became tolerated by the sanitizer.

Exploit Example:

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

Here we are animating an anchor's href, starting from a value that's a javascript URI,
allowing the executing of arbitrary javascript in the process.

Preventing only the animation of links is tricky, as SVG is weird and namespaces aren't predictable.
We've decided to have the sanitizer filter out SVG animation tags instead.

Considering the sanitizer is commonly used to sanitize untrusted HTML code, this shouldn't affect
many apps in the wild. Also, no release has been with #11124 in it, but not this fix.

[rodyhaddad]

/cc @sirdarckcat

[IgorMinar]

can you add a comment that the animation related elements are intentionally omitted?

[rodyhaddad]

Done

[IgorMinar]

also the fix commit contains a breaking change that should be documented per our conventions.

otherwise this looks good for master branch.

for 1.3 we should just revert the other fix that adds case-sensitivity support.

[petebacondarwin]

Do we need to block <animate> element altogether. I thought we were able to simply block attributeName and attributeType attributes (case-sensitive)?

If we did this, then we would not have a breaking change, since previously we were lowercasing those attributes, which was effectively blocking them anyway...

[lgalfaso]

+1 to @petebacondarwin solution

[rodyhaddad]

From my research the only way to animate something is using the attributeName attribute.

So blocking [attributeName] or blocking <animate /> has the same effect: preventing animation functionality from passing through the sanitizer.

I personally believe blocking the element is better than blocking the attribute. It's easier to "debug" for a developer imo.

---

I didn't mark it as a breaking change because animation were never possible in SVG before #11124, which has yet to be released in a version.

Adding the breaking change will just make it appear in the ChangeLog and our migration guide, and it will be "useless" because there is no prior angular version that allows SVG animations.

[rodyhaddad]

For now I've removed the following elements:
animate, animateColor, animateMotion, animateTransform, set

And the following attributes, because they're useless with the above tags removed:
attributeName, attributeType

[cure53]

I agree, <set> and <animate> should be blocked completely. Nothing good comes out of those two elements and according to our research, usage across e.g. Wikipedia SVGs is very very low.

Sources:

• https://www.ei.rub.de/media/hgi/veroeffentlichungen/2011/10/19/svgSecurity-ccs11.pdf
• http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.469.7647&rep=rep1&type=pdf

[petebacondarwin]

LGTM

[petebacondarwin]

I landed this on master and added the commit with the warning messages to 1.3.x. I didn't land the security fix since we never seemed to have landed the original mixed case fix in the first place there.

[cure53]

Just one small note: We have not yet managed to identify any attack surface coming from animateColor, animateMotion and animateTransform. They don't permit declarative changes on arbitrary attributes but have their own, specific and limited space for changes.

Unless (of course) you have attack examples that also work in these, I would go and say they are safe. Well, two of them. Note that animateColor (that would have had exploitation potential) is flagged deprecated and not supported by FF, IE or Chrome (or the respected browser engines).

[petebacondarwin]

To stay on the safe side I would prefer to wait for people to request that these elements are white listed and look at this again, then.

[cure53]

@petebacondarwin Agreed, makes perfect sense!