diff --git a/addOns/ascanrules/CHANGELOG.md b/addOns/ascanrules/CHANGELOG.md index 3290b41cf80..7b63f4914bd 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 and pre-checks the original message to reduce false positives (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..e9c2f99d1b0 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 @@ -40,6 +40,7 @@ import org.parosproxy.paros.core.scanner.Category; import org.parosproxy.paros.network.HttpMessage; import org.zaproxy.addon.commonlib.CommonAlertTag; +import org.zaproxy.addon.commonlib.ResourceIdentificationUtils; import org.zaproxy.addon.commonlib.vulnerabilities.Vulnerabilities; import org.zaproxy.addon.commonlib.vulnerabilities.Vulnerability; import org.zaproxy.addon.network.common.ZapSocketTimeoutException; @@ -67,6 +68,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 +118,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 +180,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 */ @@ -221,6 +226,10 @@ public String getReference() { @Override public void scan(HttpMessage msg, String param, String value) { + if (!isGoodCandidate(getBaseMsg())) { + return; + } + try { // figure out how aggressively we should test int nixCount = 0; @@ -555,6 +564,18 @@ public void scan(HttpMessage msg, String param, String value) { } } + private static boolean isGoodCandidate(HttpMessage msg) { + if (ResourceIdentificationUtils.isJavaScript(msg) + || ResourceIdentificationUtils.isCss(msg) + || ResourceIdentificationUtils.responseContainsControlChars(msg)) { + + return false; + } + return DirNamesContentsMatcher.matchNixDirectories(msg.getResponseBody().toString()) == null + && DirNamesContentsMatcher.matchWinDirectories(msg.getResponseBody().toString()) + == null; + } + private boolean sendAndCheckPayload( String param, String newValue, ContentsMatcher contentsMatcher, int check) throws IOException { @@ -644,12 +665,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 + : DirNamesContentsMatcher.NIX_MATCHES)); + } + return builder; } @Override @@ -691,6 +723,24 @@ 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")); + private static final Pattern PROC_PATT = + Pattern.compile( + "(?:^|\\W)proc(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE); + private static final Pattern ETC_PATT = + Pattern.compile( + "(?:^|\\W)etc(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE); + private static final Pattern BOOT_PATT = + Pattern.compile( + "(?:^|\\W)boot(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE); + private static final Pattern TMP_PATT = + Pattern.compile( + "(?:^|\\W)tmp(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE); + private static final Pattern HOME_PATT = + Pattern.compile( + "(?:^|\\W)home(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE); + @Override public String match(String contents) { String result = matchNixDirectories(contents); @@ -701,36 +751,24 @@ public String match(String contents) { } private static String matchNixDirectories(String contents) { - Pattern procPattern = - Pattern.compile("(?:^|\\W)proc(?:\\W|$)", Pattern.CASE_INSENSITIVE); - Pattern etcPattern = Pattern.compile("(?:^|\\W)etc(?:\\W|$)", Pattern.CASE_INSENSITIVE); - Pattern bootPattern = - Pattern.compile("(?:^|\\W)boot(?:\\W|$)", Pattern.CASE_INSENSITIVE); - Pattern tmpPattern = Pattern.compile("(?:^|\\W)tmp(?:\\W|$)", Pattern.CASE_INSENSITIVE); - Pattern homePattern = - Pattern.compile("(?:^|\\W)home(?:\\W|$)", Pattern.CASE_INSENSITIVE); - - Matcher procMatcher = procPattern.matcher(contents); - Matcher etcMatcher = etcPattern.matcher(contents); - Matcher bootMatcher = bootPattern.matcher(contents); - Matcher tmpMatcher = tmpPattern.matcher(contents); - Matcher homeMatcher = homePattern.matcher(contents); - - if (procMatcher.find() - && etcMatcher.find() - && bootMatcher.find() - && tmpMatcher.find() - && homeMatcher.find()) { - return "etc"; + if (PROC_PATT.matcher(contents).find() + && ETC_PATT.matcher(contents).find() + && BOOT_PATT.matcher(contents).find() + && TMP_PATT.matcher(contents).find() + && HOME_PATT.matcher(contents).find()) { + 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..64ab6f45212 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; @@ -34,6 +35,7 @@ import org.apache.commons.lang3.ArrayUtils; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.EnumSource; import org.junit.jupiter.params.provider.ValueSource; import org.parosproxy.paros.core.scanner.Alert; @@ -42,6 +44,7 @@ import org.parosproxy.paros.core.scanner.Plugin.AttackStrength; import org.parosproxy.paros.network.HttpMalformedHeaderException; import org.parosproxy.paros.network.HttpMessage; +import org.parosproxy.paros.network.HttpResponseHeader; import org.zaproxy.addon.commonlib.CommonAlertTag; import org.zaproxy.zap.model.Tech; import org.zaproxy.zap.testutils.NanoServerHandler; @@ -163,6 +166,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 +191,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 +216,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 +296,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 +313,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 +334,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 @@ -356,6 +383,47 @@ void shouldNotAlertOnCheckFiveAtLowThresholdUnderInvalidConditions(String errorT 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 + @ValueSource( + strings = { + "etc root tmp bin boot dev home mnt opt proc", + "Program Files Users Windows" + }) + void shouldSkipIfResponseHasEvidenceToStartWith(String content) throws Exception { + // Given + HttpMessage msg = getHttpMessage("/?p=v"); + msg.setResponseBody(content); + rule.init(msg, parent); + // When + rule.scan(); + // Then + assertThat(httpMessagesSent, hasSize(equalTo(0))); + } + private abstract static class ListDirsOnAttack extends NanoServerHandler { private final String param;