zap-extensions
zap-extensions copied to clipboard
zaproxy
ascanrules: Path Traversal add details for dir match Alerts & reduce FPs
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 zaproxy/zaproxy#8379
Checklist
- [na] Update help
- [x] Update changelog
- [x] Run
./gradlew spotlessApplyfor code formatting - [x] Write tests
- [x] Check code coverage
- [x] Sign-off commits
- [x] Squash commits
- [x] Use a descriptive title
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.
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).
This rule doesn't seem to pre-check the response. I'll tackle that as well.
Checkmarx One – Scan Summary & Details – fb8ede2c-6d02-403e-afad-c80f2c638a79
Great job! No new security vulnerabilities introduced in this pull request
I've rebased this to current. I believe all the previous feedback has been addressed.
Please correct me if I'm wrong.
Rebased current, and reviewed previous comments. As far as I see they're all addressed.
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.
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?
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
As we talked before the pre-checks were already being done, the claim that this reduces FPs and the referenced issue should be reviewed.
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: https://github.com/zaproxy/zap-extensions/pull/5824#issuecomment-2711197366
Could :grinning:
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.)
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.
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.