ascanrulesBeta & pscanrules: CSRF rules ignore GET unless Low threshold

- CHANGELOGs > Added change note. - Scan Rules > Added condition to bail of now Low threshold and GET request. - Unit Tests > Updated to assert the new behavior and leverage parameterized tests. - Help files > Updated to note the new condition. Signed-off-by: kingthorin <[email protected]>
zaproxy · Sep 18, 2024 · c749c13 · c749c13
1 parent fed220d
commit c749c13
Show file tree

Hide file tree

Showing 8 changed files with 109 additions and 32 deletions.
diff --git a/addOns/ascanrulesBeta/CHANGELOG.md b/addOns/ascanrulesBeta/CHANGELOG.md
@@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ### Changed
 - Log exception details in Out of Band XSS scan rule.
 - Maintenance changes.
+- The CSRF Token scan rule now only considers GET requests at Low Threshold (Issue 7741).
 
 ## [55] - 2024-09-02
 ### Changed

diff --git a/...anrulesBeta/src/main/java/org/zaproxy/zap/extension/ascanrulesBeta/CsrfTokenScanRule.java b/...anrulesBeta/src/main/java/org/zaproxy/zap/extension/ascanrulesBeta/CsrfTokenScanRule.java
@@ -41,6 +41,7 @@
 import org.parosproxy.paros.model.OptionsParam;
 import org.parosproxy.paros.network.HtmlParameter;
 import org.parosproxy.paros.network.HttpMessage;
+import org.parosproxy.paros.network.HttpRequestHeader;
 import org.zaproxy.addon.commonlib.CommonAlertTag;
 import org.zaproxy.addon.commonlib.vulnerabilities.Vulnerabilities;
 import org.zaproxy.addon.commonlib.vulnerabilities.Vulnerability;
@@ -127,6 +128,11 @@ public void scan() {
             return;
         }
 
+        if (!AlertThreshold.LOW.equals(getAlertThreshold())
+                && HttpRequestHeader.GET.equals(getBaseMsg().getRequestHeader().getMethod())) {
+            return;
+        }
+
         boolean vuln = false;
         Map<String, String> tagsMap = new HashMap<>();
         Source s1;

diff --git a/.../javahelp/org/zaproxy/zap/extension/ascanrulesBeta/resources/help/contents/ascanbeta.html b/.../javahelp/org/zaproxy/zap/extension/ascanrulesBeta/resources/help/contents/ascanbeta.html
@@ -49,6 +49,8 @@ <H2 id="id-20012">CSRF Token</H2>
 Any FORMs with a name or ID that matches one of these identifiers will be ignored when scanning for missing Anti-CSRF tokens.
 Only use this feature to ignore FORMs that you know are safe, for example search forms.
 <p>
+Note: GET requests are only evaluated at Low Threshold
+<p>
 Latest code: <a href="https://github.com/zaproxy/zap-extensions/blob/main/addOns/ascanrulesBeta/src/main/java/org/zaproxy/zap/extension/ascanrulesBeta/CsrfTokenScanRule.java">CsrfTokenScanRule.java</a>
 <br>
 Alert ID: <a href="https://www.zaproxy.org/docs/alerts/20012/">20012</a>.

diff --git a/...eta/src/test/java/org/zaproxy/zap/extension/ascanrulesBeta/CsrfTokenScanRuleUnitTest.java b/...eta/src/test/java/org/zaproxy/zap/extension/ascanrulesBeta/CsrfTokenScanRuleUnitTest.java
@@ -33,15 +33,22 @@
 import java.util.List;
 import java.util.Map;
 import java.util.TreeSet;
+import java.util.stream.Stream;
 import org.apache.commons.httpclient.URIException;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.EnumSource;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.parosproxy.paros.core.scanner.Alert;
 import org.parosproxy.paros.core.scanner.Plugin.AlertThreshold;
 import org.parosproxy.paros.model.Model;
 import org.parosproxy.paros.model.OptionsParam;
 import org.parosproxy.paros.network.HtmlParameter;
 import org.parosproxy.paros.network.HttpMalformedHeaderException;
 import org.parosproxy.paros.network.HttpMessage;
+import org.parosproxy.paros.network.HttpRequestHeader;
 import org.zaproxy.addon.commonlib.CommonAlertTag;
 import org.zaproxy.addon.commonlib.http.HttpFieldsNames;
 import org.zaproxy.zap.extension.httpsessions.HttpSessionsParam;
@@ -229,13 +236,16 @@ void shouldProcessAtHighThresholdAndInScope()
         assertThat(httpMessagesSent, hasSize(greaterThan(0)));
     }
 
-    @Test
-    void shouldProcessAtMediumThresholdAndOutOfScope()
+    @ParameterizedTest
+    @EnumSource(
+            value = AlertThreshold.class,
+            names = {"MEDIUM", "LOW"})
+    void shouldProcessAtBelowHighThresholdAndOutOfScope(AlertThreshold threshold)
             throws HttpMalformedHeaderException, URIException {
         // Given
         HttpMessage msg = createMessage(false);
         rule.setConfig(new ZapXmlConfiguration());
-        rule.setAlertThreshold(AlertThreshold.MEDIUM);
+        rule.setAlertThreshold(threshold);
         // Note: This Test leverages the context setup in a previous test
         rule.init(msg, parent);
         // When
@@ -244,40 +254,37 @@ void shouldProcessAtMediumThresholdAndOutOfScope()
         assertThat(httpMessagesSent, hasSize(greaterThan(0)));
     }
 
-    @Test
-    void shouldProcessAtMediumThresholdAndInScope()
-            throws HttpMalformedHeaderException, URIException {
-        // Given
-        HttpMessage msg = createMessage(true);
-        rule.setConfig(new ZapXmlConfiguration());
-        rule.setAlertThreshold(AlertThreshold.MEDIUM);
-        // Note: This Test leverages the context setup in a previous test
-        rule.init(msg, parent);
-        // When
-        this.rule.scan();
-        // Then
-        assertThat(httpMessagesSent, hasSize(greaterThan(0)));
+    private static Stream<Arguments> provideNonLowThresholdsAndBooleans() {
+        return Stream.of(
+                Arguments.of(AlertThreshold.MEDIUM, true),
+                Arguments.of(AlertThreshold.MEDIUM, false),
+                Arguments.of(AlertThreshold.HIGH, true),
+                Arguments.of(AlertThreshold.HIGH, false));
     }
 
-    @Test
-    void shouldProcessAtLowThresholdAndOutOfScope()
+    @ParameterizedTest
+    @MethodSource("provideNonLowThresholdsAndBooleans")
+    void shouldNotProcessGetAtNonLowThresholdRegardlessOfScope(
+            AlertThreshold threshold, boolean inScope)
             throws HttpMalformedHeaderException, URIException {
         // Given
-        HttpMessage msg = createMessage(false);
+        HttpMessage msg = createMessage(inScope, HttpRequestHeader.GET);
         rule.setConfig(new ZapXmlConfiguration());
-        rule.setAlertThreshold(AlertThreshold.LOW);
+        rule.setAlertThreshold(threshold);
         // Note: This Test leverages the context setup in a previous test
         rule.init(msg, parent);
         // When
         this.rule.scan();
         // Then
-        assertThat(httpMessagesSent, hasSize(greaterThan(0)));
+        assertThat(httpMessagesSent, hasSize(0));
     }
 
-    @Test
-    void shouldProcessAtLowThresholdAndInScope() throws HttpMalformedHeaderException, URIException {
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    void shouldProcessGetAtLowThresholdRegardlessOfScope(boolean inScope)
+            throws HttpMalformedHeaderException, URIException {
         // Given
-        HttpMessage msg = createMessage(true);
+        HttpMessage msg = createMessage(inScope, HttpRequestHeader.GET);
         rule.setConfig(new ZapXmlConfiguration());
         rule.setAlertThreshold(AlertThreshold.LOW);
         // Note: This Test leverages the context setup in a previous test
@@ -333,6 +340,11 @@ public void shouldHaveValidReferences() {
 
     private HttpMessage createMessage(boolean isInScope)
             throws URIException, HttpMalformedHeaderException {
+        return createMessage(isInScope, HttpRequestHeader.POST);
+    }
+
+    private HttpMessage createMessage(boolean isInScope, String method)
+            throws URIException, HttpMalformedHeaderException {
         HttpMessage msg =
                 new HttpMessage() {
 
@@ -354,6 +366,7 @@ public boolean isInScope() {
                             } catch (HttpMalformedHeaderException e) {
                                 throw new RuntimeException(e);
                             }
+                            newMsg.getRequestHeader().setMethod(method);
                             newMsg.setRequestBody(this.getRequestBody().getBytes());
                         }
                         return newMsg;
@@ -378,7 +391,7 @@ private static void setUpHttpSessionsParam() {
 
     private HttpMessage getAntiCSRFCompatibleMessage() throws HttpMalformedHeaderException {
         return getHttpMessage(
-                "GET",
+                "POST",
                 "/",
                 "<html><form><input type=\"hidden\" name=\"customAntiCSRF\" value="
                         + Math.random()

diff --git a/addOns/pscanrules/CHANGELOG.md b/addOns/pscanrules/CHANGELOG.md
@@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 - Maintenance changes.
 - Rename Mac OSX salted SHA-1 in the Hash Disclosure scan rule to "Salted SHA-1", reduce the associated alerts to Low risk and Low confidence, to align with other SHA related patterns it will only be evaluated a Low Threshold. (Note such matches may indicate leaks related but not limited to: MacOS X, Oracle, Tiger-192, Haval-192) (Issue 8624).
 - The Insecure JSF ViewState now includes example alert functionality for documentation generation purposes (Issue 6119).
+- The CSRF Countermeasures scan rule now only considers GET requests at Low Threshold (Issue 7741).
 
 ## [60] - 2024-09-02
 ### Changed

diff --git a/...rules/src/main/java/org/zaproxy/zap/extension/pscanrules/CsrfCountermeasuresScanRule.java b/...rules/src/main/java/org/zaproxy/zap/extension/pscanrules/CsrfCountermeasuresScanRule.java
@@ -37,6 +37,7 @@
 import org.parosproxy.paros.core.scanner.Plugin.AlertThreshold;
 import org.parosproxy.paros.model.Model;
 import org.parosproxy.paros.network.HttpMessage;
+import org.parosproxy.paros.network.HttpRequestHeader;
 import org.zaproxy.addon.commonlib.CommonAlertTag;
 import org.zaproxy.addon.commonlib.vulnerabilities.Vulnerabilities;
 import org.zaproxy.addon.commonlib.vulnerabilities.Vulnerability;
@@ -91,6 +92,11 @@ public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) {
             return;
         }
 
+        if (!AlertThreshold.LOW.equals(getAlertThreshold())
+                && HttpRequestHeader.GET.equals(msg.getRequestHeader().getMethod())) {
+            return;
+        }
+
         // need to do this if we are to be able to get an element's parent. Do it as early as
         // possible in the logic
         source.fullSequentialParse();

diff --git a/...ain/javahelp/org/zaproxy/zap/extension/pscanrules/resources/help/contents/pscanrules.html b/...ain/javahelp/org/zaproxy/zap/extension/pscanrules/resources/help/contents/pscanrules.html
@@ -213,7 +213,13 @@ <H2 id="id-10202">CSRF Countermeasures</H2>
 Only use this feature to ignore FORMs that you know are safe, for example search forms.
 Form element names are sorted and de-duplicated when they are printed in the ZAP Report.
 <br>
-Note: The rule also takes into account the Partial match setting within the Anti-CSRF Options.
+Note:
+<ul>
+    <li>The rule also takes into account the Partial match setting within the Anti-CSRF Options.</li>
+    <li>GET requests are only evaluated at Low Threshold</li>
+</ul>
+
+<p>]
 <p>
 Latest code: <a href="https://github.com/zaproxy/zap-extensions/blob/main/addOns/pscanrules/src/main/java/org/zaproxy/zap/extension/pscanrules/CsrfCountermeasuresScanRule.java">CsrfCountermeasuresScanRule.java</a>
 <br>

diff --git a/...c/test/java/org/zaproxy/zap/extension/pscanrules/CsrfCountermeasuresScanRuleUnitTest.java b/...c/test/java/org/zaproxy/zap/extension/pscanrules/CsrfCountermeasuresScanRuleUnitTest.java
@@ -21,6 +21,7 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.is;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -32,10 +33,16 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.stream.Stream;
 import org.apache.commons.httpclient.URI;
 import org.apache.commons.httpclient.URIException;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.EnumSource;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.mockito.quality.Strictness;
 import org.parosproxy.paros.core.scanner.Alert;
 import org.parosproxy.paros.core.scanner.Plugin.AlertThreshold;
@@ -426,24 +433,30 @@ void shouldRaiseAlertWhenThresholdHighAndMessageInScope() throws URIException {
         assertEquals(1, alertsRaised.size());
     }
 
-    @Test
-    void shouldRaiseAlertWhenThresholdMediumAndMessageOutOfScope() throws URIException {
+    @ParameterizedTest
+    @EnumSource(
+            value = AlertThreshold.class,
+            names = {"MEDIUM", "LOW"})
+    void shouldRaiseAlertBelowHighThresholdAndOutOfScope(AlertThreshold threshold)
+            throws URIException {
         // Given
         rule.setCSRFIgnoreAttName("ignore");
         HttpMessage msg = createScopedMessage(false);
         // When
         rule.setConfig(new ZapXmlConfiguration());
-        rule.setAlertThreshold(AlertThreshold.MEDIUM);
+        rule.setAlertThreshold(threshold);
         scanHttpResponseReceive(msg);
         // Then
         assertEquals(1, alertsRaised.size());
     }
 
-    @Test
-    void shouldRaiseAlertWhenThresholdLowAndMessageOutOfScope() throws URIException {
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    void shouldRaiseAlertOnGetAtLowThresholdRegardlessOfScope(boolean isInScope)
+            throws URIException {
         // Given
         rule.setCSRFIgnoreAttName("ignore");
-        HttpMessage msg = createScopedMessage(false);
+        HttpMessage msg = createScopedMessage(isInScope, HttpRequestHeader.GET);
         // When
         rule.setConfig(new ZapXmlConfiguration());
         rule.setAlertThreshold(AlertThreshold.LOW);
@@ -452,12 +465,40 @@ void shouldRaiseAlertWhenThresholdLowAndMessageOutOfScope() throws URIException
         assertEquals(1, alertsRaised.size());
     }
 
+    private static Stream<Arguments> provideNonLowThresholdsAndBooleans() {
+        return Stream.of(
+                Arguments.of(AlertThreshold.MEDIUM, true),
+                Arguments.of(AlertThreshold.MEDIUM, false),
+                Arguments.of(AlertThreshold.HIGH, true),
+                Arguments.of(AlertThreshold.HIGH, false));
+    }
+
+    @ParameterizedTest
+    @MethodSource("provideNonLowThresholdsAndBooleans")
+    void shouldNotRaiseAlertOnGetAtNonLowThresholdRegardlessOfScope(
+            AlertThreshold threshold, boolean isInScope) throws URIException {
+        // Given
+        rule.setCSRFIgnoreAttName("ignore");
+        HttpMessage msg = createScopedMessage(isInScope, HttpRequestHeader.GET);
+        // When
+        rule.setConfig(new ZapXmlConfiguration());
+        rule.setAlertThreshold(threshold);
+        scanHttpResponseReceive(msg);
+        // Then
+        assertThat(alertsRaised, hasSize(0));
+    }
+
     void formWithoutAntiCsrfToken() {
         msg.setResponseBody(
                 "<html><head></head><body><form id=\"no_csrf_token\"><input type=\"text\"/><input type=\"submit\"/></form></body></html>");
     }
 
     private static HttpMessage createScopedMessage(boolean isInScope) throws URIException {
+        return createScopedMessage(isInScope, HttpRequestHeader.POST);
+    }
+
+    private static HttpMessage createScopedMessage(boolean isInScope, String method)
+            throws URIException {
         HttpMessage newMsg =
                 new HttpMessage() {
                     @Override
@@ -466,6 +507,7 @@ public boolean isInScope() {
                     }
                 };
         newMsg.getRequestHeader().setURI(new URI("http://", "localhost", "/", ""));
+        newMsg.getRequestHeader().setMethod(method);
         newMsg.getResponseHeader().setHeader(HttpHeader.CONTENT_TYPE, "text/html");
         newMsg.setResponseBody(
                 "<html><head></head><body>"