ascanrulesBeta: Address possible FP in proxy detection rule

- CHANGELOG > Added change note. - ProxyDisclosureScanRule > Added condition to skip messages if they have evidence content to start with. Removed misleading Attack text from the Alert. - ProxyDisclosureScanRuleUnitTest > Added a test to assert the new behavior. Signed-off-by: kingthorin <[email protected]>
zaproxy · Sep 10, 2024 · f62d8fa · f62d8fa
1 parent d34392d
commit f62d8fa
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 6 deletions.
diff --git a/addOns/ascanrulesBeta/CHANGELOG.md b/addOns/ascanrulesBeta/CHANGELOG.md
@@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ## Unreleased
 ### Changed
 - Log exception details in Out of Band XSS scan rule.
+- The Proxy Disclosure scan rule will no longer alert on HTTP messages that have evidence to start with, in order to reduce possible false positives (Issue 8556). The misleading Attack string for the Alerts was also removed.
 
 ## [55] - 2024-09-02
 ### Changed

diff --git a/...sBeta/src/main/java/org/zaproxy/zap/extension/ascanrulesBeta/ProxyDisclosureScanRule.java b/...sBeta/src/main/java/org/zaproxy/zap/extension/ascanrulesBeta/ProxyDisclosureScanRule.java
@@ -331,7 +331,12 @@ public void scan() {
                             String proxyServer = PROXY_REQUEST_HEADERS.get(proxyHeaderPattern);
                             Matcher proxyHeaderMatcher =
                                     proxyHeaderPattern.matcher(traceResponseBody);
-                            if (proxyHeaderMatcher.find()) {
+                            Matcher originalBodyMatcher =
+                                    proxyHeaderPattern.matcher(
+                                            getBaseMsg().getResponseBody().toString());
+                            // Ensure the original message didn't already have evidence type
+                            // content
+                            if (!originalBodyMatcher.find() && proxyHeaderMatcher.find()) {
                                 String proxyHeaderName = proxyHeaderMatcher.group(1);
                                 proxyActuallyFound = true;
                                 LOGGER.debug(
@@ -752,7 +757,6 @@ public void scan() {
                                 Constant.messages.getString(
                                         MESSAGE_PREFIX + "desc",
                                         step2numberOfNodes - 1 + silentProxySet.size()))
-                        .setAttack(getAttack())
                         .setOtherInfo(extraInfo)
                         .setMessage(getBaseMsg())
                         .raise();
@@ -773,10 +777,6 @@ private String getPath(URI uri) {
         return "/";
     }
 
-    private String getAttack() {
-        return Constant.messages.getString(MESSAGE_PREFIX + "attack");
-    }
-
     @Override
     public int getRisk() {
         return Alert.RISK_MEDIUM;

diff --git a/...c/test/java/org/zaproxy/zap/extension/ascanrulesBeta/ProxyDisclosureScanRuleUnitTest.java b/...c/test/java/org/zaproxy/zap/extension/ascanrulesBeta/ProxyDisclosureScanRuleUnitTest.java
@@ -19,13 +19,24 @@
  */
 package org.zaproxy.zap.extension.ascanrulesBeta;
 
+import static fi.iki.elonen.NanoHTTPD.newFixedLengthResponse;
 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 fi.iki.elonen.NanoHTTPD;
+import fi.iki.elonen.NanoHTTPD.IHTTPSession;
+import fi.iki.elonen.NanoHTTPD.Response;
 import java.util.Map;
+import org.apache.commons.httpclient.URIException;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.parosproxy.paros.network.HttpMalformedHeaderException;
+import org.parosproxy.paros.network.HttpMessage;
 import org.zaproxy.addon.commonlib.CommonAlertTag;
+import org.zaproxy.zap.testutils.NanoServerHandler;
 
 class ProxyDisclosureScanRuleUnitTest extends ActiveScannerTest<ProxyDisclosureScanRule> {
 
@@ -57,4 +68,34 @@ void shouldReturnExpectedMappings() {
                 tags.get(CommonAlertTag.OWASP_2017_A06_SEC_MISCONFIG.getTag()),
                 is(equalTo(CommonAlertTag.OWASP_2017_A06_SEC_MISCONFIG.getValue())));
     }
+
+    @ParameterizedTest
+    @ValueSource(
+            strings = {
+                "X-Forwarded-For: 127.0.0.1",
+                "X-Forwarded-Port: 443",
+                "X-Forwarded-Proto: https",
+                "Via: 1.1 vegur"
+            })
+    void shouldNotAlertIfOriginalHasEvidence(String header)
+            throws HttpMalformedHeaderException, URIException {
+        // Given
+        String test = "/";
+        nano.addHandler(
+                new NanoServerHandler(test) {
+
+                    @Override
+                    protected Response serve(IHTTPSession session) {
+                        String content = "<html>" + header + "</html>";
+                        return newFixedLengthResponse(
+                                Response.Status.OK, NanoHTTPD.MIME_HTML, content);
+                    }
+                });
+        HttpMessage msg = getHttpMessage(test);
+        rule.init(msg, parent);
+        // When
+        rule.scan();
+        // Then
+        assertThat(alertsRaised, hasSize(equalTo(0))); // No messages sent
+    }
 }