Merge pull request #353 from nahsra/style-processing-fix

Style processing fix

src/main/java/org/owasp/validator/html/InternalPolicy.java

@@ -43,7 +43,7 @@ protected InternalPolicy(ParseContext parseContext) {
     this.entityEncodeIntlCharacters = isTrue(Policy.ENTITY_ENCODE_INTL_CHARS);
     this.embedTag = getTagByLowercaseName("embed");
     this.onUnknownTag = getDirective("onUnknownTag");
-    this.isEncodeUnknownTag = "encode".equals(onUnknownTag);
+    this.isEncodeUnknownTag = Policy.ACTION_ENCODE.equals(onUnknownTag);
     this.preserveComments = isTrue(Policy.PRESERVE_COMMENTS);
     this.styleTag = getTagByLowercaseName("style");
     this.embedStyleSheets = isTrue(Policy.EMBED_STYLESHEETS);
@@ -74,7 +74,7 @@ protected InternalPolicy(
     this.entityEncodeIntlCharacters = isTrue(Policy.ENTITY_ENCODE_INTL_CHARS);
     this.embedTag = getTagByLowercaseName("embed");
     this.onUnknownTag = getDirective("onUnknownTag");
-    this.isEncodeUnknownTag = "encode".equals(onUnknownTag);
+    this.isEncodeUnknownTag = Policy.ACTION_ENCODE.equals(onUnknownTag);
     this.preserveComments = isTrue(Policy.PRESERVE_COMMENTS);
     this.styleTag = getTagByLowercaseName("style");
     this.embedStyleSheets = isTrue(Policy.EMBED_STYLESHEETS);

src/main/java/org/owasp/validator/html/Policy.java

@@ -136,6 +136,7 @@ public class Policy {
   public static final String ACTION_VALIDATE = "validate";
   public static final String ACTION_FILTER = "filter";
   public static final String ACTION_TRUNCATE = "truncate";
+  public static final String ACTION_ENCODE = "encode";
 
   private final Map<String, AntiSamyPattern> commonRegularExpressions;
   protected final Map<String, Tag> tagRules;

src/main/java/org/owasp/validator/html/scan/AntiSamyDOMScanner.java

@@ -288,7 +288,7 @@ private void recursiveValidateTag(final Node node, int currentStackDepth) throws
     }
 
     if ((tagRule == null && policy.isEncodeUnknownTag())
-        || (tagRule != null && tagRule.isAction("encode"))) {
+        || (tagRule != null && tagRule.isAction(Policy.ACTION_ENCODE))) {
       encodeTag(currentStackDepth, ele, tagName, eleChildNodes);
     } else if (tagRule == null || tagRule.isAction(Policy.ACTION_FILTER)) {
       actionFilter(currentStackDepth, ele, tagName, tagRule, eleChildNodes);
@@ -473,7 +473,17 @@ private boolean processStyleTag(Element ele, Node parentNode) {
         String cleanHTML = cr.getCleanHTML();
         cleanHTML = cleanHTML == null || cleanHTML.equals("") ? "/* */" : cleanHTML;
 
-        ele.getFirstChild().setNodeValue(cleanHTML);
+        /*
+         * First node present may not be Text and setNodeValue will not work.
+         * If not a Text node, create one at the beginning before removal of the rest, if any.
+         */
+        Node firstChild = ele.getFirstChild();
+        if (firstChild instanceof Text) firstChild.setNodeValue(cleanHTML);
+        else {
+          ele.insertBefore(document.createTextNode(cleanHTML), firstChild);
+          childNodesCount++;
+        }
+
         /*
          * Remove every other node after cleaning CSS, there will
          * be only one node in the end, as it always should have.

src/main/java/org/owasp/validator/html/scan/MagicSAXFilter.java

@@ -273,7 +273,7 @@ public void startElement(QName element, XMLAttributes attributes, Augmentations
       // we also remove all child elements of a style element
       this.operations.push(Ops.REMOVE);
     } else if ((tag == null && policy.isEncodeUnknownTag())
-        || (tag != null && tag.isAction("encode"))) {
+        || (tag != null && tag.isAction(Policy.ACTION_ENCODE))) {
       String name = "<" + element.localpart + ">";
       super.characters(new XMLString(name.toCharArray(), 0, name.length()), augs);
       this.operations.push(Ops.ENCODE);
@@ -282,7 +282,7 @@ public void startElement(QName element, XMLAttributes attributes, Augmentations
           ErrorMessageUtil.ERROR_TAG_NOT_IN_POLICY,
           new Object[] {HTMLEntityEncoder.htmlEntityEncode(element.localpart)});
       this.operations.push(Ops.FILTER);
-    } else if (tag.isAction("filter")) {
+    } else if (tag.isAction(Policy.ACTION_FILTER)) {
       addError(
           ErrorMessageUtil.ERROR_TAG_FILTERED,
           new Object[] {HTMLEntityEncoder.htmlEntityEncode(element.localpart)});
@@ -440,7 +440,7 @@ public void startElement(QName element, XMLAttributes attributes, Augmentations
         this.operations.push(Ops.KEEP);
       }
 
-    } else if (tag.isAction("truncate")) {
+    } else if (tag.isAction(Policy.ACTION_TRUNCATE)) {
       this.operations.push(Ops.TRUNCATE);
     } else {
       // no options left, so the tag will be removed

src/test/java/org/owasp/validator/html/test/AntiSamyTest.java

@@ -946,7 +946,7 @@ public void issue30() throws ScanException, PolicyException {
   public void issue31() throws ScanException, PolicyException {
 
     String test = "<b><u><g>foo</g></u></b>";
-    Policy revised = policy.cloneWithDirective("onUnknownTag", "encode");
+    Policy revised = policy.cloneWithDirective("onUnknownTag", Policy.ACTION_ENCODE);
     CleanResults cr = as.scan(test, revised, AntiSamy.DOM);
     String s = cr.getCleanHTML();
     assertFalse(!s.contains("&lt;g&gt;"));
@@ -955,7 +955,7 @@ public void issue31() throws ScanException, PolicyException {
     assertFalse(!s.contains("&lt;g&gt;"));
     assertFalse(!s.contains("&lt;/g&gt;"));
 
-    Tag tag = policy.getTagByLowercaseName("b").mutateAction("encode");
+    Tag tag = policy.getTagByLowercaseName("b").mutateAction(Policy.ACTION_ENCODE);
     Policy policy1 = policy.mutateTag(tag);
 
     cr = as.scan(test, policy1, AntiSamy.DOM);
@@ -2519,7 +2519,7 @@ public void testGithubIssue151() throws ScanException, PolicyException {
   }
 
   @Test
-  public void testSmuggledTagsInStyleContent() throws ScanException, PolicyException {
+  public void testSmuggledTagsInStyleContentCase1() throws ScanException, PolicyException {
     // HTML tags may be smuggled into a style tag after parsing input to an internal representation.
     // If that happens, they should be treated as text content and not as children nodes.
     assertThat(
@@ -2585,4 +2585,18 @@ public void testQuotesInsideStyles() throws ScanException, PolicyException {
         as.scan(input, policy, AntiSamy.SAX).getCleanHTML(),
         both(containsString("'comic sans ms'")).and(containsString("\"font-family")));
   }
+
+  @Test
+  public void testSmuggledTagsInStyleContentCase2() throws ScanException, PolicyException {
+    // Style tag processing was not handling correctly the value set to its child node that should
+    // be only text. On some mutation scenarios due to filtering tags by default, content was being
+    // smuggled and not properly sanitized by the output serializer.
+    String input = "<style/><listing/>]]><noembed></style><img src=x onerror=mxss(1)></noembed>";
+    assertThat(as.scan(input, policy, AntiSamy.DOM).getCleanHTML(), not(containsString("mxss")));
+    assertThat(as.scan(input, policy, AntiSamy.SAX).getCleanHTML(), not(containsString("mxss")));
+
+    input = "<style/><math>'<noframes ></style><img src=x onerror=mxss(1)></noframes>'";
+    assertThat(as.scan(input, policy, AntiSamy.DOM).getCleanHTML(), not(containsString("mxss")));
+    assertThat(as.scan(input, policy, AntiSamy.SAX).getCleanHTML(), not(containsString("mxss")));
+  }
 }