ascanrules: Path Traversal add details for dir match Alerts

[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.
• PathTraversalScanRuleUnitTest > Updated to assert Other Info or lack thereof where applicable, also assure appropriate skipping due to pre-conditions.

Related Issues

• 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 – fb8ede2c-6d02-403e-afad-c80f2c638a79

Great job! No new security vulnerabilities introduced 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]

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

[kingthorin]

If anyone sees a better way to do this (and not maintain two lists) lemme know.

[kingthorin]

Deconflicted

[kingthorin]

I believe this is ready for review now.

[thc202]

As we talked before the pre-checks were already being done, the claim that this reduces FPs and the referenced issue should be reviewed.

[kingthorin]

Adjusted.

The rule should could also be skipping js/css/binaryish, but we had previously said that should could be done in a separate PR.

... if necessary: #5824 (comment)

[thc202]

The shoulds in that sentence are not accurate.

[kingthorin]

Could 😀

When things sit for 6, 9, 12 months, don't be surprised if I'm inaccurate 😜 (Edit: To be clear, that's not blame, I know it's been waiting on me.)

[thc202]

I'm well aware of what I said, the split was to remove the resource exclusion from this PR (good, we don't want FNs). And the for now does not imply that resource exclusion should (or could, even) be done anyway. This is not a passive rule that can't discern cause and effect.

[kingthorin]

Oh okay, I wasn't looking at it that way. I just figured that the resource exclusions would be more efficient than the pre-check regexes. But now I think I see what you're getting at.
(Linking the comment was for me.)

Anyway this is no longer "fixing" the issue, and I've adjusted the changelog and title to not claim FP reduction.

[thc202]

Thank you!