From 14f84fd49ebe68918b3aa0944a6a658674a39e6b Mon Sep 17 00:00:00 2001
From: Mike Samuel <mikesamuel@gmail.com>
Date: Mon, 18 Oct 2021 15:35:14 -0400
Subject: [PATCH] Recognize that `<style>` is not really workable inside
 `<select>`

Rather than mucking with `<style>` tag content in all cases, this is a more
tailored fix to the recent vulnerability that just closes `<style>` elements
when we realize they're in a dodgy parsing context.
---
 .../org/owasp/html/HtmlStreamRenderer.java    | 21 ----------
 .../TagBalancingHtmlStreamEventReceiver.java  | 20 ++++++++++
 .../java/org/owasp/html/SanitizersTest.java   | 39 ++++++++++++-------
 .../TagBalancingHtmlStreamRendererTest.java   |  6 +--
 4 files changed, 47 insertions(+), 39 deletions(-)

diff --git a/src/main/java/org/owasp/html/HtmlStreamRenderer.java b/src/main/java/org/owasp/html/HtmlStreamRenderer.java
index 23d94d06..9c079d1c 100644
--- a/src/main/java/org/owasp/html/HtmlStreamRenderer.java
+++ b/src/main/java/org/owasp/html/HtmlStreamRenderer.java
@@ -254,25 +254,8 @@ private final void writeCloseTag(String uncanonElementName)
         Encoding.stripBannedCodeunits(cdataContent);
         int problemIndex = checkHtmlCdataCloseable(lastTagOpened, cdataContent);
         if (problemIndex == -1) {
-          String prefix = "";
-          String suffix = "";
-          Set<String> bannedSubstrings = Collections.emptySet();
-          if ("style".equals(elementName)) {
-            prefix = "/*<![CDATA[<!--*/\n";
-            suffix = "\n/*-->]]>*/";
-            bannedSubstrings = BANNED_IN_STYLE_ELEMENTS;
-          }
-
-          for (String bannedSubstring : bannedSubstrings) {
-            if (cdataContent.indexOf(bannedSubstring) >= 0) {
-              cdataContent.setLength(0);
-            }
-          }
-
           if (cdataContent.length() != 0) {
-            output.append(prefix);
             output.append(cdataContent);
-            output.append(suffix);
           }
         } else {
           error(
@@ -457,8 +440,4 @@ public void close() throws IOException {
   private static boolean isTagEnd(char ch) {
     return ch < 63 && 0 != (TAG_ENDS & (1L << ch));
   }
-
-  private static Set<String> BANNED_IN_STYLE_ELEMENTS = ImmutableSet.of(
-    "<![CDATA[", "]]>", "<!--", "-->"
-  );
 }
diff --git a/src/main/java/org/owasp/html/TagBalancingHtmlStreamEventReceiver.java b/src/main/java/org/owasp/html/TagBalancingHtmlStreamEventReceiver.java
index aa2eb075..6df2a286 100644
--- a/src/main/java/org/owasp/html/TagBalancingHtmlStreamEventReceiver.java
+++ b/src/main/java/org/owasp/html/TagBalancingHtmlStreamEventReceiver.java
@@ -217,6 +217,15 @@ && canContain(elIndex, toResume, nOpen)) {
   private boolean canContain(
       int child, int container, int containerIndexOnStack) {
     Preconditions.checkArgument(containerIndexOnStack >= 0);
+    if (child == HtmlElementTables.TEXT_NODE && hasSpecialTextMode(container)) {
+      // If there's a select element on the stack, then we need to be extra careful.
+      int selectElementIndex = METADATA.indexForName("select");
+      for (int i = containerIndexOnStack; --i >= 0;) {
+        if (selectElementIndex == openElements.get(i)) {
+          return false;
+        }
+      }
+    }
     int anc = container;
     int ancIndexOnStack = containerIndexOnStack;
     while (true) {
@@ -363,6 +372,17 @@ private static boolean isHeaderElementName(String canonElementName) {
         && canonElementName.charAt(1) <= '9';
   }
 
+  private static boolean hasSpecialTextMode(int elementIndex) {
+    String name = METADATA.canonNameForIndex(elementIndex);
+    switch (HtmlTextEscapingMode.getModeForTag(name)) {
+      case PCDATA: case VOID:
+        return false;
+      case CDATA: case CDATA_SOMETIMES: case RCDATA: case PLAIN_TEXT:
+        return true;
+    }
+    throw new IllegalArgumentException(name);
+  }
+
   private static final byte ALL_SCOPES;
   private static final byte[] SCOPES_BY_ELEMENT;
   private static final byte[] SCOPE_FOR_END_TAG;
diff --git a/src/test/java/org/owasp/html/SanitizersTest.java b/src/test/java/org/owasp/html/SanitizersTest.java
index db522309..4cb7bbca 100644
--- a/src/test/java/org/owasp/html/SanitizersTest.java
+++ b/src/test/java/org/owasp/html/SanitizersTest.java
@@ -439,11 +439,7 @@ public static final void testStyleTagsInAllTheWrongPlaces() {
     String input = ""
       + "<select><option><style><script>alert(1)</script></style></option></select>"
       + "<svg><style>.r { color: red }</style></svg>"
-      + "<style>.b { color: blue }</style>"
-      + "<style>#a { content: \"<!--\" }</style>"
-      + "<style>#a { content: \"<![CDATA[\" }</style>"
-      + "<style>#a { content: \"-->\" }</style>"
-      + "<style>#a { content: \"]]>\" }</style>";
+      + "<style>.b { color: blue }</style>";
     PolicyFactory pf = new HtmlPolicyBuilder()
         .allowElements("option", "select", "style", "svg")
         .allowTextIn("style")
@@ -451,36 +447,49 @@ public static final void testStyleTagsInAllTheWrongPlaces() {
     assertEquals(
         ""
         + "<select><option>"
-        + "<style>/*<![CDATA[<!--*/\n<script>alert(1)</script>\n/*-->]]>*/</style>"
+        + "<style></style>&lt;script&gt;alert(1)&lt;/script&gt;"
         + "</option></select>"
         + "<svg>"
-        + "<style>/*<![CDATA[<!--*/\n.r { color: red }\n/*-->]]>*/</style>"
+        + "<style>.r { color: red }</style>"
         + "</svg>"
-        + "<style>/*<![CDATA[<!--*/\n.b { color: blue }\n/*-->]]>*/</style>"
-        + "<style></style>"
-        + "<style></style>"
-        + "<style></style>"
-        + "<style></style>",
+        + "<style>.b { color: blue }</style>",
         pf.sanitize(input)
     );
   }
 
   @Test
   public static final void testSelectIsOdd() {
+    // Special text modes interact badly with select and option
     String input = "<select><option><xmp><script>alert(1)</script></xmp></option></select>";
     PolicyFactory pf = new HtmlPolicyBuilder()
         .allowElements("option", "select", "xmp")
-        .allowTextIn("xmp")
+        .allowTextIn("xmp", "option")
         .toFactory();
     assertEquals(
         ""
-        + "<select><option>"
-        + "<pre>&lt;script&gt;alert(1)&lt;/script&gt;</pre>"
+        + "<select><option><pre></pre>"
+        + "&lt;script&gt;alert(1)&lt;/script&gt;"
         + "</option></select>",
         pf.sanitize(input)
     );
   }
 
+  @Test
+  public static final void testOptionAllowsText() {
+    String input = "<select><option><pre>code goes here</pre></option></select>";
+    PolicyFactory pf = new HtmlPolicyBuilder()
+            .allowElements("option", "select", "pre")
+            .allowTextIn("pre", "option")
+            .toFactory();
+    assertEquals(
+            ""
+                    + "<select><option>"
+                    + "<pre>code goes here</pre>"
+                    + "</option></select>",
+            pf.sanitize(input)
+    );
+  }
+
   @Test
   public static final void testStyleGlobally() {
     PolicyFactory policyBuilder = new HtmlPolicyBuilder()
diff --git a/src/test/java/org/owasp/html/TagBalancingHtmlStreamRendererTest.java b/src/test/java/org/owasp/html/TagBalancingHtmlStreamRendererTest.java
index 5195fde9..38a854ef 100644
--- a/src/test/java/org/owasp/html/TagBalancingHtmlStreamRendererTest.java
+++ b/src/test/java/org/owasp/html/TagBalancingHtmlStreamRendererTest.java
@@ -158,9 +158,9 @@ public final void testTextContent() {
         + "<p>Hello, <textarea>World!</textarea></p>"
         + "<h1>Hello"
         // Text allowed in special style tag.
-        + "<style type=\"text/css\">/*<![CDATA[<!--*/\n"
-        + "\n.World {\n  color: blue\n}\n"
-        + "\n/*-->]]>*/</style></h1>"
+        + "<style type=\"text/css\">\n"
+        + ".World {\n  color: blue\n}\n"
+        + "</style></h1>"
         // Whitespace allowed inside <ul> but non-whitespace text nodes are
         // moved inside <li>.
         + "<ul><li>Hello,</li><li>World!</li></ul>",