CVE-2020-28919: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: A stored cross site scripting (XSS) vulnerability in Checkmk 1.6.0x prior to 1.6.0p19 allows an authenticated remote attacker to inject arbitrary JavaScript via a javascript: URL in a view title.
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: 2022-01-15T17:15Z
Repository: https://github.com/tribe29/checkmk
Language: Python
Files changed: cmk/gui/escaping.py, tests/unit/cmk/gui/test_htmllib_Escaper.py
Commit date: 2020-10-21 09:29:44 +0200
Commit message: Rewrite matching a href unescape regex to separate attributes

The goal of this commit is to separate the values of the href and target
attributes in dedicated match groups. We also exclude the quotes from the
match groups to simplify the code.

Change-Id: I1e64946a1a426d81284b3173db43135ee0d1debc

Diff: diff --git a/cmk/gui/escaping.py b/cmk/gui/escaping.py
index 1761d449e2..03dcdda9ed 100644
--- a/cmk/gui/escaping.py
+++ b/cmk/gui/escaping.py
@@ -28,12 +28,13 @@ from cmk.gui.utils.html import HTML
 # All the HTML-related types are slightly chaotic...
 EscapableEntity = Union[None, int, HTML, str]
 
 _UNESCAPER_TEXT = re.compile(
     r'&lt;(/?)(h1|h2|b|tt|i|u|br(?: /)?|nobr(?: /)?|pre|a|sup|p|li|ul|ol)&gt;')
-_QUOTE = re.compile(r"(?:&quot;|&#x27;)")
-_A_HREF = re.compile(r'&lt;a href=((?:&quot;|&#x27;).*?(?:&quot;|&#x27;))&gt;')
+_A_HREF = re.compile(
+    r'&lt;a href=(?:(?:&quot;|&#x27;)(.*?)(?:&quot;|&#x27;))(?: target=(?:(?:&quot;|&#x27;)(.*?)(?:&quot;|&#x27;)))?&gt;'
+)
 
 
 # TODO: Cleanup the accepted types!
 def escape_attribute(value: EscapableEntity) -> str:
     """Escape HTML attributes.
@@ -109,12 +110,20 @@ def escape_text(text: EscapableEntity) -> str:
         return text.__html__()
 
     text = escape_attribute(text)
     text = _UNESCAPER_TEXT.sub(r'<\1\2>', text)
     for a_href in _A_HREF.finditer(text):
-        text = text.replace(a_href.group(0), u"<a href=%s>" % _QUOTE.sub(u"\"", a_href.group(1)))
-    return text.replace(u"&amp;nbsp;", u"&nbsp;")
+        href = a_href.group(1)
+        target = a_href.group(2)
+
+        if target:
+            unescaped_tag = "<a href=\"%s\" target=\"%s\">" % (href, target)
+        else:
+            unescaped_tag = "<a href=\"%s\">" % href
+
+        text = text.replace(a_href.group(0), unescaped_tag)
+    return text.replace("&amp;nbsp;", u"&nbsp;")
 
 
 def strip_scripts(ht: str) -> str:
     """Strip script tags from text.
 
diff --git a/tests/unit/cmk/gui/test_htmllib_Escaper.py b/tests/unit/cmk/gui/test_htmllib_Escaper.py
index c96ef1b6ae..3a34499bad 100644
--- a/tests/unit/cmk/gui/test_htmllib_Escaper.py
+++ b/tests/unit/cmk/gui/test_htmllib_Escaper.py
@@ -54,10 +54,13 @@ def test_unescape_attribute(inp, out):
         ("<li></li>", None),
         ("<ul></ul>", None),
         ("<ol></ol>", None),
         ("<a href=\"xyz\">abc</a>", None),
         ("<a href=\"xyz\" target=\"123\">abc</a>", None),
+        # Links with target 1st and href 2nd will not be unescaped
+        ("<a target=\"123\" href=\"xyz\">abc</a>",
+         "&lt;a target=&quot;123&quot; href=&quot;xyz&quot;&gt;abc</a>"),
         ("blah<a href=\"link0\">aaa</a>blah<a href=\"link1\" target=\"ttt\">bbb</a>", None),
         ("\"I am not a link\" target=\"still not a link\"",
          "&quot;I am not a link&quot; target=&quot;still not a link&quot;"),
         # The next test is perverse: it contains the string `target=` inside of an
         # <a> tag (which must be unescaped) as well as outside (which must not).
@@ -67,10 +70,12 @@ def test_unescape_attribute(inp, out):
         (
             "<a href=\"xyz\">abc</a><script>alert(1)</script><a href=\"xyz\">abc</a>",
             "<a href=\"xyz\">abc</a>&lt;script&gt;alert(1)&lt;/script&gt;<a href=\"xyz\">abc</a>",
         ),
         ("&nbsp;", None),
+        # At the moment also javascript URLs are accepted. This will be refused in the next step
+        ("<a href=\"javascript:alert(1)\">abc</a>", None),
     ])
 def test_escape_text(inp, out):
     if out is None:
         out = inp
     assert escaping.escape_text(inp) == out

Vulnerable code: _QUOTE = re.compile(r"(?:&quot;|&#x27;)")
_A_HREF = re.compile(r'&lt;a href=((?:&quot;|&#x27;).*?(?:&quot;|&#x27;))&gt;')
        text = text.replace(a_href.group(0), u"<a href=%s>" % _QUOTE.sub(u"\"", a_href.group(1)))
    return text.replace(u"&amp;nbsp;", u"&nbsp;")
Fixed code: _A_HREF = re.compile(
    r'&lt;a href=(?:(?:&quot;|&#x27;)(.*?)(?:&quot;|&#x27;))(?: target=(?:(?:&quot;|&#x27;)(.*?)(?:&quot;|&#x27;)))?&gt;'
)
        href = a_href.group(1)
        target = a_href.group(2)

        if target:
            unescaped_tag = "<a href=\"%s\" target=\"%s\">" % (href, target)
        else:
            unescaped_tag = "<a href=\"%s\">" % href

        text = text.replace(a_href.group(0), unescaped_tag)
    return text.replace("&amp;nbsp;", u"&nbsp;")
        # Links with target 1st and href 2nd will not be unescaped
        ("<a target=\"123\" href=\"xyz\">abc</a>",
         "&lt;a target=&quot;123&quot; href=&quot;xyz&quot;&gt;abc</a>"),
        # At the moment also javascript URLs are accepted. This will be refused in the next step
        ("<a href=\"javascript:alert(1)\">abc</a>", None),