From 21096ab4a634ca5177bb6839e60473e8d2ef9b87 Mon Sep 17 00:00:00 2001 From: kingthorin Date: Thu, 17 Oct 2024 09:55:19 -0400 Subject: [PATCH] ascanrules: Path Traversal add Other Info for directory match Alerts - 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. Signed-off-by: kingthorin --- addOns/ascanrules/CHANGELOG.md | 1 + .../ascanrules/PathTraversalScanRule.java | 39 ++++++++++++++----- .../ascanrules/resources/Messages.properties | 1 + .../PathTraversalScanRuleUnitTest.java | 25 ++++++++++++ 4 files changed, 57 insertions(+), 9 deletions(-) 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..a643e45ea83 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", @@ -177,6 +179,8 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin */ private static final String[] LOCAL_FILE_RELATIVE_PREFIXES = {"", "/", "\\"}; + private static final List DIR_EVIDENCE_LIST = + List.of(NIX_DIR_EVIDENCE, WIN_DIR_EVIDENCE); /* * details of the vulnerability which we are attempting to find */ @@ -644,12 +648,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.toString() + : DirNamesContentsMatcher.NIX_MATCHES.toString())); + } + return builder; } @Override @@ -691,6 +706,9 @@ 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")); + @Override public String match(String contents) { String result = matchNixDirectories(contents); @@ -721,16 +739,19 @@ private static String matchNixDirectories(String contents) { && bootMatcher.find() && tmpMatcher.find() && homeMatcher.find()) { - return "etc"; + 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; 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..8f2de634a7b 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 @@ -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. 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..1986c17f3f3 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