CVE-2023-37908: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: XWiki Rendering is a generic Rendering system that converts textual input in a given syntax into another syntax. The cleaning of attributes during XHTML rendering, introduced in version 14.6-rc-1, allowed the injection of arbitrary HTML code and thus cross-site scripting via invalid attribute names. This can be exploited, e.g., via the link syntax in any content that supports XWiki syntax like comments in XWiki. When a user moves the mouse over a malicious link, the malicious JavaScript code is executed in the context of the user session. When this user is a privileged user who has programming rights, this allows server-side code execution with programming rights, impacting the confidentiality, integrity and availability of the XWiki instance. While this attribute was correctly recognized as not allowed, the attribute was still printed with a prefix `data-xwiki-translated-attribute-` without further cleaning or validation. This problem has been patched in XWiki 14.10.4 and 15.0 RC1 by removing characters not allowed in data attributes and then validating the cleaned attribute again. 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.6
Published: 2023-10-25T18:17Z
Repository: https://github.com/xwiki/xwiki-rendering
Language: Java
Files changed: xwiki-rendering-xml/src/main/java/org/xwiki/rendering/renderer/printer/XHTMLWikiPrinter.java, xwiki-rendering-xml/src/test/java/org/xwiki/rendering/renderer/printer/XHTMLWikiPrinterTest.java
Commit date: 2023-01-13 17:40:51 +0100
Commit message: XRENDERING-697: XHTMLWikiPrinter doesn't validate generated data attributes

* Strip invalid characters and make sure the data attribute is allowed.
* Add tests.

Diff: diff --git a/xwiki-rendering-xml/src/main/java/org/xwiki/rendering/renderer/printer/XHTMLWikiPrinter.java b/xwiki-rendering-xml/src/main/java/org/xwiki/rendering/renderer/printer/XHTMLWikiPrinter.java
index 766250a17..83df754d1 100644
--- a/xwiki-rendering-xml/src/main/java/org/xwiki/rendering/renderer/printer/XHTMLWikiPrinter.java
+++ b/xwiki-rendering-xml/src/main/java/org/xwiki/rendering/renderer/printer/XHTMLWikiPrinter.java
@@ -21,10 +21,12 @@
 
 import java.util.Arrays;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
+import java.util.regex.Pattern;
 
 import org.apache.commons.lang3.StringUtils;
 import org.xml.sax.Attributes;
 import org.xml.sax.helpers.AttributesImpl;
 import org.xwiki.stability.Unstable;
@@ -46,10 +48,21 @@ public class XHTMLWikiPrinter extends XMLWikiPrinter
      * @since 14.6RC1
      */
     @Unstable
     public static final String TRANSLATED_ATTRIBUTE_PREFIX = "data-xwiki-translated-attribute-";
 
+    /**
+     * Pattern for matching characters not allowed in data attributes.
+     * <p>
+     * This is the inverse of the definition of a name being
+     * <a href="https://html.spec.whatwg.org/multipage/infrastructure.html#xml-compatible>XML-compatible</a>,
+     * i.e., matching the <a href="https://www.w3.org/TR/xml/#NT-Name">Name production</a> without ":".
+     */
+    private static final Pattern DATA_REPLACEMENT_PATTERN = Pattern.compile("[^A-Z_a-z\\u00C0-\\u00D6\\u00D8-\\u00F6"
+        + "\\u00F8-\\u02ff\\u0370-\\u037d\\u037f-\\u1fff\\u200c\\u200d\\u2070-\\u218f\\u2c00-\\u2fef\\u3001-\\ud7ff"
+        + "\\uf900-\\ufdcf\\ufdf0-\\ufffd\\x{10000}-\\x{EFFFF}\\-.0-9\\u00b7\\u0300-\\u036f\\u203f-\\u2040]");
+
     /**
      * The sanitizer used to restrict allowed elements and attributes, can be null (no restrictions).
      *
      * @since 14.6RC1
      */
@@ -258,11 +271,17 @@ private Map<String, String> cleanAttributes(String elementName, Map<String, Stri
             cleanAttributes = new LinkedHashMap<>();
             for (Map.Entry<String, String> e : attributes.entrySet()) {
                 if (this.htmlElementSanitizer.isAttributeAllowed(elementName, e.getKey(), e.getValue())) {
                     cleanAttributes.put(e.getKey(), e.getValue());
                 } else {
-                    cleanAttributes.put(TRANSLATED_ATTRIBUTE_PREFIX + e.getKey(), e.getValue());
+                    // Keep but clean invalid attributes with a prefix (removed during parsing) to avoid loosing them
+                    // through WYSIWYG editing.
+                    String translatedName =
+                        TRANSLATED_ATTRIBUTE_PREFIX + removeInvalidDataAttributeCharacters(e.getKey());
+                    if (this.htmlElementSanitizer.isAttributeAllowed(elementName, translatedName, e.getValue())) {
+                        cleanAttributes.put(translatedName, e.getValue());
+                    }
                 }
             }
         }
 
         return cleanAttributes;
@@ -277,13 +296,22 @@ private String[][] cleanAttributes(String elementName, String[][] attributes)
             allowedAttributes = Arrays.stream(attributes)
                 .map(entry -> {
                     if (this.htmlElementSanitizer.isAttributeAllowed(elementName, entry[0], entry[1])) {
                         return entry;
                     } else {
-                        return new String[] { TRANSLATED_ATTRIBUTE_PREFIX + entry[0], entry[1] };
+                        // Keep but clean invalid attributes with a prefix (removed during parsing) to avoid loosing
+                        // them through WYSIWYG editing.
+                        String translatedName =
+                            TRANSLATED_ATTRIBUTE_PREFIX + removeInvalidDataAttributeCharacters(entry[0]);
+                        if (this.htmlElementSanitizer.isAttributeAllowed(elementName, translatedName, entry[1])) {
+                            return new String[] { translatedName, entry[1] };
+                        } else {
+                            return null;
+                        }
                     }
                 })
+                .filter(Objects::nonNull)
                 .toArray(String[][]::new);
         }
 
         return allowedAttributes;
     }
@@ -302,19 +330,38 @@ private Attributes cleanAttributes(String elementName, Attributes attributes)
                     attributes.getValue(i)))
                 {
                     ((AttributesImpl) allowedAttribute).addAttribute(null, null, attributes.getQName(i),
                         null, attributes.getValue(i));
                 } else {
-                    ((AttributesImpl) allowedAttribute).addAttribute(null, null,
-                        TRANSLATED_ATTRIBUTE_PREFIX + attributes.getQName(i), null, attributes.getValue(i));
+                    // Keep but clean invalid attributes with a prefix (removed during parsing) to avoid loosing them
+                    // through WYSIWYG editing.
+                    String translatedName =
+                        TRANSLATED_ATTRIBUTE_PREFIX + removeInvalidDataAttributeCharacters(attributes.getQName(i));
+                    if (this.htmlElementSanitizer.isAttributeAllowed(elementName, translatedName,
+                        attributes.getValue(i)))
+                    {
+                        ((AttributesImpl) allowedAttribute).addAttribute(null, null,
+                            translatedName, null, attributes.getValue(i));
+                    }
                 }
             }
         }
 
         return allowedAttribute;
     }
 
+    /**
+     * Strips out invalid characters from names used for data attributes.
+     *
+     * @param name the data attribute name to clean
+     * @return valid name, to be prefixed with data-
+     */
+    public static String removeInvalidDataAttributeCharacters(String name)
+    {
+        return DATA_REPLACEMENT_PATTERN.matcher(name).replaceAll("");
+    }
+
     private void handleSpaceWhenStartElement()
     {
         // Use case: <tag1>something <tag2>...
         // Use case: <tag1>something <!--...
         if (this.spaceCount > 0) {
diff --git a/xwiki-rendering-xml/src/test/java/org/xwiki/rendering/renderer/printer/XHTMLWikiPrinterTest.java b/xwiki-rendering-xml/src/test/java/org/xwiki/rendering/renderer/printer/XHTMLWikiPrinterTest.java
index 19785cd28..f2b778965 100644
--- a/xwiki-rendering-xml/src/test/java/org/xwiki/rendering/renderer/printer/XHTMLWikiPrinterTest.java
+++ b/xwiki-rendering-xml/src/test/java/org/xwiki/rendering/renderer/printer/XHTMLWikiPrinterTest.java
@@ -17,15 +17,25 @@
  * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
  * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
  */
 package org.xwiki.rendering.renderer.printer;
 
+import java.util.Map;
+
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.CsvSource;
+import org.mockito.InOrder;
+import org.mockito.Mockito;
+import org.xml.sax.helpers.AttributesImpl;
+import org.xwiki.xml.html.HTMLElementSanitizer;
 
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.atLeast;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.when;
 
 /**
  * Unit tests for {@link XHTMLWikiPrinter}.
  *
  * @version $Id$
@@ -44,6 +54,77 @@ void testRawEscaping(String input, String expected)
         WikiPrinter mockPrinter = mock(WikiPrinter.class);
         XHTMLWikiPrinter xhtmlWikiPrinter = new XHTMLWikiPrinter(mockPrinter);
         xhtmlWikiPrinter.printRaw(input);
         verify(mockPrinter).print(expected);
     }
+
+    @ParameterizedTest
+    @CsvSource({
+        "invalid, value, data-xwiki-translated-attribute-invalid, value",
+        "valid, test&, valid, test&amp;",
+        "in/valid, value, data-xwiki-translated-attribute-invalid, value"
+    })
+    void testParameterCleaning(String parameterName, String parameterValue, String expectedName, String expectedValue)
+    {
+        HTMLElementSanitizer mockSanitizer = mock(HTMLElementSanitizer.class);
+        when(mockSanitizer.isElementAllowed(anyString())).thenReturn(true);
+        when(mockSanitizer.isAttributeAllowed(anyString(), anyString(), anyString())).then(invocation ->
+        {
+            String attributeName = invocation.getArgument(1, String.class);
+            return "valid".equals(attributeName) || attributeName.startsWith("data-");
+        });
+
+        // Test all possibilities of invoking the printer (with different kinds of arguments).
+        WikiPrinter mockPrinter = mock(WikiPrinter.class);
+        XHTMLWikiPrinter xhtmlWikiPrinter = new XHTMLWikiPrinter(mockPrinter, mockSanitizer);
+        Map<String, String> mapParameters = Map.of(parameterName, parameterValue);
+        xhtmlWikiPrinter.printXMLStartElement("div", mapParameters);
+        verify(mockSanitizer, atLeast(1)).isElementAllowed("div");
+        verify(mockSanitizer, atLeast(1)).isAttributeAllowed("div", parameterName, parameterValue);
+        verify(mockSanitizer, atLeast(1)).isAttributeAllowed("div", expectedName, parameterValue);
+        verifyPrinting(mockPrinter, expectedName, expectedValue, true);
+
+        mockPrinter = mock(WikiPrinter.class);
+        xhtmlWikiPrinter = new XHTMLWikiPrinter(mockPrinter, mockSanitizer);
+        xhtmlWikiPrinter.printXMLElement("div", mapParameters);
+        verifyPrinting(mockPrinter, expectedName, expectedValue, false);
+
+        mockPrinter = mock(WikiPrinter.class);
+        xhtmlWikiPrinter = new XHTMLWikiPrinter(mockPrinter, mockSanitizer);
+        String[][] arrayParameters = { { parameterName, parameterValue } };
+        xhtmlWikiPrinter.printXMLStartElement("div", arrayParameters);
+        verifyPrinting(mockPrinter, expectedName, expectedValue, true);
+
+        mockPrinter = mock(WikiPrinter.class);
+        xhtmlWikiPrinter = new XHTMLWikiPrinter(mockPrinter, mockSanitizer);
+        xhtmlWikiPrinter.printXMLElement("div", arrayParameters);
+        verifyPrinting(mockPrinter, expectedName, expectedValue, false);
+
+        mockPrinter = mock(WikiPrinter.class);
+        xhtmlWikiPrinter = new XHTMLWikiPrinter(mockPrinter, mockSanitizer);
+        AttributesImpl attributes = new AttributesImpl();
+        attributes.addAttribute(null, null, parameterName, null, parameterValue);
+        xhtmlWikiPrinter.printXMLStartElement("div", attributes);
+        verifyPrinting(mockPrinter, expectedName, expectedValue, true);
+    }
+
+    private void verifyPrinting(WikiPrinter mockPrinter, String attributeName,
+        String attributeValue, boolean isStart)
+    {
+        InOrder inOrder = Mockito.inOrder(mockPrinter);
+        inOrder.verify(mockPrinter).print("<");
+        inOrder.verify(mockPrinter).print("div");
+        inOrder.verify(mockPrinter).print(" ");
+        inOrder.verify(mockPrinter).print(attributeName);
+        inOrder.verify(mockPrinter).print("=");
+        inOrder.verify(mockPrinter).print("\"");
+        inOrder.verify(mockPrinter).print(attributeValue);
+        inOrder.verify(mockPrinter).print("\"");
+        if (isStart) {
+            inOrder.verify(mockPrinter).print(">");
+        } else {
+            inOrder.verify(mockPrinter).print("/>");
+        }
+        verifyNoMoreInteractions(mockPrinter);
+
+    }
 }

Vulnerable code:                     cleanAttributes.put(TRANSLATED_ATTRIBUTE_PREFIX + e.getKey(), e.getValue());
                        return new String[] { TRANSLATED_ATTRIBUTE_PREFIX + entry[0], entry[1] };
                    ((AttributesImpl) allowedAttribute).addAttribute(null, null,
                        TRANSLATED_ATTRIBUTE_PREFIX + attributes.getQName(i), null, attributes.getValue(i));
Fixed code: import java.util.Objects;
import java.util.regex.Pattern;
    /**
     * Pattern for matching characters not allowed in data attributes.
     * <p>
     * This is the inverse of the definition of a name being
     * <a href="https://html.spec.whatwg.org/multipage/infrastructure.html#xml-compatible>XML-compatible</a>,
     * i.e., matching the <a href="https://www.w3.org/TR/xml/#NT-Name">Name production</a> without ":".
     */
    private static final Pattern DATA_REPLACEMENT_PATTERN = Pattern.compile("[^A-Z_a-z\\u00C0-\\u00D6\\u00D8-\\u00F6"
        + "\\u00F8-\\u02ff\\u0370-\\u037d\\u037f-\\u1fff\\u200c\\u200d\\u2070-\\u218f\\u2c00-\\u2fef\\u3001-\\ud7ff"
        + "\\uf900-\\ufdcf\\ufdf0-\\ufffd\\x{10000}-\\x{EFFFF}\\-.0-9\\u00b7\\u0300-\\u036f\\u203f-\\u2040]");

                    // Keep but clean invalid attributes with a prefix (removed during parsing) to avoid loosing them
                    // through WYSIWYG editing.
                    String translatedName =
                        TRANSLATED_ATTRIBUTE_PREFIX + removeInvalidDataAttributeCharacters(e.getKey());
                    if (this.htmlElementSanitizer.isAttributeAllowed(elementName, translatedName, e.getValue())) {
                        cleanAttributes.put(translatedName, e.getValue());
                    }
                        // Keep but clean invalid attributes with a prefix (removed during parsing) to avoid loosing
                        // them through WYSIWYG editing.
                        String translatedName =
                            TRANSLATED_ATTRIBUTE_PREFIX + removeInvalidDataAttributeCharacters(entry[0]);
                        if (this.htmlElementSanitizer.isAttributeAllowed(elementName, translatedName, entry[1])) {
                            return new String[] { translatedName, entry[1] };
                        } else {
                            return null;
                        }
                .filter(Objects::nonNull)
                    // Keep but clean invalid attributes with a prefix (removed during parsing) to avoid loosing them
                    // through WYSIWYG editing.
                    String translatedName =
                        TRANSLATED_ATTRIBUTE_PREFIX + removeInvalidDataAttributeCharacters(attributes.getQName(i));
                    if (this.htmlElementSanitizer.isAttributeAllowed(elementName, translatedName,
                        attributes.getValue(i)))
                    {
                        ((AttributesImpl) allowedAttribute).addAttribute(null, null,
                            translatedName, null, attributes.getValue(i));
                    }
    /**
     * Strips out invalid characters from names used for data attributes.
     *
     * @param name the data attribute name to clean
     * @return valid name, to be prefixed with data-
     */
    public static String removeInvalidDataAttributeCharacters(String name)
    {
        return DATA_REPLACEMENT_PATTERN.matcher(name).replaceAll("");
    }

import java.util.Map;

import org.mockito.InOrder;
import org.mockito.Mockito;
import org.xml.sax.helpers.AttributesImpl;
import org.xwiki.xml.html.HTMLElementSanitizer;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.atLeast;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

    @ParameterizedTest
    @CsvSource({
        "invalid, value, data-xwiki-translated-attribute-invalid, value",
        "valid, test&, valid, test&amp;",
        "in/valid, value, data-xwiki-translated-attribute-invalid, value"
    })
    void testParameterCleaning(String parameterName, String parameterValue, String expectedName, String expectedValue)
    {
        HTMLElementSanitizer mockSanitizer = mock(HTMLElementSanitizer.class);
        when(mockSanitizer.isElementAllowed(anyString())).thenReturn(true);
        when(mockSanitizer.isAttributeAllowed(anyString(), anyString(), anyString())).then(invocation ->
        {
            String attributeName = invocation.getArgument(1, String.class);
            return "valid".equals(attributeName) || attributeName.startsWith("data-");
        });

        // Test all possibilities of invoking the printer (with different kinds of arguments).
        WikiPrinter mockPrinter = mock(WikiPrinter.class);
        XHTMLWikiPrinter xhtmlWikiPrinter = new XHTMLWikiPrinter(mockPrinter, mockSanitizer);
        Map<String, String> mapParameters = Map.of(parameterName, parameterValue);
        xhtmlWikiPrinter.printXMLStartElement("div", mapParameters);
        verify(mockSanitizer, atLeast(1)).isElementAllowed("div");
        verify(mockSanitizer, atLeast(1)).isAttributeAllowed("div", parameterName, parameterValue);
        verify(mockSanitizer, atLeast(1)).isAttributeAllowed("div", expectedName, parameterValue);
        verifyPrinting(mockPrinter, expectedName, expectedValue, true);

        mockPrinter = mock(WikiPrinter.class);
        xhtmlWikiPrinter = new XHTMLWikiPrinter(mockPrinter, mockSanitizer);
        xhtmlWikiPrinter.printXMLElement("div", mapParameters);
        verifyPrinting(mockPrinter, expectedName, expectedValue, false);

        mockPrinter = mock(WikiPrinter.class);
        xhtmlWikiPrinter = new XHTMLWikiPrinter(mockPrinter, mockSanitizer);
        String[][] arrayParameters = { { parameterName, parameterValue } };
        xhtmlWikiPrinter.printXMLStartElement("div", arrayParameters);
        verifyPrinting(mockPrinter, expectedName, expectedValue, true);

        mockPrinter = mock(WikiPrinter.class);
        xhtmlWikiPrinter = new XHTMLWikiPrinter(mockPrinter, mockSanitizer);
        xhtmlWikiPrinter.printXMLElement("div", arrayParameters);
        verifyPrinting(mockPrinter, expectedName, expectedValue, false);

        mockPrinter = mock(WikiPrinter.class);
        xhtmlWikiPrinter = new XHTMLWikiPrinter(mockPrinter, mockSanitizer);
        AttributesImpl attributes = new AttributesImpl();
        attributes.addAttribute(null, null, parameterName, null, parameterValue);
        xhtmlWikiPrinter.printXMLStartElement("div", attributes);
        verifyPrinting(mockPrinter, expectedName, expectedValue, true);
    }

    private void verifyPrinting(WikiPrinter mockPrinter, String attributeName,
        String attributeValue, boolean isStart)
    {
        InOrder inOrder = Mockito.inOrder(mockPrinter);
        inOrder.verify(mockPrinter).print("<");
        inOrder.verify(mockPrinter).print("div");
        inOrder.verify(mockPrinter).print(" ");
        inOrder.verify(mockPrinter).print(attributeName);
        inOrder.verify(mockPrinter).print("=");
        inOrder.verify(mockPrinter).print("\"");
        inOrder.verify(mockPrinter).print(attributeValue);
        inOrder.verify(mockPrinter).print("\"");
        if (isStart) {
            inOrder.verify(mockPrinter).print(">");
        } else {
            inOrder.verify(mockPrinter).print("/>");
        }
        verifyNoMoreInteractions(mockPrinter);

    }