CVE-2023-29528: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: XWiki Commons are technical libraries common to several other top level XWiki projects. The "restricted" mode of the HTML cleaner in XWiki, introduced in version 4.2-milestone-1 and massively improved in version 14.6-rc-1, allowed the injection of arbitrary HTML code and thus cross-site scripting via invalid HTML comments. As a consequence, any code relying on this "restricted" mode for security is vulnerable to JavaScript injection ("cross-site scripting"/XSS). When a privileged user with programming rights visits such a comment in XWiki, the malicious JavaScript code is executed in the context of the user session. This allows server-side code execution with programming rights, impacting the confidentiality, integrity and availability of the XWiki instance. This problem has been patched in XWiki 14.10, HTML comments are now removed in restricted mode and a check has been introduced that ensures that comments don't start with `>`. There are no known workarounds apart from upgrading to a version including the fix.

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: 2023-04-20T18:15Z
Repository: https://github.com/xwiki/xwiki-commons
Language: Java
Files changed: xwiki-commons-core/xwiki-commons-xml/src/main/java/org/xwiki/xml/html/HTMLUtils.java, xwiki-commons-core/xwiki-commons-xml/src/main/java/org/xwiki/xml/internal/html/DefaultHTMLCleaner.java, xwiki-commons-core/xwiki-commons-xml/src/test/java/org/xwiki/xml/internal/html/DefaultHTMLCleanerTest.java
Commit date: 2022-11-16 18:20:53 +0100
Commit message: XCOMMONS-2568: Improve comment handling in HTMLCleaner (#306)


Diff: diff --git a/xwiki-commons-core/xwiki-commons-xml/src/main/java/org/xwiki/xml/html/HTMLUtils.java b/xwiki-commons-core/xwiki-commons-xml/src/main/java/org/xwiki/xml/html/HTMLUtils.java
index 961b648235..075360fb08 100644
--- a/xwiki-commons-core/xwiki-commons-xml/src/main/java/org/xwiki/xml/html/HTMLUtils.java
+++ b/xwiki-commons-core/xwiki-commons-xml/src/main/java/org/xwiki/xml/html/HTMLUtils.java
@@ -24,10 +24,11 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import org.jdom.Comment;
 import org.jdom.DocType;
 import org.jdom.Element;
 import org.jdom.input.DOMBuilder;
 import org.jdom.output.Format;
 import org.jdom.output.XMLOutputter;
@@ -177,10 +178,26 @@ protected void printElement(Writer out, Element element, int level, NamespaceSta
             } finally {
                 // Reset the format
                 currentFormat.setExpandEmptyElements(currentFormatPolicy);
             }
         }
+
+        @Override
+        protected void printComment(Writer out, Comment comment) throws IOException
+        {
+            String commentText = comment.getText();
+
+            // TODO: remove this again when https://sourceforge.net/p/htmlcleaner/bugs/234/ has been fixed.
+            // Make sure that the comment text conforms to the HTML specification, in particular: "Optionally, text,
+            // with the additional restriction that the text must not start with the string ">", nor start with the
+            // string "->", nor contain the strings "<!--", "-->", or "--!>", nor end with the string "<!-"."
+            while (commentText.startsWith(">") || commentText.startsWith("->")) {
+                commentText = commentText.substring(1);
+            }
+
+            super.printComment(out, new Comment(commentText));
+        }
     }
 
     /**
      * Private constructor since this is a utility class that shouldn't be instantiated (all methods are static).
      */
diff --git a/xwiki-commons-core/xwiki-commons-xml/src/main/java/org/xwiki/xml/internal/html/DefaultHTMLCleaner.java b/xwiki-commons-core/xwiki-commons-xml/src/main/java/org/xwiki/xml/internal/html/DefaultHTMLCleaner.java
index 10a4db0b71..c0edea4d98 100644
--- a/xwiki-commons-core/xwiki-commons-xml/src/main/java/org/xwiki/xml/internal/html/DefaultHTMLCleaner.java
+++ b/xwiki-commons-core/xwiki-commons-xml/src/main/java/org/xwiki/xml/internal/html/DefaultHTMLCleaner.java
@@ -265,10 +265,17 @@ private CleanerProperties getDefaultCleanerProperties(HTMLCleanerConfiguration c
         boolean translateSpecialEntities = (param != null) && Boolean.parseBoolean(param);
         defaultProperties.setTranslateSpecialEntities(translateSpecialEntities);
 
         defaultProperties.setDeserializeEntities(true);
 
+        // Omit comments in restricted mode to avoid any potential parser confusion.
+        // Any part of the filtered HTML that contains unfiltered input is potentially dangerous/a candidate for
+        // parser confusion. Comments, style and script elements seem to be frequently found ingredients in successful
+        // attacks against good sanitizers. We're already removing style and script elements, so removing comments
+        // seems like a good defense against future attacks.
+        defaultProperties.setOmitComments(isRestricted(configuration));
+
         return defaultProperties;
     }
 
     /**
      * @param configuration The cleaner configuration.
@@ -312,12 +319,11 @@ private TrimAttributeCleanerTransformations getDefaultCleanerTransformations(HTM
             tt = new TrimAttributeTagTransformation(HTMLConstants.TAG_TT, HTMLConstants.TAG_SPAN);
             tt.addAttributeTransformation(HTMLConstants.ATTRIBUTE_CLASS, "${class} monospace");
             defaultTransformations.addTransformation(tt);
         }
 
-        String restricted = configuration.getParameters().get(HTMLCleanerConfiguration.RESTRICTED);
-        if ("true".equalsIgnoreCase(restricted)) {
+        if (isRestricted(configuration)) {
 
             tt = new TagTransformation(HTMLConstants.TAG_SCRIPT, HTMLConstants.TAG_PRE, false);
             defaultTransformations.addTransformation(tt);
 
             tt = new TagTransformation(HTMLConstants.TAG_STYLE, HTMLConstants.TAG_PRE, false);
@@ -334,10 +340,20 @@ private TrimAttributeCleanerTransformations getDefaultCleanerTransformations(HTM
     private boolean isHTML5(HTMLCleanerConfiguration configuration)
     {
         return getHTMLVersion(configuration) == 5;
     }
 
+    /**
+     * @param configuration the configuration to parse
+     * @return if the parsing should happen in restricted mode
+     */
+    private boolean isRestricted(HTMLCleanerConfiguration configuration)
+    {
+        String restricted = configuration.getParameters().get(HTMLCleanerConfiguration.RESTRICTED);
+        return "true".equalsIgnoreCase(restricted);
+    }
+
     /**
      * @param configuration The configuration to parse.
      * @return The HTML version specified in the configuration.
      */
     private int getHTMLVersion(HTMLCleanerConfiguration configuration)
diff --git a/xwiki-commons-core/xwiki-commons-xml/src/test/java/org/xwiki/xml/internal/html/DefaultHTMLCleanerTest.java b/xwiki-commons-core/xwiki-commons-xml/src/test/java/org/xwiki/xml/internal/html/DefaultHTMLCleanerTest.java
index 7029671819..844404d8a3 100644
--- a/xwiki-commons-core/xwiki-commons-xml/src/test/java/org/xwiki/xml/internal/html/DefaultHTMLCleanerTest.java
+++ b/xwiki-commons-core/xwiki-commons-xml/src/test/java/org/xwiki/xml/internal/html/DefaultHTMLCleanerTest.java
@@ -31,10 +31,12 @@
 import org.htmlcleaner.HtmlCleaner;
 import org.htmlcleaner.TagNode;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 import org.w3c.dom.Document;
 import org.w3c.dom.NodeList;
 import org.xwiki.component.manager.ComponentManager;
 import org.xwiki.test.junit5.mockito.ComponentTest;
 import org.xwiki.test.junit5.mockito.InjectMockComponents;
@@ -325,10 +327,48 @@ void restrictedAttributesAndTags() throws Exception
                 + "</mi></math></p>",
             "<math xmlns=\"http://www.w3.org/1998/Math/MathML\"><span></span><mtext>X</mtext><mi><span>foo</span>"
                 + "</mi></math>");
     }
 
+    /**
+     * Verify comment handling in restricted mode.
+     */
+    @ParameterizedTest
+    @CsvSource({
+        "<p><strong>Hello  World</strong></p>,<strong>Hello <!-- a comment --> World</strong>",
+        "'', <!--My favorite operators are > and <!-->",
+        // FIXME: Actually, just the comment should be removed but due to erroneous parsing in HTMLCleaner, the whole
+        // string is treated as a comment.
+        "'', <!--> <a href=\"#\">no comment</a>",
+        "'', <!---> <a href=\"#\">no comment</a>"
+    })
+    void restrictedComments(String expected, String actual)
+    {
+        Map<String, String> parameters = new HashMap<>(this.cleanerConfiguration.getParameters());
+        parameters.put("restricted", "true");
+        this.cleanerConfiguration.setParameters(parameters);
+
+        assertHTML(expected, actual);
+    }
+
+    @ParameterizedTest
+    @CsvSource({
+        "<!--My favorite operators are > and <!-->, <!--My favorite operators are > and <!-->",
+        "<!-- a comment ==!> not a comment-->, <!-- a comment --!> not a comment",
+        // FIXME: this is wrongly parsed as a full comment.
+        "<!-- <a foo=`bar`>not a comment</a>-->, <!--> <a foo=`bar`>not a comment</a>",
+        "<!--=>-->, <!--->",
+        // FIXME: according to the HTML specification, this should be a comment.
+        "'', <! fake comment >",
+        "<!-- <!== comment -->, <!-- <!-- comment -->",
+        "<!--My favorite operators are > and <!=-->, <!--My favorite operators are > and <!--->"
+    })
+    void comments(String expected, String actual)
+    {
+        assertHTML(expected, actual);
+    }
+
     /**
      * Verify that passing a fully-formed XHTML header works fine.
      */
     @Test
     void fullXHTMLHeader()

Vulnerable code:         String restricted = configuration.getParameters().get(HTMLCleanerConfiguration.RESTRICTED);
        if ("true".equalsIgnoreCase(restricted)) {
Fixed code: import org.jdom.Comment;

        @Override
        protected void printComment(Writer out, Comment comment) throws IOException
        {
            String commentText = comment.getText();

            // TODO: remove this again when https://sourceforge.net/p/htmlcleaner/bugs/234/ has been fixed.
            // Make sure that the comment text conforms to the HTML specification, in particular: "Optionally, text,
            // with the additional restriction that the text must not start with the string ">", nor start with the
            // string "->", nor contain the strings "<!--", "-->", or "--!>", nor end with the string "<!-"."
            while (commentText.startsWith(">") || commentText.startsWith("->")) {
                commentText = commentText.substring(1);
            }

            super.printComment(out, new Comment(commentText));
        }
        // Omit comments in restricted mode to avoid any potential parser confusion.
        // Any part of the filtered HTML that contains unfiltered input is potentially dangerous/a candidate for
        // parser confusion. Comments, style and script elements seem to be frequently found ingredients in successful
        // attacks against good sanitizers. We're already removing style and script elements, so removing comments
        // seems like a good defense against future attacks.
        defaultProperties.setOmitComments(isRestricted(configuration));

        if (isRestricted(configuration)) {
    /**
     * @param configuration the configuration to parse
     * @return if the parsing should happen in restricted mode
     */
    private boolean isRestricted(HTMLCleanerConfiguration configuration)
    {
        String restricted = configuration.getParameters().get(HTMLCleanerConfiguration.RESTRICTED);
        return "true".equalsIgnoreCase(restricted);
    }

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
    /**
     * Verify comment handling in restricted mode.
     */
    @ParameterizedTest
    @CsvSource({
        "<p><strong>Hello  World</strong></p>,<strong>Hello <!-- a comment --> World</strong>",
        "'', <!--My favorite operators are > and <!-->",
        // FIXME: Actually, just the comment should be removed but due to erroneous parsing in HTMLCleaner, the whole
        // string is treated as a comment.
        "'', <!--> <a href=\"#\">no comment</a>",
        "'', <!---> <a href=\"#\">no comment</a>"
    })
    void restrictedComments(String expected, String actual)
    {
        Map<String, String> parameters = new HashMap<>(this.cleanerConfiguration.getParameters());
        parameters.put("restricted", "true");
        this.cleanerConfiguration.setParameters(parameters);

        assertHTML(expected, actual);
    }

    @ParameterizedTest
    @CsvSource({
        "<!--My favorite operators are > and <!-->, <!--My favorite operators are > and <!-->",
        "<!-- a comment ==!> not a comment-->, <!-- a comment --!> not a comment",
        // FIXME: this is wrongly parsed as a full comment.
        "<!-- <a foo=`bar`>not a comment</a>-->, <!--> <a foo=`bar`>not a comment</a>",
        "<!--=>-->, <!--->",
        // FIXME: according to the HTML specification, this should be a comment.
        "'', <! fake comment >",
        "<!-- <!== comment -->, <!-- <!-- comment -->",
        "<!--My favorite operators are > and <!=-->, <!--My favorite operators are > and <!--->"
    })
    void comments(String expected, String actual)
    {
        assertHTML(expected, actual);
    }