XWiki vulnerable to remote code execution through insufficient protection against {{/html}} injection

@@ -224,7 +224,9 @@ public class XWikiDocument implements DocumentModelBridge, Cloneable, Disposable
 
     private static final String TM_FAILEDDOCUMENTPARSE = "core.document.error.failedParse";
 
-    private static final String CLOSE_HTML_MACRO = "{{/html}}";
+    private static final String[] HTML_MACRO_SEARCH_STRINGS = new String[] { "{{html", "{{/html" };
+
+    private static final String[] HTML_MACRO_REPLACE_STRINGS = new String[] { "{{html", "{{/html" };
 
     /**
      * An attachment waiting to be deleted at next document save.
@@ -4058,14 +4060,13 @@ public String display(String fieldname, String type, String pref, BaseObject obj
             // macro syntax since it's not needed for pure text
             if (isInRenderingEngine && !is10Syntax(wrappingSyntaxId)
                 && (HTMLUtils.containsElementText(result) || result.indexOf("{") != -1)) {
-                result.insert(0, "{{html clean=\"false\" wiki=\"false\"}}");
-                // Escape closing HTML macro syntax.
-                int startIndex = 0;
-                // Start searching at the last match to avoid scanning the whole string again.
-                while ((startIndex = result.indexOf(CLOSE_HTML_MACRO, startIndex)) != -1) {
-                    result.replace(startIndex, startIndex + 2, "{{");
-                }
-                result.append(CLOSE_HTML_MACRO);
+                // Escapes closing and opening HTML macro syntax. We need to escape the opening HTML macro syntax in
+                // addition to the closing one as otherwise, the wrapping HTML macro might not close correctly.
+                // For simplicity, to avoid having to deal with complex expressions that would need to be
+                // synchronized with the parser, match just the start of the opening/closing macro syntax.
+                return "{{html clean=\"false\" wiki=\"false\"}}"
+                    + StringUtils.replaceEach(result.toString(), HTML_MACRO_SEARCH_STRINGS, HTML_MACRO_REPLACE_STRINGS)
+                    + "{{/html}}";
             }
 
             return result.toString();

@@ -672,12 +672,13 @@ void displayEscapesClosingHTMLMacro()
         PropertyClass propertyInterface = mock(PropertyClass.class);
         when(xClass.get("mock")).thenReturn(propertyInterface);
         doAnswer(call -> {
-            call.getArgument(0, StringBuffer.class).append("{{/html}}content{{/html}}");
+            call.getArgument(0, StringBuffer.class).append("{{/html }}content{{/html}}{{html wiki=\"true\"}}");
             return null;
         }).when(propertyInterface).displayView(any(StringBuffer.class), eq("mock"), any(String.class), eq(object),
             anyBoolean(), any(XWikiContext.class));
 
-        assertEquals("{{html clean=\"false\" wiki=\"false\"}}{{/html}}content{{/html}}{{/html}}",
+        assertEquals("{{html clean=\"false\" wiki=\"false\"}}"
+                + "{{/html }}content{{/html}}{{html wiki=\"true\"}}{{/html}}",
             this.document.display("mock", "view", object, this.oldcore.getXWikiContext()));
     }
 

@@ -19,6 +19,7 @@
  */
 package org.xwiki.rendering.renderer.printer;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -59,6 +60,16 @@ public class XHTMLWikiPrinter extends XMLWikiPrinter
         + "\\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]");
 
+    // Precomputed forbidden suffixes/prefixes to prevent the injection of opening or closing HTML macro syntaxes at
+    // the end of raw content.
+    private static final List<String> FORBIDDEN_RAW_SUFFIX_PREFIXES = computeForbiddenRawSuffixPrefixes();
+
+    // Forbidden substrings to prevent the injection of opening and closing HTML macro syntaxes in raw content to
+    // ensure that rendering output can be used safely in HTML macros.
+    private static final String[] FORBIDDEN_RAW_STRINGS = new String[] { "{{html", "{{/html" };
+
+    private static final String[] FORBIDDEN_RAW_REPLACEMENTS = new String[] { "&#123;&#123;html", "&#123;&#123;/html" };
+
     /**
      * The sanitizer used to restrict allowed elements and attributes, can be null (no restrictions).
      *
@@ -230,17 +241,17 @@ public void printSpace()
     public void printRaw(String raw)
     {
         handleSpaceWhenStartElement();
-        // Prevent injecting {{/html}}. We escape {{/html}} as well as prefixes of {{/html}} at the end of the raw
-        // content to avoid that raw content and plain texts can be combined to construct the full {{/html}}. This may
-        // cause errors as we might not be using the right escaping for the context (e.g., JSON or HTML comments) but
-        // for this reason we also escape in JSON output and HTML comments.
-        String escapedRaw = raw.replace("{{/html}}", "&#123;&#123;/html}}");
-
-        StringBuilder prefix = new StringBuilder();
-        for (Character nextChar : List.of('{', '/', 'h', 't', 'm', 'l', '}', '}')) {
-            prefix.append(nextChar);
-
-            if (escapedRaw.endsWith(prefix.toString())) {
+        // Prevent injecting {{/html}}. As there can be an arbitrary number of spaces before the }}, we actually
+        // escape {{/html. We escape {{/html as well as prefixes of {/html and {html at the end of the raw content to
+        // avoid that raw content and plain texts can be combined to construct the full {{/html}} or {{html}}. This may
+        // cause errors as we might not be using the right escaping for the context (e.g., JSON or HTML comments), but
+        // for this reason we also escape { in JSON output and HTML comments.
+        String escapedRaw = StringUtils.replaceEach(raw, FORBIDDEN_RAW_STRINGS, FORBIDDEN_RAW_REPLACEMENTS);
+
+        // Check all prefixes, they are pre-computed to ensure that this code is as efficient as possible in
+        // particular in the very likely case that no such suffix actually exists.
+        for (String prefix : FORBIDDEN_RAW_SUFFIX_PREFIXES) {
+            if (escapedRaw.endsWith(prefix)) {
                 escapedRaw =
                     escapedRaw.substring(0, escapedRaw.length() - prefix.length()) + "&#123;" + prefix.substring(1);
                 break;
@@ -250,6 +261,19 @@ public void printRaw(String raw)
         this.elementEnded = true;
     }
 
+    private static List<String> computeForbiddenRawSuffixPrefixes()
+    {
+        List<String> forbidden = new ArrayList<>(12);
+        // Add the common { prefix separately such that we won't add it twice in the loop below.
+        forbidden.add("{");
+        for (String suffix : List.of("{/html", "{html")) {
+            for (int i = 2; i <= suffix.length(); i++) {
+                forbidden.add(suffix.substring(0, i));
+            }
+        }
+        return forbidden;
+    }
+
     private void handleSpaceWhenInText()
     {
         if (this.elementEnded || this.hasTextBeenPrinted) {

@@ -45,9 +45,11 @@ class XHTMLWikiPrinterTest
     @ParameterizedTest
     @CsvSource({
         "Closing the {{/html}} macro., Closing the {{/html}} macro.",
+        "{{html clean=\"false\"}} {{/html }}, {{html clean=\"false\"}} {{/html }}",
         "Starting a macro {, Starting a macro {",
+        "Partial open: {{h, Partial open: {{h",
         "Partial: {{/h, Partial: {{/h",
-        "{{html}}, {{html}}"
+        "{{html}}, {{html}}"
     })
     void testRawEscaping(String input, String expected)
     {