New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Fixed
Closed: Jan 2017
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

issue 680418

Sign in to add a comment

Issue 679291: SVG animations on non-animatable elements.

Reported by, Jan 9 2017 Project Member

Issue description

Clever folks have noted that it's possible to change the `href` attribute of a `<script>` element via things like `<set>` and `<animate>`.

This is somewhat useful for XSS attacks: injecting something like `<svg><set href=#x attributeName=href to=// />` or `<svg><animate href=#x attributeName=href to=// />` gets you code execution on the site.

The PoCs don't work in Firefox, and based on a quick skim of what I think is the relevant spec (, this shouldn't be possible. Am I reading that right? 

I've CC'd a few folks who know more about SVG than I do. Hopefully one of y'all can triage this to the right person. (fs@, I'm assigning it to you, since you're the last one to explicitly touch animation... thanks!)

I'm not marking this as RestrictView, as the discussion of the bug is already public.

Comment 1 by, Jan 9 2017

The list in the link there applies specifically to the <animateMotion> element, a more pertinent spec link might be (the "Animatable: no" bit mostly.) I think we end up falling into this hole because of how 'href' on SVGScriptElement is actually an SVGAnimatedString (because it implements SVGURIReference. SVG 1.1 had the same setup, so this is nothing new.) So we have an SVGAnimatedString that can never be animated! href.baseValilicious!

Comment 2 by, Jan 9 2017

Ok. Hrm. Can we subclass `SVGURIReference` into `SVGURIReferenceThatCanNotBeAnimatedBecauseWhoWouldEverAnimateHrefAttributesThatsWeird`? :)

Comment 3 by, Jan 9 2017

Project Member
Status: Assigned (was: Available)

Comment 4 by, Jan 9 2017

I think an easier path of approach could be to remove this 'href' (and hence also 'xlink:href') from the property-map - that way the animation system would not see it as animatable.

Comment 5 by, Jan 9 2017

Can you walk me through doing that? I have zero idea how SVG works. :)

Comment 6 by, Jan 9 2017

Sure, just let me upload the CL first ;-)

Comment 8 by, Jan 11 2017

Status: Fixed (was: Assigned)
If we want to merge this (to 56 primarily), I don't think it'll merge cleanly, but rather needs some tending too. Hopefully not too complicated though.

Comment 9 by, Jan 12 2017

Blocking: 680418

Comment 10 by, Jan 12 2017

Labels: -Type-Bug-Security Type-Bug

Comment 11 by, Jan 12 2017

fs@: I don't think we _need_ to merge it to 56; it's not at all a new bug. If you have free time you'd like to fill, however, please go right ahead. :)

Comment 12 by, Jan 12 2017

Well, if I did think it'd merge cleanly I might've tried already, but since it likely won't, I'll spend those minutes somewhere else until otherwise prompted... =)

Sign in to add a comment