From 6955b2fd5abeeef179e6a8d4ebe2e5d472edb6e7 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Tue, 6 Sep 2022 17:01:56 +0100
Subject: [PATCH] Widened svg content attribute xss filtering

Takes care of additional cases that can occur.
Closes #3705
---
 app/Util/HtmlContentFilter.php   | 7 ++++---
 tests/Entity/PageContentTest.php | 3 +++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/app/Util/HtmlContentFilter.php b/app/Util/HtmlContentFilter.php
index 182f6e63529..5e3c4822c2e 100644
--- a/app/Util/HtmlContentFilter.php
+++ b/app/Util/HtmlContentFilter.php
@@ -45,10 +45,11 @@ public static function removeScripts(string $html): string
         $badIframes = $xPath->query('//*[' . static::xpathContains('@src', 'data:') . '] | //*[' . static::xpathContains('@src', 'javascript:') . '] | //*[@srcdoc]');
         static::removeNodes($badIframes);
 
-        // Remove tags hiding JavaScript or data uris in values attribute.
+        // Remove attributes, within svg children, hiding JavaScript or data uris.
+        // A bunch of svg element and attribute combinations expose xss possibilities.
         // For example, SVG animate tag can exploit javascript in values.
-        $badValuesTags = $xPath->query('//*[' . static::xpathContains('@values', 'data:') . '] | //*[' . static::xpathContains('@values', 'javascript:') . ']');
-        static::removeNodes($badValuesTags);
+        $badValuesAttrs = $xPath->query('//svg//@*[' . static::xpathContains('.', 'data:') . '] | //svg//@*[' . static::xpathContains('.', 'javascript:') . ']');
+        static::removeAttributes($badValuesAttrs);
 
         // Remove elements with a xlink:href attribute
         // Used in SVG but deprecated anyway, so we'll be a bit more heavy-handed here.
diff --git a/tests/Entity/PageContentTest.php b/tests/Entity/PageContentTest.php
index f88e4d513f0..1c051958630 100644
--- a/tests/Entity/PageContentTest.php
+++ b/tests/Entity/PageContentTest.php
@@ -333,6 +333,9 @@ public function test_svg_script_usage_is_removed()
             '<svg><animate href=#xss attributeName=href values=javascript:alert(1) /></svg>',
             '<svg><animate href="#xss" attributeName="href" values="a;javascript:alert(1)" /></svg>',
             '<svg><animate href="#xss" attributeName="href" values="a;data:alert(1)" /></svg>',
+            '<svg><animate href=#xss attributeName=href from=javascript:alert(1) to=1 /><a id=xss><text x=20 y=20>XSS</text></a>',
+            '<svg><set href=#xss attributeName=href from=? to=javascript:alert(1) /><a id=xss><text x=20 y=20>XSS</text></a>',
+            '<svg><g><g><g><animate href=#xss attributeName=href values=javascript:alert(1) /></g></g></g></svg>',
         ];
 
         $this->asEditor();