org.xwiki.rendering:xwiki-rendering-xml Improper Neutralization of Invalid Characters in Identifiers in Web Pages vulnerability

@@ -23,6 +23,8 @@
 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;
@@ -48,6 +50,17 @@ public class XHTMLWikiPrinter extends XMLWikiPrinter
     @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).
      *
@@ -260,7 +273,13 @@ private Map<String, String> cleanAttributes(String elementName, Map<String, Stri
                 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());
+                    }
                 }
             }
         }
@@ -279,9 +298,18 @@ private String[][] cleanAttributes(String elementName, String[][] attributes)
                     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);
         }
 
@@ -304,15 +332,34 @@ private Attributes cleanAttributes(String elementName, Attributes attributes)
                     ((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>...

@@ -19,11 +19,21 @@
  */
 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}.
@@ -46,4 +56,75 @@ void testRawEscaping(String input, String expected)
         xhtmlWikiPrinter.printRaw(input);
         verify(mockPrinter).print(expected);
     }
+
+    @ParameterizedTest
+    @CsvSource({
+        "invalid, value, data-xwiki-translated-attribute-invalid, value",
+        "valid, test&, valid, test&",
+        "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);
+
+    }
 }