CVE-2021-3767: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: bookstack is vulnerable to Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting')
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
CVSS v3 base score: 5.4
Published: 2021-09-06T12:15Z
Repository: https://github.com/bookstackapp/bookstack
Language: PHP
Files changed: app/Util/HtmlContentFilter.php, tests/Entity/PageContentTest.php
Commit date: 2021-09-03 22:34:49 +0100
Commit message: Added filter for xlink:href svg xss

Simply remove all such attributes

Diff: diff --git a/app/Util/HtmlContentFilter.php b/app/Util/HtmlContentFilter.php
index 729b80474..f3f29ae04 100644
--- a/app/Util/HtmlContentFilter.php
+++ b/app/Util/HtmlContentFilter.php
@@ -1,9 +1,10 @@
 <?php
 
 namespace BookStack\Util;
 
+use DOMAttr;
 use DOMDocument;
 use DOMNodeList;
 use DOMXPath;
 
 class HtmlContentFilter
@@ -41,17 +42,18 @@ class HtmlContentFilter
 
         // Remove data or JavaScript iFrames
         $badIframes = $xPath->query('//*[' . static::xpathContains('@src', 'data:') . '] | //*[' . static::xpathContains('@src', 'javascript:') . '] | //*[@srcdoc]');
         static::removeNodes($badIframes);
 
+        // Remove elements with a xlink:href attribute
+        // Used in SVG but deprecated anyway, so we'll be a bit more heavy-handed here.
+        $xlinkHrefAttributes = $xPath->query('//@*[contains(name(), \'xlink:href\')]');
+        static::removeAttributes($xlinkHrefAttributes);
+
         // Remove 'on*' attributes
         $onAttributes = $xPath->query('//@*[starts-with(name(), \'on\')]');
-        foreach ($onAttributes as $attr) {
-            /** @var \DOMAttr $attr */
-            $attrName = $attr->nodeName;
-            $attr->parentNode->removeAttribute($attrName);
-        }
+        static::removeAttributes($onAttributes);
 
         $html = '';
         $topElems = $doc->documentElement->childNodes->item(0)->childNodes;
         foreach ($topElems as $child) {
             $html .= $doc->saveHTML($child);
@@ -70,14 +72,26 @@ class HtmlContentFilter
         $upperVal = strtoupper($value);
         return 'contains(translate(' . $property . ', \'' . $upperVal . '\', \'' . $value . '\'), \'' . $value . '\')';
     }
 
     /**
-     * Removed all of the given DOMNodes.
+     * Remove all the given DOMNodes.
      */
     protected static function removeNodes(DOMNodeList $nodes): void
     {
         foreach ($nodes as $node) {
             $node->parentNode->removeChild($node);
         }
     }
+
+    /**
+     * Remove all the given attribute nodes.
+     */
+    protected static function removeAttributes(DOMNodeList $attrs): void
+    {
+        /** @var DOMAttr $attr */
+        foreach ($attrs as $attr) {
+            $attrName = $attr->nodeName;
+            $attr->parentNode->removeAttribute($attrName);
+        }
+    }
 }
diff --git a/tests/Entity/PageContentTest.php b/tests/Entity/PageContentTest.php
index 193f81400..1b2ce2db2 100644
--- a/tests/Entity/PageContentTest.php
+++ b/tests/Entity/PageContentTest.php
@@ -303,10 +303,32 @@ class PageContentTest extends TestCase
         $pageView = $this->get($page->getUrl());
         $pageView->assertSee($script);
         $pageView->assertDontSee('abc123abc123');
     }
 
+    public function test_svg_xlink_hrefs_are_removed()
+    {
+        $checks = [
+            '<svg id="test" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="100" height="100"><a xlink:href="javascript:alert(document.domain)"><rect x="0" y="0" width="100" height="100" /></a></svg>',
+            '<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"><use xlink:href="data:application/xml;base64 ,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHhtbG5zOnhsaW5rPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5L3hsaW5rIj4KPGRlZnM+CjxjaXJjbGUgaWQ9InRlc3QiIHI9IjAiIGN4PSIwIiBjeT0iMCIgc3R5bGU9ImZpbGw6ICNGMDAiPgo8c2V0IGF0dHJpYnV0ZU5hbWU9ImZpbGwiIGF0dHJpYnV0ZVR5cGU9IkNTUyIgb25iZWdpbj0nYWxlcnQoZG9jdW1lbnQuZG9tYWluKScKb25lbmQ9J2FsZXJ0KCJvbmVuZCIpJyB0bz0iIzAwRiIgYmVnaW49IjBzIiBkdXI9Ijk5OXMiIC8+CjwvY2lyY2xlPgo8L2RlZnM+Cjx1c2UgeGxpbms6aHJlZj0iI3Rlc3QiLz4KPC9zdmc+#test"/></svg>'
+        ];
+
+        $this->asEditor();
+        $page = Page::query()->first();
+
+        foreach ($checks as $check) {
+            $page->html = $check;
+            $page->save();
+
+            $pageView = $this->get($page->getUrl());
+            $pageView->assertStatus(200);
+            $pageView->assertElementNotContains('.page-content', 'alert');
+            $pageView->assertElementNotContains('.page-content', 'xlink:href');
+            $pageView->assertElementNotContains('.page-content', 'application/xml');
+        }
+    }
+
     public function test_page_inline_on_attributes_show_if_configured()
     {
         $this->asEditor();
         $page = Page::query()->first();
         config()->push('app.allow_content_scripts', 'true');

Vulnerable code:         foreach ($onAttributes as $attr) {
            /** @var \DOMAttr $attr */
            $attrName = $attr->nodeName;
            $attr->parentNode->removeAttribute($attrName);
        }
     * Removed all of the given DOMNodes.
Fixed code: use DOMAttr;
        // Remove elements with a xlink:href attribute
        // Used in SVG but deprecated anyway, so we'll be a bit more heavy-handed here.
        $xlinkHrefAttributes = $xPath->query('//@*[contains(name(), \'xlink:href\')]');
        static::removeAttributes($xlinkHrefAttributes);

        static::removeAttributes($onAttributes);
     * Remove all the given DOMNodes.

    /**
     * Remove all the given attribute nodes.
     */
    protected static function removeAttributes(DOMNodeList $attrs): void
    {
        /** @var DOMAttr $attr */
        foreach ($attrs as $attr) {
            $attrName = $attr->nodeName;
            $attr->parentNode->removeAttribute($attrName);
        }
    }
    public function test_svg_xlink_hrefs_are_removed()
    {
        $checks = [
            '<svg id="test" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="100" height="100"><a xlink:href="javascript:alert(document.domain)"><rect x="0" y="0" width="100" height="100" /></a></svg>',
            '<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"><use xlink:href="data:application/xml;base64 ,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHhtbG5zOnhsaW5rPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5L3hsaW5rIj4KPGRlZnM+CjxjaXJjbGUgaWQ9InRlc3QiIHI9IjAiIGN4PSIwIiBjeT0iMCIgc3R5bGU9ImZpbGw6ICNGMDAiPgo8c2V0IGF0dHJpYnV0ZU5hbWU9ImZpbGwiIGF0dHJpYnV0ZVR5cGU9IkNTUyIgb25iZWdpbj0nYWxlcnQoZG9jdW1lbnQuZG9tYWluKScKb25lbmQ9J2FsZXJ0KCJvbmVuZCIpJyB0bz0iIzAwRiIgYmVnaW49IjBzIiBkdXI9Ijk5OXMiIC8+CjwvY2lyY2xlPgo8L2RlZnM+Cjx1c2UgeGxpbms6aHJlZj0iI3Rlc3QiLz4KPC9zdmc+#test"/></svg>'
        ];

        $this->asEditor();
        $page = Page::query()->first();

        foreach ($checks as $check) {
            $page->html = $check;
            $page->save();

            $pageView = $this->get($page->getUrl());
            $pageView->assertStatus(200);
            $pageView->assertElementNotContains('.page-content', 'alert');
            $pageView->assertElementNotContains('.page-content', 'xlink:href');
            $pageView->assertElementNotContains('.page-content', 'application/xml');
        }
    }