ascanrules: Path Traversal add details for dir match Alerts & reduce FPs

kingthorin · kingthorin · commit 4697d52afa6f · 2025-03-12T10:12:16.000-04:00
- CHANGELOG &gt; Added change note.
- Message.properties &gt; Added key/value pair supporting the new Alert
details.
- PathTraversalScanRule &gt; Updated to include Other Info on Alerts when
applicable, and pre-check the original message response to reduce false
positives.
- PathTraversalScanRuleUnitTest &gt; Updated to assert Other Info or lack
thereof where applicable, also assure appropriate skipping due to
pre-conditions.

Signed-off-by: kingthorin &lt;kingthorin@users.noreply.github.com&gt;
diff --git a/addOns/ascanrules/CHANGELOG.md b/addOns/ascanrules/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
-
+### Changed
+- The Path Traversal scan rule now includes further details when directory matches are made and pre-checks the original message to reduce false positives (Issue 8379).
 
 ## [71] - 2025-03-04
 ### Fixed
diff --git a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java
@@ -81,6 +81,7 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
      */
     private static final ContentsMatcher WIN_PATTERN =
             new PatternContentsMatcher(Pattern.compile("\\[drivers\\]"));
+    private static final String WIN_DIR_EVIDENCE = "Windows";
     private static final String[] WIN_LOCAL_FILE_TARGETS = {
         // Absolute Windows file retrieval (we suppose C:\\)
         "c:/Windows/system.ini",
@@ -130,6 +131,7 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
     // Dot used to match 'x' or '!' (used in AIX)
     private static final ContentsMatcher NIX_PATTERN =
             new PatternContentsMatcher(Pattern.compile("root:.:0:0"));
+    private static final String NIX_DIR_EVIDENCE = "etc";
     private static final String[] NIX_LOCAL_FILE_TARGETS = {
         // Absolute file retrieval
         "/etc/passwd",
@@ -159,7 +161,8 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
      * Windows/Unix/Linux/etc. local directory targets and detection pattern
      */
     private static final ContentsMatcher DIR_PATTERN = new DirNamesContentsMatcher();
-    private static final String[] WIN_LOCAL_DIR_TARGETS = {
+    /* Increased visibility purely for testing purposes */
+    public static final String[] WIN_LOCAL_DIR_TARGETS = {
         "c:/",
         "c:\\",
         "..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\",
@@ -176,7 +179,7 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
         "file:\\\\\\d:\\",
         "file:\\\\\\d:/"
     };
-    private static final String[] NIX_LOCAL_DIR_TARGETS = {
+    public static final String[] NIX_LOCAL_DIR_TARGETS = {
         "/",
         "../../../../../../../../../../../../../../../../",
         "/../../../../../../../../../../../../../../../../",
@@ -191,6 +194,8 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
      */
     private static final String[] LOCAL_FILE_RELATIVE_PREFIXES = {"", "/", "\\"};
 
+    private static final List<String> DIR_EVIDENCE_LIST =
+            List.of(NIX_DIR_EVIDENCE, WIN_DIR_EVIDENCE);
     /*
      * details of the vulnerability which we are attempting to find
      */
@@ -341,7 +346,6 @@ public void scan(HttpMessage msg, String param, String value) {
 
             // Check 2: Start detection for *NIX patterns
             if (inScope(Tech.Linux) || inScope(Tech.MacOS)) {
-
                 for (int h = 0; h < nixCount; h++) {
 
                     // Check if a there was a finding or the scan has been stopped
@@ -382,8 +386,11 @@ public void scan(HttpMessage msg, String param, String value) {
 
             // Check 3: Detect if this page is a directory browsing component
             if (inScope(Tech.Linux) || inScope(Tech.MacOS)) {
-                for (int h = 0; h < nixDirCount; h++) {
-
+                int h = 0;
+                if (!isGoodCandidate(getBaseMsg(), Tech.Linux)) {
+                    h = nixDirCount;
+                }
+                for (; h < nixDirCount; h++) {
                     // Check if a there was a finding or the scan has been stopped
                     // if yes dispose resources and exit
                     if (sendAndCheckPayload(param, NIX_LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3)
@@ -395,7 +402,11 @@ public void scan(HttpMessage msg, String param, String value) {
                 }
             }
             if (inScope(Tech.Windows)) {
-                for (int h = 0; h < winDirCount; h++) {
+                int h = 0;
+                if (!isGoodCandidate(getBaseMsg(), Tech.Windows)) {
+                    h = winDirCount;
+                }
+                for (; h < winDirCount; h++) {
                     if (sendAndCheckPayload(param, WIN_LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3)
                             || isStop()) {
                         // Dispose all resources
@@ -569,6 +580,19 @@ public void scan(HttpMessage msg, String param, String value) {
         }
     }
 
+    private static boolean isGoodCandidate(HttpMessage msg, Tech targetTech) {
+        // It is a good candidate if it doesn't have evidence to start with
+        if (targetTech == Tech.Windows) {
+            return DirNamesContentsMatcher.matchWinDirectories(msg.getResponseBody().toString())
+                    == null;
+        }
+        if (targetTech == Tech.Linux) {
+            return DirNamesContentsMatcher.matchNixDirectories(msg.getResponseBody().toString())
+                    == null;
+        }
+        return false;
+    }
+
     private boolean sendAndCheckPayload(
             String param, String newValue, ContentsMatcher contentsMatcher, int check)
             throws IOException {
@@ -658,12 +682,23 @@ private AlertBuilder createUnmatchedAlert(String param, String attack) {
 
     private AlertBuilder createMatchedAlert(
             String param, String attack, String evidence, int check) {
-        return newAlert()
-                .setConfidence(Alert.CONFIDENCE_MEDIUM)
-                .setParam(param)
-                .setAttack(attack)
-                .setEvidence(evidence)
-                .setAlertRef(getId() + "-" + check);
+        AlertBuilder builder =
+                newAlert()
+                        .setConfidence(Alert.CONFIDENCE_MEDIUM)
+                        .setParam(param)
+                        .setAttack(attack)
+                        .setEvidence(evidence)
+                        .setAlertRef(getId() + "-" + check);
+        if (DIR_EVIDENCE_LIST.contains(evidence)) {
+            builder.setOtherInfo(
+                    Constant.messages.getString(
+                            MESSAGE_PREFIX + "info",
+                            evidence,
+                            evidence.equals(WIN_DIR_EVIDENCE)
+                                    ? DirNamesContentsMatcher.WIN_MATCHES
+                                    : DirNamesContentsMatcher.NIX_MATCHES));
+        }
+        return builder;
     }
 
     @Override
@@ -705,6 +740,26 @@ public String match(String contents) {
 
     private static class DirNamesContentsMatcher implements ContentsMatcher {
 
+        private static final String NIX_MATCHES =
+                String.join(", ", List.of("proc", NIX_DIR_EVIDENCE, "boot", "tmp", "home"));
+        private static final String WIN_MATCHES =
+                String.join(", ", List.of(WIN_DIR_EVIDENCE, "Program Files"));
+        private static final Pattern PROC_PATT =
+                Pattern.compile(
+                        "(?:^|\\W)proc(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
+        private static final Pattern ETC_PATT =
+                Pattern.compile(
+                        "(?:^|\\W)etc(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
+        private static final Pattern BOOT_PATT =
+                Pattern.compile(
+                        "(?:^|\\W)boot(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
+        private static final Pattern TMP_PATT =
+                Pattern.compile(
+                        "(?:^|\\W)tmp(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
+        private static final Pattern HOME_PATT =
+                Pattern.compile(
+                        "(?:^|\\W)home(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
+
         @Override
         public String match(String contents) {
             String result = matchNixDirectories(contents);
@@ -715,36 +770,23 @@ public String match(String contents) {
         }
 
         private static String matchNixDirectories(String contents) {
-            Pattern procPattern =
-                    Pattern.compile("(?:^|\\W)proc(?:\\W|$)", Pattern.CASE_INSENSITIVE);
-            Pattern etcPattern = Pattern.compile("(?:^|\\W)etc(?:\\W|$)", Pattern.CASE_INSENSITIVE);
-            Pattern bootPattern =
-                    Pattern.compile("(?:^|\\W)boot(?:\\W|$)", Pattern.CASE_INSENSITIVE);
-            Pattern tmpPattern = Pattern.compile("(?:^|\\W)tmp(?:\\W|$)", Pattern.CASE_INSENSITIVE);
-            Pattern homePattern =
-                    Pattern.compile("(?:^|\\W)home(?:\\W|$)", Pattern.CASE_INSENSITIVE);
-
-            Matcher procMatcher = procPattern.matcher(contents);
-            Matcher etcMatcher = etcPattern.matcher(contents);
-            Matcher bootMatcher = bootPattern.matcher(contents);
-            Matcher tmpMatcher = tmpPattern.matcher(contents);
-            Matcher homeMatcher = homePattern.matcher(contents);
-
-            if (procMatcher.find()
-                    && etcMatcher.find()
-                    && bootMatcher.find()
-                    && tmpMatcher.find()
-                    && homeMatcher.find()) {
-                return "etc";
+            if (PROC_PATT.matcher(contents).find()
+                    && ETC_PATT.matcher(contents).find()
+                    && BOOT_PATT.matcher(contents).find()
+                    && TMP_PATT.matcher(contents).find()
+                    && HOME_PATT.matcher(contents).find()) {
+                return NIX_DIR_EVIDENCE;
             }
 
             return null;
         }
 
         private static String matchWinDirectories(String contents) {
-            if (contents.contains("Windows")
-                    && Pattern.compile("Program\\sFiles").matcher(contents).find()) {
-                return "Windows";
+            if (contents.contains(WIN_DIR_EVIDENCE)
+                    && Pattern.compile("Program\\sFiles", Pattern.CASE_INSENSITIVE)
+                            .matcher(contents)
+                            .find()) {
+                return WIN_DIR_EVIDENCE;
             }
 
             return null;
diff --git a/addOns/ascanrules/src/main/resources/org/zaproxy/zap/extension/ascanrules/resources/Messages.properties b/addOns/ascanrules/src/main/resources/org/zaproxy/zap/extension/ascanrules/resources/Messages.properties
@@ -114,6 +114,7 @@ ascanrules.parametertamper.desc = Parameter manipulation caused an error page or
 ascanrules.parametertamper.name = Parameter Tampering
 ascanrules.parametertamper.soln = Identify the cause of the error and fix it. Do not trust client side input and enforce a tight check in the server side. Besides, catch the exception properly. Use a generic 500 error page for internal server error.
 
+ascanrules.pathtraversal.info = While the evidence field indicates {0}, the rule actually checked that the response contains matches for all of the following: {1}.
 ascanrules.pathtraversal.name = Path Traversal
 
 ascanrules.payloader.desc = Provides support for custom payloads in scan rules.
diff --git a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java
@@ -21,6 +21,7 @@
 
 import static fi.iki.elonen.NanoHTTPD.newFixedLengthResponse;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.emptyString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.hasSize;
@@ -29,12 +30,17 @@
 import fi.iki.elonen.NanoHTTPD;
 import fi.iki.elonen.NanoHTTPD.IHTTPSession;
 import fi.iki.elonen.NanoHTTPD.Response;
+import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
+import java.util.stream.Stream;
 import org.apache.commons.lang3.ArrayUtils;
 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.CsvSource;
 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;
@@ -169,6 +175,13 @@ void shouldAlertIfAttackResponseListsWindowsDirectories() throws Exception {
         assertThat(alertsRaised.get(0).getAttack(), is(equalTo("c:/")));
         assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH)));
         assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM)));
+        assertThat(
+                alertsRaised.get(0).getOtherInfo(),
+                is(
+                        equalTo(
+                                "While the evidence field indicates Windows, the rule actually "
+                                        + "checked that the response contains matches for all of the "
+                                        + "following: Windows, Program Files.")));
         assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3")));
     }
 
@@ -180,13 +193,20 @@ void shouldAlertIfAttackResponseListsLinuxDirectories() throws Exception {
         // When
         rule.scan();
         // Then
-        assertThat(httpMessagesSent, hasSize(greaterThan(1)));
+        assertThat(httpMessagesSent, hasSize(equalTo(7)));
         assertThat(alertsRaised, hasSize(1));
         assertThat(alertsRaised.get(0).getEvidence(), is(equalTo("etc")));
         assertThat(alertsRaised.get(0).getParam(), is(equalTo("p")));
         assertThat(alertsRaised.get(0).getAttack(), is(equalTo("/")));
         assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH)));
         assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM)));
+        assertThat(
+                alertsRaised.get(0).getOtherInfo(),
+                is(
+                        equalTo(
+                                "While the evidence field indicates etc, the rule actually "
+                                        + "checked that the response contains matches for all of the "
+                                        + "following: proc, etc, boot, tmp, home.")));
         assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3")));
     }
 
@@ -198,13 +218,20 @@ void shouldAlertIfAttackResponseListsLinuxDirectoriesInPlainText() throws Except
         // When
         rule.scan();
         // Then
-        assertThat(httpMessagesSent, hasSize(greaterThan(1)));
+        assertThat(httpMessagesSent, hasSize(equalTo(7)));
         assertThat(alertsRaised, hasSize(1));
         assertThat(alertsRaised.get(0).getEvidence(), is(equalTo("etc")));
         assertThat(alertsRaised.get(0).getParam(), is(equalTo("p")));
         assertThat(alertsRaised.get(0).getAttack(), is(equalTo("/")));
         assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH)));
         assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM)));
+        assertThat(
+                alertsRaised.get(0).getOtherInfo(),
+                is(
+                        equalTo(
+                                "While the evidence field indicates etc, the rule actually "
+                                        + "checked that the response contains matches for all of the "
+                                        + "following: proc, etc, boot, tmp, home.")));
         assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3")));
     }
 
@@ -278,6 +305,7 @@ void shouldRaiseAlertIfResponseHasPasswdFileContentAndPayloadIsNullByteBased()
         // Then
         assertThat(alertsRaised, hasSize(1));
         assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-2")));
+        assertThat(alertsRaised.get(0).getOtherInfo(), is(emptyString()));
     }
 
     @Test
@@ -294,6 +322,7 @@ void shouldRaiseAlertIfResponseHasSystemINIFileContentAndPayloadIsNullByteBased(
         // Then
         assertThat(alertsRaised, hasSize(1));
         assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-1")));
+        assertThat(alertsRaised.get(0).getOtherInfo(), is(emptyString()));
     }
 
     @ParameterizedTest
@@ -314,6 +343,7 @@ void shouldAlertOnCheckFiveBelowHighThresholdUnderValidConditions(AlertThreshold
         assertThat(alertsRaised, hasSize(1));
         assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_LOW)));
         assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-5")));
+        assertThat(alertsRaised.get(0).getOtherInfo(), is(emptyString()));
     }
 
     @Test
@@ -362,6 +392,59 @@ void shouldNotAlertOnCheckFiveAtLowThresholdUnderInvalidConditions(String errorT
         assertThat(alertsRaised, hasSize(0));
     }
 
+    @ParameterizedTest
+    @CsvSource({
+        // Windows will always happen before ignoring Linux, if only Linux pre-check matches
+        "foo etc root tmp bin boot dev home mnt opt proc bar, 11",
+        "foo Program Files Users Windows bar, 11",
+        "foo etc root tmp bin boot dev home mnt opt proc Program Files Users Windows bar, 11"
+    })
+    void shouldSkipDirChecksIfResponseHasDirEvidenceToStartWith(String content, int expected)
+            throws Exception {
+        // Given
+        HttpMessage msg = getHttpMessage("/?p=v");
+        msg.setResponseBody(content);
+        rule.init(msg, parent);
+        // When
+        rule.scan();
+        // Then
+        assertThat(httpMessagesSent, hasSize(equalTo(expected)));
+    }
+
+    private static Stream<Arguments> handlersProvider() {
+        return Stream.of(
+                Arguments.of(
+                        new ListLinuxDirsOnAttack("/foo", "p", "/"),
+                        Arrays.asList(PathTraversalScanRule.NIX_LOCAL_DIR_TARGETS)),
+                Arguments.of(
+                        new ListWinDirsOnAttack("/bar", "p", "c:/"),
+                        Arrays.asList(PathTraversalScanRule.WIN_LOCAL_DIR_TARGETS)));
+    }
+
+    @ParameterizedTest
+    @MethodSource("handlersProvider")
+    void shouldNotSkipIfGoodCandidateForOnlyOneTech(
+            NanoServerHandler handler, List<String> skippedPayloads) throws Exception {
+        // Given
+        nano.addHandler(handler);
+        HttpMessage msg = getHttpMessage("/?p=v");
+        rule.init(msg, parent);
+        // When
+        rule.scan();
+        // Then
+        System.out.println("Expected Skipped Payloads: " + skippedPayloads);
+        for (HttpMessage m : httpMessagesSent) {
+            String paramVal = m.getUrlParams().first().getValue();
+            for (String payload : skippedPayloads) {
+                System.out.println("Payload: " + payload);
+                System.out.println("Param Val: " + m.getUrlParams().first().getValue());
+                assertThat(paramVal.equals(payload), is(equalTo(false)));
+            }
+        }
+        assertThat(alertsRaised, hasSize(equalTo(1)));
+        System.out.println("Attack: " + alertsRaised.get(0).getAttack());
+    }
+
     private abstract static class ListDirsOnAttack extends NanoServerHandler {
 
         private final String param;
