Support multiple children handling on style tags

nahsra · Mar 27, 2022 · 0199e7e · 0199e7e
1 parent 578d1f9
commit 0199e7e
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 9 deletions.
diff --git a/src/main/java/org/owasp/validator/html/scan/AntiSamyDOMScanner.java b/src/main/java/org/owasp/validator/html/scan/AntiSamyDOMScanner.java
@@ -407,10 +407,17 @@ private boolean processStyleTag(Element ele, Node parentNode) {
         CssScanner styleScanner = new CssScanner(policy, messages, policy.isEmbedStyleSheets());
 
         try {
-            Node firstChild = ele.getFirstChild();
-            if (firstChild != null) {
+            if (ele.getChildNodes().getLength() > 0) {
+                String toScan = "";
+
+                for (int i = 0; i < ele.getChildNodes().getLength(); i++) {
+                    Node childNode = ele.getChildNodes().item(i);
+                    if (!toScan.isEmpty()){
+                        toScan += "\n";
+                    }
+                    toScan += childNode.getTextContent();
+                }
 
-                String toScan = firstChild.getNodeValue();
                 CleanResults cr = styleScanner.scanStyleSheet(toScan, policy.getMaxInputSize());
                 errorMessages.addAll(cr.getErrorMessages());
 
@@ -422,12 +429,17 @@ private boolean processStyleTag(Element ele, Node parentNode) {
                  * break all CSS. To prevent that, we have this check.
                  */
 
-                final String cleanHTML = cr.getCleanHTML();
+                String cleanHTML = cr.getCleanHTML();
+                cleanHTML = cleanHTML == null || cleanHTML.equals("") ? "/* */" : cleanHTML;
 
-                if (cleanHTML == null || cleanHTML.equals("")) {
-                    firstChild.setNodeValue("/* */");
-                } else {
-                    firstChild.setNodeValue(cleanHTML);
+                ele.getFirstChild().setNodeValue(cleanHTML);
+                /*
+                 * Remove every other node after cleaning CSS, there will
+                 * be only one node in the end, as it always should have.
+                 */
+                for (int i = 1; i < ele.getChildNodes().getLength(); i++) {
+                    Node childNode = ele.getChildNodes().item(i);
+                    ele.removeChild(childNode);
                 }
             }
 

diff --git a/src/test/java/org/owasp/validator/html/test/AntiSamyTest.java b/src/test/java/org/owasp/validator/html/test/AntiSamyTest.java
@@ -1702,5 +1702,19 @@ public void testGithubIssue151() throws ScanException, PolicyException {
         assertThat(result.getErrorMessages().size(), is(1));
         assertThat(result.getCleanHTML(), both(containsString("img")).and(not(containsString("CURSOR"))));
     }
+
+    @Test
+    public void testSmuggledTagsInStyleContent() 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.
+
+        Policy revised = policy.cloneWithDirective(Policy.USE_XHTML,"true");
+        assertThat(as.scan("<style/>b<![cdata[</style><a href=javascript:alert(1)>test", revised, AntiSamy.DOM).getCleanHTML(), not(containsString("javascript")));
+        assertThat(as.scan("<style/>b<![cdata[</style><a href=javascript:alert(1)>test", revised, AntiSamy.SAX).getCleanHTML(), not(containsString("javascript")));
+
+        Policy revised2 = policy.cloneWithDirective(Policy.USE_XHTML,"false");
+        assertThat(as.scan("<select<style/>W<xmp<script>alert(1)</script>", revised2, AntiSamy.DOM).getCleanHTML(), not(containsString("script")));
+        assertThat(as.scan("<select<style/>W<xmp<script>alert(1)</script>", revised2, AntiSamy.SAX).getCleanHTML(), not(containsString("script")));
+    }
 }
 
diff --git a/src/test/java/org/owasp/validator/html/test/TestPolicy.java b/src/test/java/org/owasp/validator/html/test/TestPolicy.java
@@ -43,7 +43,7 @@
  */
 public class TestPolicy extends InternalPolicy {
 
-    protected TestPolicy(Policy.ParseContext parseContext) throws PolicyException {
+    protected TestPolicy(Policy.ParseContext parseContext) {
         super(parseContext);
     }