Sanitize SVG animation attributes which could set JavaScript URL values.

Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp

Index: Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp
diff --git a/Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp b/Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..f3ad56aa57f37c8e3d5185673803f087af744584
--- /dev/null
+++ b/Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp
@@ -0,0 +1,257 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "config.h"
+// FIXME(dominicc): Poor confused check-webkit-style demands Attribute.h here.
+#include "core/dom/Attribute.h"
+
+#include "core/HTMLNames.h"
+#include "core/SVGNames.h"
+#include "core/XLinkNames.h"
+#include "core/clipboard/Pasteboard.h"
+#include "core/dom/QualifiedName.h"
+#include "core/editing/Editor.h"
+#include "core/editing/SelectionType.h"
+#include "core/editing/VisibleSelection.h"
+#include "core/html/HTMLElement.h"
+#include "core/svg/SVGAElement.h"
+#include "core/svg/SVGAnimateElement.h"
+#include "core/svg/SVGDiscardElement.h"
+#include "core/svg/SVGSetElement.h"
+#include "core/svg/animation/SVGSMILElement.h"
+#include "core/svg/properties/SVGPropertyInfo.h"
+#include "core/testing/DummyPageHolder.h"
+#include "platform/geometry/IntSize.h"
+#include "platform/weborigin/KURL.h"
+#include "wtf/Vector.h"
+#include "wtf/text/AtomicString.h"
+#include "wtf/text/WTFString.h"
+#include <gtest/gtest.h>
+
+namespace blink {
+
+// The following SVG elements, although related to animation, cannot
+// set JavaScript URLs:
+//
+// - 'discard' can only remove elements, not set their attributes
+// - 'animateMotion' does not use attribute name and produces floats
+// - 'animateTransform' can only animate transform lists
+
+// Pastes htmlToPaste into the body of pageHolder's document, and
+// returns the new content of the body.
+String contentAfterPastingHTML(
+    DummyPageHolder* pageHolder,
+    const char* htmlToPaste)
+{
+    LocalFrame& frame = pageHolder->frame();
+    HTMLElement* body = pageHolder->document().body();
+
+    // Make the body editable, and put the caret in it.
+    body->setAttribute(HTMLNames::contenteditableAttr, "true");
+    frame.selection().setSelection(VisibleSelection::selectionFromContentsOfNode(body));
+    EXPECT_EQ(CaretSelection, frame.selection().selectionType());
+    EXPECT_TRUE(frame.selection().isContentEditable()) <<
+        "We should be pasting into something editable.";
+
+    Pasteboard pasteboard;
+    pasteboard.writeHTML(htmlToPaste, blankURL(), "", false);

[dcheng, 2015/06/10 09:35:28]

Can you remind me how this works for the Blink unit tests? Internally,
Pasteboard uses Platform::current()->clipboard() to get to the clipboard: are we
installing a test Platform implementation that exposes a mocked WebClipboard?

+    frame.editor().pasteWithPasteboard(&pasteboard);

[dcheng, 2015/06/10 09:35:28]

How about frame.editor().execCommand("paste") to avoid the need to friend a test
function in Editor's declaration?

+
+    return body->innerHTML();
+}
+
+// Integration tests.
+//
+// Other sanitization integration tests are layout tests that use
+// document.execCommand('Copy') to source content that they later
+// paste. However SVG animation elements are not serialized when
+// copying, which means we can't test sanitizing these attributes in
+// layout tests: there is nowhere to source the unsafe content from.
+
+TEST(UnsafeSVGAttributeSanitizationTest, pasteAnchor_javaScriptHrefIsStripped)
+{
+    OwnPtr<DummyPageHolder> pageHolder = DummyPageHolder::create(IntSize(1, 1));
+    const char* unsafeContent =

[dcheng, 2015/06/10 09:35:28]

Nit: const char* unsafeContent is actually a mutable pointer to a string
constant, and can be reassigned. const char unsafeContent[] is completely
immutable and uses one pointer less storage.

[Tom Sepez, 2015/06/10 17:29:02]

nit: Don't you want static const char unsafeContent[] in that case? 

+        "<svg xmlns='http://www.w3.org/2000/svg' "
+        "     xmlns:xlink='http://www.w3.org/1999/xlink'"
+        "     width='1cm' height='1cm'>"
+        "  <a xlink:href='javascript:alert()'></a>"
+        "</svg>";
+    String sanitizedContent =
+        contentAfterPastingHTML(pageHolder.get(), unsafeContent);
+
+    EXPECT_TRUE(sanitizedContent.contains("</a>")) <<
+        "We should have pasted *something*; the document is: " <<
+        sanitizedContent.utf8().data();
+    EXPECT_FALSE(sanitizedContent.contains("javascript:")) <<
+        "The JavaScript URL is unsafe and should have been stripped; "
+        "instead: " <<
+        sanitizedContent.utf8().data();
+}
+
+TEST(
+    UnsafeSVGAttributeSanitizationTest,

[Tom Sepez, 2015/06/10 17:29:02]

Nit: funny indentation? I'd expect UnsafeSVGAttributeSanitizationTest to be on
the same line with TEST(

+    pasteAnimatedAnchor_javaScriptHrefIsStripped)
+{
+    OwnPtr<DummyPageHolder> pageHolder = DummyPageHolder::create(IntSize(1, 1));
+    const char* unsafeContent =
+        "<svg xmlns='http://www.w3.org/2000/svg' "
+        "     xmlns:xlink='http://www.w3.org/1999/xlink'"
+        "     width='1cm' height='1cm'>"
+        "  <a xlink:href='https://www.google.com/'>"
+        "    <animate xmlns:ng='http://www.w3.org/1999/xlink' "
+        "             attributeName='ng:href' values='evil;javascript:alert()'>"

[Tom Sepez, 2015/06/10 17:29:02]

nit: How about writing JavASCRipT in mixed case in one of these, just because? 
Also, I presume HTML entities are already resolved, but writing j&#x41vascript:
or jAvascript: might be nice, too.

+        "  </a>"
+        "</svg>";
+    String sanitizedContent =
+        contentAfterPastingHTML(pageHolder.get(), unsafeContent);
+
+    EXPECT_TRUE(sanitizedContent.contains("<a xlink:href=\"https://www.goo")) <<
+        "We should have pasted *something*; the document is: " <<
+        sanitizedContent.utf8().data();
+    EXPECT_FALSE(sanitizedContent.contains("javascript:")) <<
+        "The JavaScript URL is unsafe and should have been stripped; "
+        "instead: " <<
+        sanitizedContent.utf8().data();
+}
+
+// Unit tests
+
+// stripScriptingAttributes inspects animation attributes for
+// javascript: URLs. This check could be defeated if strings supported
+// addition. If this test starts failing you must strengthen
+// Element::stripScriptingAttributes perhaps to strip all
+// isSVGAttributeValueAnimatingAttribute attributes.
+TEST(UnsafeSVGAttributeSanitizationTest, stringsShouldNotSupportAddition)
+{
+    RefPtrWillBeRawPtr<Document> document = Document::create();
+    RefPtrWillBeRawPtr<SVGElement> target = SVGAElement::create(*document);
+    RefPtrWillBeRawPtr<SVGAnimateElement> element = SVGAnimateElement::create(*document);
+    element->setTargetElement(target.get());
+    element->setAttributeName(XLinkNames::hrefAttr);
+
+    // Sanity check that xlink:href was identified as a "string" attribute
+    EXPECT_EQ(AnimatedString, element->animatedPropertyType());
+
+    EXPECT_FALSE(element->animatedPropertyTypeSupportsAddition());
+}
+
+TEST(
+    UnsafeSVGAttributeSanitizationTest,
+    stripScriptingAttributes_stripsHrefContainingJavascriptURL)
+{
+    Vector<Attribute> attributes;
+    attributes.append(Attribute(XLinkNames::hrefAttr, "javascript:alert()"));
+
+    RefPtrWillBeRawPtr<Document> document = Document::create();
+    RefPtrWillBeRawPtr<Element> element = SVGAElement::create(*document);
+    element->stripScriptingAttributes(attributes);
+
+    EXPECT_EQ(0ul, attributes.size()) <<
+        "stripScriptingAttributes should strip an 'xlink:href' attribute with "
+        "a JavaScript URL value.";
+}
+
+TEST(
+    UnsafeSVGAttributeSanitizationTest,
+    stripScriptingAttributes_stripsHrefContainingJavascriptURL_alternatePrefix)
+{
+    Vector<Attribute> attributes;
+    QualifiedName hrefAlternatePrefix(
+        "foo", "href", XLinkNames::xlinkNamespaceURI);
+    attributes.append(Attribute(hrefAlternatePrefix, "javascript:alert()"));
+
+    RefPtrWillBeRawPtr<Document> document = Document::create();
+    RefPtrWillBeRawPtr<Element> element = SVGAElement::create(*document);
+    element->stripScriptingAttributes(attributes);
+
+    EXPECT_EQ(0ul, attributes.size()) <<
+        "stripScriptingAttributes should strip an XLink 'href' attribute with "
+        "a JavaScript URL value, even if the attribute doesn't use the typical "
+        "'xlink' prefix.";
+}
+
+TEST(
+    UnsafeSVGAttributeSanitizationTest,
+    stripScriptingAttributes_stripsAnimationsThatSetJavascriptURLs_from)
+{
+    Vector<Attribute> attributes;
+    attributes.append(Attribute(SVGNames::fromAttr, "javascript:alert()"));
+
+    RefPtrWillBeRawPtr<Document> document = Document::create();
+    RefPtrWillBeRawPtr<Element> element = SVGAnimateElement::create(*document);
+    element->stripScriptingAttributes(attributes);
+
+    EXPECT_EQ(0ul, attributes.size()) <<
+        "stripScriptingAttributes should strip an animation 'from' attribute "
+        "with a JavaScript URL value.";
+}
+
+TEST(
+    UnsafeSVGAttributeSanitizationTest,
+    stripScriptingAttributes_stripsAnimationsThatSetJavascriptURLs_to)
+{
+    Vector<Attribute> attributes;
+    attributes.append(Attribute(SVGNames::toAttr, "javascript:window.close()"));
+
+    RefPtrWillBeRawPtr<Document> document = Document::create();
+    RefPtrWillBeRawPtr<Element> element = SVGSetElement::create(*document);
+    element->stripScriptingAttributes(attributes);
+
+    EXPECT_EQ(0ul, attributes.size()) <<
+        "stripScriptingAttributes should strip an animation 'to' attribute "
+        "with a JavaScript URL value.";
+}
+
+TEST(
+    UnsafeSVGAttributeSanitizationTest,
+    stripScriptingAttributes_stripsAnimationsThatSetJavascriptURLs_values)
+{
+    Vector<Attribute> attributes;
+    attributes.append(
+        Attribute(SVGNames::valuesAttr, "evil; javascript:confirm()"));
+
+    RefPtrWillBeRawPtr<Document> document = Document::create();
+    RefPtrWillBeRawPtr<Element> element = SVGAnimateElement::create(*document);
+    element = SVGAnimateElement::create(*document);
+    element->stripScriptingAttributes(attributes);
+
+    EXPECT_EQ(0ul, attributes.size()) <<
+        "stripScriptingAttributes should strip an animation 'values' attribute "
+        "which contains a JavaScript URL value.";
+}
+
+TEST(
+    UnsafeSVGAttributeSanitizationTest,
+    stripScriptingAttributes_doesNotStripInnocuousAnimationAttributes)
+{
+    Vector<Attribute> attributes;
+    attributes.append(Attribute(SVGNames::fromAttr, "hello, world!"));
+
+    RefPtrWillBeRawPtr<Document> document = Document::create();
+    RefPtrWillBeRawPtr<Element> element = SVGSetElement::create(*document);
+    element->stripScriptingAttributes(attributes);
+
+    EXPECT_EQ(1ul, attributes.size()) <<
+        "stripScriptingAttributes should not strip an animation 'from' "
+        "attribute with a non-JavaScript URL value.";
+}
+
+TEST(
+    UnsafeSVGAttributeSanitizationTest,
+    stripScriptingAttributes_doesNotStripAnimationAttributesInOtherContexts)
+{
+    Vector<Attribute> attributes;
+    attributes.append(Attribute(SVGNames::fromAttr, "javascript:alert()"));
+
+    RefPtrWillBeRawPtr<Document> document = Document::create();
+    RefPtrWillBeRawPtr<Element> element = SVGDiscardElement::create(*document);
+    element->stripScriptingAttributes(attributes);
+
+    EXPECT_EQ(1ul, attributes.size()) <<
+        "stripScriptingAttributes should not strip a 'from' attribute used on "
+        "a non-animation element.";
+}
+
+} // namespace blink