Skip to content

Commit

Permalink
ascanrules: Path Traversal add Other Info for directory match Alerts
Browse files Browse the repository at this point in the history 
- 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 <kingthorin@users.noreply.github.com>
  • Loading branch information
@kingthorin
kingthorin committed Oct 19, 2024
1 parent c139f92 commit 6cdb014
Show file tree
Hide file tree
Showing 4 changed files with 144 additions and 31 deletions.
1 change: 1 addition & 0 deletions addOns/ascanrules/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 response to reduce false positives (Issue 8379).

### Fixed
- Added more checks for valid .htaccess files to reduce false positives (Issue 7632).
Expand Down
102 changes: 72 additions & 30 deletions .../ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.function.Predicate;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import org.apache.commons.httpclient.URIException;
Expand All @@ -40,6 +41,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;
Expand Down Expand Up @@ -67,6 +69,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",
Expand Down Expand Up @@ -116,6 +119,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",
Expand Down Expand Up @@ -177,6 +181,8 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
*/
private static final String[] LOCAL_FILE_RELATIVE_PREFIXES = {"", "/", "\\"};

private static final List<String> DIR_EVIDENCE_LIST =
List.of(NIX_DIR_EVIDENCE, WIN_DIR_EVIDENCE);
/*
* details of the vulnerability which we are attempting to find
*/
Expand Down Expand Up @@ -221,6 +227,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;
Expand Down Expand Up @@ -555,6 +565,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 {
Expand Down Expand Up @@ -644,12 +666,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
Expand Down Expand Up @@ -691,6 +724,32 @@ 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);

private static final Predicate<String> CHECK_NIX =
content ->
PROC_PATT.matcher(content).find()
&& ETC_PATT.matcher(content).find()
&& BOOT_PATT.matcher(content).find()
&& TMP_PATT.matcher(content).find()
&& HOME_PATT.matcher(content).find();

@Override
public String match(String contents) {
String result = matchNixDirectories(contents);
Expand All @@ -701,36 +760,19 @@ 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 (CHECK_NIX.test(contents)) {
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;
Expand Down
1 change: 1 addition & 0 deletions ...les/src/main/resources/org/zaproxy/zap/extension/ascanrules/resources/Messages.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
71 changes: 70 additions & 1 deletion ...les/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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")));
}

Expand All @@ -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")));
}

Expand All @@ -199,14 +216,22 @@ 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")));
}

@Test
void shouldNotAlertIfAttackResponseListsHasFalsePositivePattern() throws Exception {
// Given
nano.addHandler(new ListFalsePositiveDirsOnAttack("/", "p", "/"));
rule.init(getHttpMessage("/?p=v"), parent);
HttpMessage msg = getHttpMessage("/?p=v");
rule.init(msg, parent);
// When
rule.scan();
// Then
Expand Down Expand Up @@ -272,6 +297,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
Expand All @@ -288,6 +314,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
Expand All @@ -308,6 +335,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
Expand Down Expand Up @@ -356,6 +384,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;
Expand Down

0 comments on commit 6cdb014

Please sign in to comment.