CVE-2021-3985: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: kimai2 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: 9
Published: 2021-12-01T11:15Z
Repository: https://github.com/kevinpapst/kimai2
Language: PHP
Files changed: src/Twig/Runtime/MarkdownExtension.php, src/Utils/Markdown.php, tests/Twig/Runtime/MarkdownExtensionTest.php, tests/Utils/MarkdownTest.php
Commit date: 2021-11-19 23:07:27 +0100
Commit message: make sure that markdown uses safe mode (#2961)


Diff: diff --git a/src/Twig/Runtime/MarkdownExtension.php b/src/Twig/Runtime/MarkdownExtension.php
index c73e8eea4..3187796f1 100644
--- a/src/Twig/Runtime/MarkdownExtension.php
+++ b/src/Twig/Runtime/MarkdownExtension.php
@@ -59,11 +59,11 @@ final class MarkdownExtension implements RuntimeExtensionInterface
         if (!$fullLength && \strlen($content) > 101) {
             $content = trim(substr($content, 0, 100)) . ' &hellip;';
         }
 
         if ($this->isMarkdownEnabled()) {
-            $content = $this->markdown->toHtml($content, false);
+            $content = $this->markdown->toHtml($content);
         } elseif ($fullLength) {
             $content = '<p>' . nl2br($content) . '</p>';
         }
 
         return $content;
@@ -110,11 +110,11 @@ final class MarkdownExtension implements RuntimeExtensionInterface
         if (empty($content)) {
             return '';
         }
 
         if ($this->isMarkdownEnabled()) {
-            return $this->markdown->toHtml($content, false);
+            return $this->markdown->toHtml($content);
         }
 
         return nl2br($content);
     }
 
@@ -124,8 +124,8 @@ final class MarkdownExtension implements RuntimeExtensionInterface
      * @param string $content
      * @return string
      */
     public function markdownToHtml(string $content): string
     {
-        return $this->markdown->toHtml($content, false);
+        return $this->markdown->toHtml($content);
     }
 }
diff --git a/src/Utils/Markdown.php b/src/Utils/Markdown.php
index 0988e7c9d..5f21a100a 100644
--- a/src/Utils/Markdown.php
+++ b/src/Utils/Markdown.php
@@ -31,10 +31,15 @@ final class Markdown
      * @param bool $safe
      * @return string
      */
     public function toHtml(string $text, bool $safe = true): string
     {
-        $this->parser->setSafeMode($safe);
+        if ($safe !== true) {
+            @trigger_error('Only safe mode is supported in Markdown since 1.16.3 to prevent XSS attacks. Parameter $safe will be removed with 2.0', E_USER_DEPRECATED);
+        }
+
+        $this->parser->setSafeMode(true);
+        $this->parser->setMarkupEscaped(true);
 
         return $this->parser->text($text);
     }
 }
diff --git a/tests/Twig/Runtime/MarkdownExtensionTest.php b/tests/Twig/Runtime/MarkdownExtensionTest.php
index 56fbaf828..4418f3e9f 100644
--- a/tests/Twig/Runtime/MarkdownExtensionTest.php
+++ b/tests/Twig/Runtime/MarkdownExtensionTest.php
@@ -25,10 +25,14 @@ class MarkdownExtensionTest extends TestCase
         $loader = $this->createMock(ConfigLoaderInterface::class);
         $config = new SystemConfiguration($loader, ['timesheet' => ['markdown_content' => true]]);
         $sut = new MarkdownExtension(new Markdown(), $config);
         $this->assertEquals('<p><em>test</em></p>', $sut->markdownToHtml('*test*'));
         $this->assertEquals('<p># foobar</p>', $sut->markdownToHtml('# foobar'));
+        $this->assertEquals(
+            '<p><a href="javascript%3Aalert(`XSS`)">XSS</a></p>',
+            $sut->markdownToHtml('[XSS](javascript:alert(`XSS`))')
+        );
     }
 
     public function testTimesheetContent()
     {
         $loader = $this->createMock(ConfigLoaderInterface::class);
@@ -45,10 +49,14 @@ class MarkdownExtensionTest extends TestCase
         $sut = new MarkdownExtension(new Markdown(), $config);
         $this->assertEquals(
             "<ul>\n<li>test</li>\n<li>foo</li>\n</ul>\n<p>foo <strong>bar</strong></p>",
             $sut->timesheetContent("- test\n- foo\n\nfoo __bar__")
         );
+        $this->assertEquals(
+            '<p><a href="javascript%3Aalert(`XSS`)">XSS</a></p>',
+            $sut->timesheetContent('[XSS](javascript:alert(`XSS`))')
+        );
     }
 
     public function testCommentContent()
     {
         $loader = $this->createMock(ConfigLoaderInterface::class);
@@ -74,10 +82,14 @@ class MarkdownExtensionTest extends TestCase
         $sut = new MarkdownExtension(new Markdown(), $config);
         $this->assertEquals(
             "<ul>\n<li>test</li>\n<li>foo</li>\n</ul>\n<p>foo <strong>bar</strong></p>",
             $sut->commentContent("- test\n- foo\n\nfoo __bar__")
         );
+        $this->assertEquals(
+            '<p><a href="javascript%3Aalert(`XSS`)">XSS</a></p>',
+            $sut->commentContent('[XSS](javascript:alert(`XSS`))')
+        );
     }
 
     public function testCommentOneLiner()
     {
         $loader = $this->createMock(ConfigLoaderInterface::class);
diff --git a/tests/Utils/MarkdownTest.php b/tests/Utils/MarkdownTest.php
index 5871a5b99..b021bf8b9 100644
--- a/tests/Utils/MarkdownTest.php
+++ b/tests/Utils/MarkdownTest.php
@@ -79,9 +79,27 @@ EOT;
         $markdown = <<<EOT
 # test
 ## test
 ### test
 # test
+EOT;
+        $this->assertEquals($html, $sut->toHtml($markdown));
+    }
+
+    public function testLinksAreSanitized()
+    {
+        $sut = new Markdown();
+
+        $html = <<<'EOT'
+<p><a href="javascript%3Aalert(`XSS`)">XSS</a><br />
+<a href="javascript%3Aalert(&quot;XSS&quot;)">XSS</a><br />
+<a href="javascript%3Aalert(&#039;XSS&#039;)">XSS</a></p>
+EOT;
+
+        $markdown = <<<EOT
+[XSS](javascript:alert(`XSS`))
+[XSS](javascript:alert("XSS"))
+[XSS](javascript:alert('XSS'))
 EOT;
         $this->assertEquals($html, $sut->toHtml($markdown));
     }
 }

Vulnerable code:             $content = $this->markdown->toHtml($content, false);
            return $this->markdown->toHtml($content, false);
        return $this->markdown->toHtml($content, false);
        $this->parser->setSafeMode($safe);
Fixed code:             $content = $this->markdown->toHtml($content);
            return $this->markdown->toHtml($content);
        return $this->markdown->toHtml($content);
        if ($safe !== true) {
            @trigger_error('Only safe mode is supported in Markdown since 1.16.3 to prevent XSS attacks. Parameter $safe will be removed with 2.0', E_USER_DEPRECATED);
        }

        $this->parser->setSafeMode(true);
        $this->parser->setMarkupEscaped(true);
        $this->assertEquals(
            '<p><a href="javascript%3Aalert(`XSS`)">XSS</a></p>',
            $sut->markdownToHtml('[XSS](javascript:alert(`XSS`))')
        );
        $this->assertEquals(
            '<p><a href="javascript%3Aalert(`XSS`)">XSS</a></p>',
            $sut->timesheetContent('[XSS](javascript:alert(`XSS`))')
        );
        $this->assertEquals(
            '<p><a href="javascript%3Aalert(`XSS`)">XSS</a></p>',
            $sut->commentContent('[XSS](javascript:alert(`XSS`))')
        );
EOT;
        $this->assertEquals($html, $sut->toHtml($markdown));
    }

    public function testLinksAreSanitized()
    {
        $sut = new Markdown();

        $html = <<<'EOT'
<p><a href="javascript%3Aalert(`XSS`)">XSS</a><br />
<a href="javascript%3Aalert(&quot;XSS&quot;)">XSS</a><br />
<a href="javascript%3Aalert(&#039;XSS&#039;)">XSS</a></p>
EOT;

        $markdown = <<<EOT
[XSS](javascript:alert(`XSS`))
[XSS](javascript:alert("XSS"))
[XSS](javascript:alert('XSS'))