Fix child node removal on style tag processing

nahsra · Apr 9, 2022 · 32e2735 · 32e2735
1 parent b6e76de
commit 32e2735
Show file tree

Hide file tree

Showing 2 changed files with 8 additions and 6 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,7 +407,8 @@ private boolean processStyleTag(Element ele, Node parentNode) {
         CssScanner styleScanner = new CssScanner(policy, messages, policy.isEmbedStyleSheets());
 
         try {
-            if (ele.getChildNodes().getLength() > 0) {
+            int childNodesCount = ele.getChildNodes().getLength();
+            if (childNodesCount > 0) {
                 StringBuffer toScan = new StringBuffer();
 
                 for (int i = 0; i < ele.getChildNodes().getLength(); i++) {
@@ -428,29 +429,26 @@ private boolean processStyleTag(Element ele, Node parentNode) {
                  * would normally be left with an empty style tag and
                  * break all CSS. To prevent that, we have this check.
                  */
-
                 String cleanHTML = cr.getCleanHTML();
                 cleanHTML = cleanHTML == null || cleanHTML.equals("") ? "/* */" : 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.
+                 * Starting from the end due to list updating on the fly.
                  */
-                for (int i = 1; i < ele.getChildNodes().getLength(); i++) {
+                for (int i = childNodesCount - 1; i >= 1; i--) {
                     Node childNode = ele.getChildNodes().item(i);
                     ele.removeChild(childNode);
                 }
             }
-
         } catch (DOMException | ScanException | ParseException | NumberFormatException e) {
-
             /*
              * ParseException shouldn't be possible anymore, but we'll leave it
              * here because I (Arshan) am hilariously dumb sometimes.
              * Batik can throw NumberFormatExceptions (see bug #48).
              */
-
             addError(ErrorMessageUtil.ERROR_CSS_TAG_MALFORMED, new Object[]{HTMLEntityEncoder.htmlEntityEncode(ele.getFirstChild().getNodeValue())});
             parentNode.removeChild(ele);
             return true;

diff --git a/src/test/java/org/owasp/validator/html/test/AntiSamyTest.java b/src/test/java/org/owasp/validator/html/test/AntiSamyTest.java
@@ -1713,10 +1713,14 @@ public void testSmuggledTagsInStyleContent() throws ScanException, PolicyExcepti
         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")));
+        assertThat(as.scan("<select<style/>k<input<</>input/onfocus=alert(1)>", revised, AntiSamy.DOM).getCleanHTML(), not(containsString("input")));
+        assertThat(as.scan("<select<style/>k<input<</>input/onfocus=alert(1)>", revised, AntiSamy.SAX).getCleanHTML(), not(containsString("input")));
 
         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")));
+        assertThat(as.scan("<select<style/>k<input<</>input/onfocus=alert(1)>", revised2, AntiSamy.DOM).getCleanHTML(), not(containsString("input")));
+        assertThat(as.scan("<select<style/>k<input<</>input/onfocus=alert(1)>", revised2, AntiSamy.SAX).getCleanHTML(), not(containsString("input")));
     }
 
     @Test(timeout = 3000)