Merge pull request #5612 from kingthorin/eval-fp

pscanrulesBeta: Adjust Dangerous JS Func handling for FP
zaproxy · Aug 1, 2024 · 2a2c827 · 2a2c827
2 parents e8f07e3 + fd020a3
commit 2a2c827
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 18 deletions.
diff --git a/addOns/pscanrulesBeta/CHANGELOG.md b/addOns/pscanrulesBeta/CHANGELOG.md
@@ -4,7 +4,8 @@ All notable changes to this add-on will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ## Unreleased
-
+### Fixed
+- A possible false positive condition with the Dangerous JS Functions scan rule with substrings in certain circumstances (Issue 8553).
 
 ## [40] - 2024-07-24
 ### Removed

diff --git a/...nrulesBeta/src/main/java/org/zaproxy/zap/extension/pscanrulesBeta/JsFunctionScanRule.java b/...nrulesBeta/src/main/java/org/zaproxy/zap/extension/pscanrulesBeta/JsFunctionScanRule.java
@@ -176,7 +176,7 @@ private void loadPayload() {
     private static void addPattern(String line, List<Pattern> list) {
         // Strip leading $, it's optionally included in the assembled patterns
         line = line.replace("$", "");
-        list.add(Pattern.compile("\\b\\$?" + line + "\\b", Pattern.CASE_INSENSITIVE));
+        list.add(Pattern.compile("\\b\\$?" + line + "\\s{0,5}\\(", Pattern.CASE_INSENSITIVE));
     }
 
     public static void setPayloadProvider(Supplier<Iterable<String>> provider) {

diff --git a/...ta/src/test/java/org/zaproxy/zap/extension/pscanrulesBeta/JsFunctionScanRuleUnitTest.java b/...ta/src/test/java/org/zaproxy/zap/extension/pscanrulesBeta/JsFunctionScanRuleUnitTest.java
@@ -25,7 +25,7 @@
 import static org.hamcrest.Matchers.hasKey;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.is;
-import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.hamcrest.core.StringStartsWith.startsWith;
 
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -38,6 +38,7 @@
 import org.apache.commons.httpclient.URIException;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 import org.junit.jupiter.params.provider.ValueSource;
 import org.parosproxy.paros.Constant;
 import org.parosproxy.paros.core.scanner.Alert;
@@ -64,19 +65,25 @@ public void setUpZap() throws Exception {
         Files.write(testFile, Arrays.asList("# Test File", "bypassSecurityTrustHtml", "eval"));
     }
 
-    @Test
-    void shouldAlertGivenFunctionInJavaScriptResponse()
+    @ParameterizedTest(name = "{1}")
+    @CsvSource({
+        "'$eval ()','Space'",
+        "'eval\t()','Tab'",
+        "'eval\n()','NewLine'",
+        "'eval\r\n()','CRLF'"
+    })
+    void shouldAlertGivenFunctionInJavaScriptResponse(String content, String argName)
             throws HttpMalformedHeaderException, URIException {
         // Given
-        String body = "Some text <script>$eval()</script>\nLine 2\n";
+        String body = "Some text <script>" + content + "</script>\nLine 2\n";
         HttpMessage msg = createHttpMessageWithRespBody(body, "text/javascript;charset=ISO-8859-1");
 
         // When
         scanHttpResponseReceive(msg);
 
         // Then
         assertThat(alertsRaised, hasSize(1));
-        assertEquals("eval", alertsRaised.get(0).getEvidence());
+        assertThat(alertsRaised.get(0).getEvidence(), startsWith("eval"));
     }
 
     @Test
@@ -94,7 +101,7 @@ void shouldAlertGivenFunctionInHtmlScriptElements()
 
         // Then
         assertThat(alertsRaised, hasSize(1));
-        assertEquals("bypassSecurityTrustHtml", alertsRaised.get(0).getEvidence());
+        assertThat(alertsRaised.get(0).getEvidence(), startsWith("bypassSecurityTrustHtml"));
     }
 
     @Test
@@ -107,7 +114,7 @@ void shouldNotAlertGivenNoMatch() throws URIException, HttpMalformedHeaderExcept
         scanHttpResponseReceive(msg);
 
         // Then
-        assertThat(alertsRaised, empty());
+        assertThat(alertsRaised, is(empty()));
     }
 
     @Test
@@ -121,7 +128,7 @@ void shouldNotAlertGivenEmptyBody() throws HttpMalformedHeaderException, URIExce
         scanHttpResponseReceive(msg);
 
         // Then
-        assertThat(alertsRaised, empty());
+        assertThat(alertsRaised, is(empty()));
     }
 
     @Test
@@ -138,7 +145,7 @@ void shouldAlertGivenCustomPayloadFunctionMatch()
 
         // Then
         assertThat(alertsRaised, hasSize(1));
-        assertEquals("badFunction", alertsRaised.get(0).getEvidence());
+        assertThat(alertsRaised.get(0).getEvidence(), startsWith("badFunction"));
     }
 
     @Test
@@ -153,7 +160,7 @@ void shouldNotAlertGivenMatchOutsideScript() throws HttpMalformedHeaderException
         scanHttpResponseReceive(msg);
 
         // Then
-        assertThat(alertsRaised, empty());
+        assertThat(alertsRaised, is(empty()));
     }
 
     @Test
@@ -170,7 +177,7 @@ void shouldAlertGivenMatchInSecondScript() throws HttpMalformedHeaderException,
 
         // Then
         assertThat(alertsRaised, hasSize(1));
-        assertEquals("eval", alertsRaised.get(0).getEvidence());
+        assertThat(alertsRaised.get(0).getEvidence(), startsWith("eval"));
     }
 
     @Test
@@ -188,7 +195,7 @@ void shouldAlertOnceGivenMultipleMatchesHTML()
 
         // Then
         assertThat(alertsRaised, hasSize(1));
-        assertEquals("eval", alertsRaised.get(0).getEvidence());
+        assertThat(alertsRaised.get(0).getEvidence(), startsWith("eval"));
     }
 
     @Test
@@ -202,15 +209,21 @@ void shouldAlertOnceGivenMultipleMatchesJS() throws HttpMalformedHeaderException
 
         // Then
         assertThat(alertsRaised, hasSize(1));
-        assertEquals("bypassSecurityTrustHtml", alertsRaised.get(0).getEvidence());
+        assertThat(alertsRaised.get(0).getEvidence(), startsWith("bypassSecurityTrustHtml"));
     }
 
     @ParameterizedTest
-    @ValueSource(strings = {"globalEval", "parentPromiseValue"})
-    void shouldNotAlertOnMatchingSubString(String funcName)
+    @ValueSource(
+            strings = {
+                "globalEval()",
+                "parentPromiseValue()",
+                "var V=hungarian:['A[href*=\"eval.example.org\"]','A[href*=\"ad.example.com \"]'",
+                "function T(){var n=window,e=navigator;return y([\"ApplePayError\"in n,\"CSSPrimitiveValue\"in"
+            })
+    void shouldNotAlertOnMatchingSubString(String content)
             throws HttpMalformedHeaderException, URIException {
         // Given
-        String body = "Some text <script>" + funcName + "()</script>\n";
+        String body = "Some text <script>" + content + "</script>\n";
         HttpMessage msg = createHttpMessageWithRespBody(body, "text/javascript;charset=ISO-8859-1");
 
         // When