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

- CHANGELOG > Added change note.
- Message.properties > Added key/value pair supporting the new Alert
details.
- PathTraversalScanRule > Updated to include Other Info on Alerts when
applicable, and pre-check the original message response to reduce false
positives.
- PathTraversalScanRuleUnitTest > Updated to assert Other Info or lack
thereof where applicable, also assure appropriate skipping due to
pre-conditions.

Signed-off-by: kingthorin <[email protected]>

Author: kingthorin

addOns/ascanrules/CHANGELOG.md

@@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ## Unreleased
 ### Changed
 - The XML External Entity Attack scan rule now include example alert functionality for documentation generation purposes (Issue 6119).
+- 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).
 
 ### Fixed
 - Added more checks for valid .htaccess files to reduce false positives (Issue 7632).

addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java

@@ -40,6 +40,7 @@
 import org.parosproxy.paros.core.scanner.Category;
 import org.parosproxy.paros.network.HttpMessage;
 import org.zaproxy.addon.commonlib.CommonAlertTag;
+import org.zaproxy.addon.commonlib.ResourceIdentificationUtils;
 import org.zaproxy.addon.commonlib.vulnerabilities.Vulnerabilities;
 import org.zaproxy.addon.commonlib.vulnerabilities.Vulnerability;
 import org.zaproxy.addon.network.common.ZapSocketTimeoutException;
@@ -67,6 +68,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",
@@ -116,6 +118,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",
@@ -177,6 +180,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
      */
@@ -221,6 +226,10 @@ public String getReference() {
     @Override
     public void scan(HttpMessage msg, String param, String value) {
 
+        if (!isGoodCandidate(getBaseMsg())) {
+            return;
+        }
+
         try {
             // figure out how aggressively we should test
             int nixCount = 0;
@@ -555,6 +564,18 @@ public void scan(HttpMessage msg, String param, String value) {
         }
     }
 
+    private static boolean isGoodCandidate(HttpMessage msg) {
+        if (ResourceIdentificationUtils.isJavaScript(msg)
+                || ResourceIdentificationUtils.isCss(msg)
+                || ResourceIdentificationUtils.responseContainsControlChars(msg)) {
+
+            return false;
+        }
+        return DirNamesContentsMatcher.matchNixDirectories(msg.getResponseBody().toString()) == null
+                && DirNamesContentsMatcher.matchWinDirectories(msg.getResponseBody().toString())
+                        == null;
+    }
+
     private boolean sendAndCheckPayload(
             String param, String newValue, ContentsMatcher contentsMatcher, int check)
             throws IOException {
@@ -644,12 +665,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
@@ -691,6 +723,24 @@ public String match(String contents) {
 
     private static class DirNamesContentsMatcher implements ContentsMatcher {
 
+        public static final String NIX_MATCHES =
+                String.join(", ", List.of("proc", NIX_DIR_EVIDENCE, "boot", "tmp", "home"));
+        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);
@@ -701,36 +751,24 @@ 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;
         }
 
+        public static final String WIN_MATCHES =
+                String.join(", ", List.of(WIN_DIR_EVIDENCE, "Program Files"));
+
         private static String matchWinDirectories(String contents) {
-            if (contents.contains("Windows")
+            if (contents.contains(WIN_DIR_EVIDENCE)
                     && Pattern.compile("Program\\sFiles").matcher(contents).find()) {
-                return "Windows";
+                return WIN_DIR_EVIDENCE;
             }
 
             return null;

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.

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;
@@ -34,6 +35,7 @@
 import org.apache.commons.lang3.ArrayUtils;
 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.EnumSource;
 import org.junit.jupiter.params.provider.ValueSource;
 import org.parosproxy.paros.core.scanner.Alert;
@@ -42,6 +44,7 @@
 import org.parosproxy.paros.core.scanner.Plugin.AttackStrength;
 import org.parosproxy.paros.network.HttpMalformedHeaderException;
 import org.parosproxy.paros.network.HttpMessage;
+import org.parosproxy.paros.network.HttpResponseHeader;
 import org.zaproxy.addon.commonlib.CommonAlertTag;
 import org.zaproxy.zap.model.Tech;
 import org.zaproxy.zap.testutils.NanoServerHandler;
@@ -163,6 +166,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")));
     }
 
@@ -181,6 +191,13 @@ void shouldAlertIfAttackResponseListsLinuxDirectories() throws Exception {
         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")));
     }
 
@@ -199,6 +216,13 @@ void shouldAlertIfAttackResponseListsLinuxDirectoriesInPlainText() throws Except
         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")));
     }
 
@@ -272,6 +296,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
@@ -288,6 +313,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
@@ -308,6 +334,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
@@ -356,6 +383,47 @@ void shouldNotAlertOnCheckFiveAtLowThresholdUnderInvalidConditions(String errorT
         assertThat(alertsRaised, hasSize(0));
     }
 
+    @ParameterizedTest
+    @ValueSource(strings = {"/styles.css", "/code.js"})
+    void shouldSkipIfReqPathIsCssOrJs(String path) throws Exception {
+        // Given
+        rule.init(getHttpMessage(path + "?p=v"), parent);
+        // When
+        rule.scan();
+        // Then
+        assertThat(httpMessagesSent, hasSize(equalTo(0)));
+    }
+
+    @ParameterizedTest
+    @CsvSource({"/styles/, text/css", "/code, text/javascript"})
+    void shouldSkipIfResponseIsCssOfJs(String path, String contentType) throws Exception {
+        // Given
+        HttpMessage msg = getHttpMessage(path + "?p=v");
+        msg.getResponseHeader().setHeader(HttpResponseHeader.CONTENT_TYPE, contentType);
+        rule.init(msg, parent);
+        // When
+        rule.scan();
+        // Then
+        assertThat(httpMessagesSent, hasSize(equalTo(0)));
+    }
+
+    @ParameterizedTest
+    @ValueSource(
+            strings = {
+                "etc root tmp bin boot dev home mnt opt proc",
+                "Program Files Users Windows"
+            })
+    void shouldSkipIfResponseHasEvidenceToStartWith(String content) throws Exception {
+        // Given
+        HttpMessage msg = getHttpMessage("/?p=v");
+        msg.setResponseBody(content);
+        rule.init(msg, parent);
+        // When
+        rule.scan();
+        // Then
+        assertThat(httpMessagesSent, hasSize(equalTo(0)));
+    }
+
     private abstract static class ListDirsOnAttack extends NanoServerHandler {
 
         private final String param;