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

[kingthorin]

Overview

• 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.

Related Issues

• Fixes False Positive - Path Traversal zaproxy#8379

Checklist

• [na] Update help
• Update changelog
• Run ./gradlew spotlessApply for code formatting
• Write tests
• Check code coverage
• Sign-off commits
• Squash commits
• Use a descriptive title

[thc202]

This does not seem to address the FP reported in the referenced issue.

[kingthorin]

No it simply provides further context. IMHO it will be rare for all 5 of the nix matches to happen.

Do you want me to also exclude JS/CSS/Binary'ish because that's an option too which I could add to this PR.

[thc202]

Not sure how rare is since JS chunks/libs tend to have lot of data, but we should not close the issue if it does not address it.

That would be better to address the issue (though the evidence match done beforehand should have caught the reported case, if actually static content).

[kingthorin]

Okay I'll make further changes.

[kingthorin]

This rule doesn't seem to pre-check the response. I'll tackle that as well.

[kingthorin]

Addressed review, further pre-checks as discussed still coming 😁

[kingthorin]

Now w/ pre-checks.

[psiinon]

Checkmarx One – Scan Summary & Details – bcdaa672-8113-4377-9ed6-fa837b9b6dac

Great job, no security vulnerabilities found in this Pull Request

[kingthorin]

I've rebased this to current. I believe all the previous feedback has been addressed.

Please correct me if I'm wrong.

[kingthorin]

Rebased current, and reviewed previous comments. As far as I see they're all addressed.

[thc202]

It would be better if this was done separately for the OS that's being checked, there's no reason to skip because of Nix occurrences if we are checking Win.

[thc202]

Probably better to update the help to mention the resources that will be skipped. Although on second thought, maybe we should go with the precheck to start with.

[kingthorin]

Probably better to update the help to mention the resources that will be skipped. Although on second thought, maybe we should go with the precheck to start with.

Do you mean split up the changes, or do the pre-check before the resource checks?

[thc202]

Split up, not skip resources for now.

[kingthorin]

Thanks for the review. It's good to get this moving again.

[kingthorin]

Tweaked.

Resource Checking that was trimmed

# Note this was a diff after removal, so these actually need to be added if deemed necessary in the future
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
index 32e85e330b..2392b1a806 100644
--- 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
@@ -579,13 +579,6 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
     }
 
     private static boolean isGoodCandidate(HttpMessage msg, Tech targetTech) {
-        if (ResourceIdentificationUtils.isJavaScript(msg)
-                || ResourceIdentificationUtils.isCss(msg)
-                || ResourceIdentificationUtils.responseContainsControlChars(msg)) {
-
-            return false;
-        }
-
         if (targetTech == Tech.Windows) {
             return DirNamesContentsMatcher.matchWinDirectories(msg.getResponseBody().toString())
                     == null;
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
index 51a7b7e2d5..a7e4a91766 100644
--- 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
@@ -389,30 +389,6 @@ class PathTraversalScanRuleUnitTest extends ActiveScannerTest<PathTraversalScanR
         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
     @CsvSource({
         // Windows will always happen before ignoring Linux, if only Linux pre-check matches