diff --git a/addOns/ascanrules/CHANGELOG.md b/addOns/ascanrules/CHANGELOG.md index 3290b41cf80..3c31129c3e1 100644 --- a/addOns/ascanrules/CHANGELOG.md +++ b/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 (Issue 8379). ### Fixed - Added more checks for valid .htaccess files to reduce false positives (Issue 7632). 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 18888009c67..bbaf78d54e1 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 @@ -67,6 +67,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 +117,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", @@ -644,12 +646,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 (List.of(NIX_DIR_EVIDENCE, WIN_DIR_EVIDENCE).contains(evidence)) { + builder.setOtherInfo( + Constant.messages.getString( + MESSAGE_PREFIX + "info", + evidence, + evidence.equals(WIN_DIR_EVIDENCE) + ? DirNamesContentsMatcher.WIN_MATCHES.toString() + : DirNamesContentsMatcher.NIX_MATCHES.toString())); + } + return builder; } @Override @@ -691,6 +704,9 @@ public String match(String contents) { private static class DirNamesContentsMatcher implements ContentsMatcher { + public static final List NIX_MATCHES = + List.of("proc", NIX_DIR_EVIDENCE, "boot", "tmp", "home"); + @Override public String match(String contents) { String result = matchNixDirectories(contents); @@ -721,16 +737,18 @@ private static String matchNixDirectories(String contents) { && bootMatcher.find() && tmpMatcher.find() && homeMatcher.find()) { - return "etc"; + return NIX_DIR_EVIDENCE; } return null; } + public static final List WIN_MATCHES = 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; 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 index 234beef424d..c0575974d7c 100644 --- 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 @@ -113,6 +113,7 @@ ascanrules.paddingoracle.soln = Update the affected server software, or modify t ascanrules.parametertamper.desc = Parameter manipulation caused an error page or Java stack trace to be displayed. This indicated lack of exception handling and potential areas for further exploit. 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 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 8babcb13726..b7d333dcc32 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 @@ -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; @@ -163,6 +164,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 +189,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 +214,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 +294,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 +311,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 +332,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