Skip to content

Commit a6d3a94

Browse files
kingthorinkingthorin
committed
ascanrules: Path Traversal add details for dir match Alerts & reduce FPs
- 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. Signed-off-by: kingthorin <[email protected]>
1 parent aced9f3 commit a6d3a94

File tree

4 files changed

+138
-30
lines changed

4 files changed

+138
-30
lines changed

‎addOns/ascanrules/CHANGELOG.md

+2-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@ All notable changes to this add-on will be documented in this file.
44
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
55

66
## Unreleased
7-
7+
### Changed
8+
- 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).
89

910
## [70] - 2025-01-09
1011
### Changed

‎addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java

+67-29
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import org.parosproxy.paros.network.HttpMessage;
4444
import org.zaproxy.addon.commonlib.CommonAlertTag;
4545
import org.zaproxy.addon.commonlib.PolicyTag;
46+
import org.zaproxy.addon.commonlib.ResourceIdentificationUtils;
4647
import org.zaproxy.addon.commonlib.vulnerabilities.Vulnerabilities;
4748
import org.zaproxy.addon.commonlib.vulnerabilities.Vulnerability;
4849
import org.zaproxy.addon.network.common.ZapSocketTimeoutException;
@@ -81,6 +82,7 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
8182
*/
8283
private static final ContentsMatcher WIN_PATTERN =
8384
new PatternContentsMatcher(Pattern.compile("\\[drivers\\]"));
85+
private static final String WIN_DIR_EVIDENCE = "Windows";
8486
private static final String[] WIN_LOCAL_FILE_TARGETS = {
8587
// Absolute Windows file retrieval (we suppose C:\\)
8688
"c:/Windows/system.ini",
@@ -130,6 +132,7 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
130132
// Dot used to match 'x' or '!' (used in AIX)
131133
private static final ContentsMatcher NIX_PATTERN =
132134
new PatternContentsMatcher(Pattern.compile("root:.:0:0"));
135+
private static final String NIX_DIR_EVIDENCE = "etc";
133136
private static final String[] NIX_LOCAL_FILE_TARGETS = {
134137
// Absolute file retrieval
135138
"/etc/passwd",
@@ -191,6 +194,8 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
191194
*/
192195
private static final String[] LOCAL_FILE_RELATIVE_PREFIXES = {"", "/", "\\"};
193196

197+
private static final List<String> DIR_EVIDENCE_LIST =
198+
List.of(NIX_DIR_EVIDENCE, WIN_DIR_EVIDENCE);
194199
/*
195200
* details of the vulnerability which we are attempting to find
196201
*/
@@ -235,6 +240,10 @@ public String getReference() {
235240
@Override
236241
public void scan(HttpMessage msg, String param, String value) {
237242

243+
if (!isGoodCandidate(getBaseMsg())) {
244+
return;
245+
}
246+
238247
try {
239248
// figure out how aggressively we should test
240249
int nixCount = 0;
@@ -569,6 +578,18 @@ public void scan(HttpMessage msg, String param, String value) {
569578
}
570579
}
571580

581+
private static boolean isGoodCandidate(HttpMessage msg) {
582+
if (ResourceIdentificationUtils.isJavaScript(msg)
583+
|| ResourceIdentificationUtils.isCss(msg)
584+
|| ResourceIdentificationUtils.responseContainsControlChars(msg)) {
585+
586+
return false;
587+
}
588+
return DirNamesContentsMatcher.matchNixDirectories(msg.getResponseBody().toString()) == null
589+
&& DirNamesContentsMatcher.matchWinDirectories(msg.getResponseBody().toString())
590+
== null;
591+
}
592+
572593
private boolean sendAndCheckPayload(
573594
String param, String newValue, ContentsMatcher contentsMatcher, int check)
574595
throws IOException {
@@ -658,12 +679,23 @@ private AlertBuilder createUnmatchedAlert(String param, String attack) {
658679

659680
private AlertBuilder createMatchedAlert(
660681
String param, String attack, String evidence, int check) {
661-
return newAlert()
662-
.setConfidence(Alert.CONFIDENCE_MEDIUM)
663-
.setParam(param)
664-
.setAttack(attack)
665-
.setEvidence(evidence)
666-
.setAlertRef(getId() + "-" + check);
682+
AlertBuilder builder =
683+
newAlert()
684+
.setConfidence(Alert.CONFIDENCE_MEDIUM)
685+
.setParam(param)
686+
.setAttack(attack)
687+
.setEvidence(evidence)
688+
.setAlertRef(getId() + "-" + check);
689+
if (DIR_EVIDENCE_LIST.contains(evidence)) {
690+
builder.setOtherInfo(
691+
Constant.messages.getString(
692+
MESSAGE_PREFIX + "info",
693+
evidence,
694+
evidence.equals(WIN_DIR_EVIDENCE)
695+
? DirNamesContentsMatcher.WIN_MATCHES
696+
: DirNamesContentsMatcher.NIX_MATCHES));
697+
}
698+
return builder;
667699
}
668700

669701
@Override
@@ -705,6 +737,24 @@ public String match(String contents) {
705737

706738
private static class DirNamesContentsMatcher implements ContentsMatcher {
707739

740+
public static final String NIX_MATCHES =
741+
String.join(", ", List.of("proc", NIX_DIR_EVIDENCE, "boot", "tmp", "home"));
742+
private static final Pattern PROC_PATT =
743+
Pattern.compile(
744+
"(?:^|\\W)proc(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
745+
private static final Pattern ETC_PATT =
746+
Pattern.compile(
747+
"(?:^|\\W)etc(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
748+
private static final Pattern BOOT_PATT =
749+
Pattern.compile(
750+
"(?:^|\\W)boot(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
751+
private static final Pattern TMP_PATT =
752+
Pattern.compile(
753+
"(?:^|\\W)tmp(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
754+
private static final Pattern HOME_PATT =
755+
Pattern.compile(
756+
"(?:^|\\W)home(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
757+
708758
@Override
709759
public String match(String contents) {
710760
String result = matchNixDirectories(contents);
@@ -715,36 +765,24 @@ public String match(String contents) {
715765
}
716766

717767
private static String matchNixDirectories(String contents) {
718-
Pattern procPattern =
719-
Pattern.compile("(?:^|\\W)proc(?:\\W|$)", Pattern.CASE_INSENSITIVE);
720-
Pattern etcPattern = Pattern.compile("(?:^|\\W)etc(?:\\W|$)", Pattern.CASE_INSENSITIVE);
721-
Pattern bootPattern =
722-
Pattern.compile("(?:^|\\W)boot(?:\\W|$)", Pattern.CASE_INSENSITIVE);
723-
Pattern tmpPattern = Pattern.compile("(?:^|\\W)tmp(?:\\W|$)", Pattern.CASE_INSENSITIVE);
724-
Pattern homePattern =
725-
Pattern.compile("(?:^|\\W)home(?:\\W|$)", Pattern.CASE_INSENSITIVE);
726-
727-
Matcher procMatcher = procPattern.matcher(contents);
728-
Matcher etcMatcher = etcPattern.matcher(contents);
729-
Matcher bootMatcher = bootPattern.matcher(contents);
730-
Matcher tmpMatcher = tmpPattern.matcher(contents);
731-
Matcher homeMatcher = homePattern.matcher(contents);
732-
733-
if (procMatcher.find()
734-
&& etcMatcher.find()
735-
&& bootMatcher.find()
736-
&& tmpMatcher.find()
737-
&& homeMatcher.find()) {
738-
return "etc";
768+
if (PROC_PATT.matcher(contents).find()
769+
&& ETC_PATT.matcher(contents).find()
770+
&& BOOT_PATT.matcher(contents).find()
771+
&& TMP_PATT.matcher(contents).find()
772+
&& HOME_PATT.matcher(contents).find()) {
773+
return NIX_DIR_EVIDENCE;
739774
}
740775

741776
return null;
742777
}
743778

779+
public static final String WIN_MATCHES =
780+
String.join(", ", List.of(WIN_DIR_EVIDENCE, "Program Files"));
781+
744782
private static String matchWinDirectories(String contents) {
745-
if (contents.contains("Windows")
783+
if (contents.contains(WIN_DIR_EVIDENCE)
746784
&& Pattern.compile("Program\\sFiles").matcher(contents).find()) {
747-
return "Windows";
785+
return WIN_DIR_EVIDENCE;
748786
}
749787

750788
return null;

‎addOns/ascanrules/src/main/resources/org/zaproxy/zap/extension/ascanrules/resources/Messages.properties

+1
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ ascanrules.parametertamper.desc = Parameter manipulation caused an error page or
114114
ascanrules.parametertamper.name = Parameter Tampering
115115
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.
116116

117+
ascanrules.pathtraversal.info = While the evidence field indicates {0}, the rule actually checked that the response contains matches for all of the following: {1}.
117118
ascanrules.pathtraversal.name = Path Traversal
118119

119120
ascanrules.payloader.desc = Provides support for custom payloads in scan rules.

‎addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java

+68
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
import static fi.iki.elonen.NanoHTTPD.newFixedLengthResponse;
2323
import static org.hamcrest.MatcherAssert.assertThat;
24+
import static org.hamcrest.Matchers.emptyString;
2425
import static org.hamcrest.Matchers.equalTo;
2526
import static org.hamcrest.Matchers.greaterThan;
2627
import static org.hamcrest.Matchers.hasSize;
@@ -34,6 +35,7 @@
3435
import org.apache.commons.lang3.ArrayUtils;
3536
import org.junit.jupiter.api.Test;
3637
import org.junit.jupiter.params.ParameterizedTest;
38+
import org.junit.jupiter.params.provider.CsvSource;
3739
import org.junit.jupiter.params.provider.EnumSource;
3840
import org.junit.jupiter.params.provider.ValueSource;
3941
import org.parosproxy.paros.core.scanner.Alert;
@@ -42,6 +44,7 @@
4244
import org.parosproxy.paros.core.scanner.Plugin.AttackStrength;
4345
import org.parosproxy.paros.network.HttpMalformedHeaderException;
4446
import org.parosproxy.paros.network.HttpMessage;
47+
import org.parosproxy.paros.network.HttpResponseHeader;
4548
import org.zaproxy.addon.commonlib.CommonAlertTag;
4649
import org.zaproxy.addon.commonlib.PolicyTag;
4750
import org.zaproxy.zap.model.Tech;
@@ -169,6 +172,13 @@ void shouldAlertIfAttackResponseListsWindowsDirectories() throws Exception {
169172
assertThat(alertsRaised.get(0).getAttack(), is(equalTo("c:/")));
170173
assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH)));
171174
assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM)));
175+
assertThat(
176+
alertsRaised.get(0).getOtherInfo(),
177+
is(
178+
equalTo(
179+
"While the evidence field indicates Windows, the rule actually "
180+
+ "checked that the response contains matches for all of the "
181+
+ "following: Windows, Program Files.")));
172182
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3")));
173183
}
174184

@@ -187,6 +197,13 @@ void shouldAlertIfAttackResponseListsLinuxDirectories() throws Exception {
187197
assertThat(alertsRaised.get(0).getAttack(), is(equalTo("/")));
188198
assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH)));
189199
assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM)));
200+
assertThat(
201+
alertsRaised.get(0).getOtherInfo(),
202+
is(
203+
equalTo(
204+
"While the evidence field indicates etc, the rule actually "
205+
+ "checked that the response contains matches for all of the "
206+
+ "following: proc, etc, boot, tmp, home.")));
190207
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3")));
191208
}
192209

@@ -205,6 +222,13 @@ void shouldAlertIfAttackResponseListsLinuxDirectoriesInPlainText() throws Except
205222
assertThat(alertsRaised.get(0).getAttack(), is(equalTo("/")));
206223
assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH)));
207224
assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM)));
225+
assertThat(
226+
alertsRaised.get(0).getOtherInfo(),
227+
is(
228+
equalTo(
229+
"While the evidence field indicates etc, the rule actually "
230+
+ "checked that the response contains matches for all of the "
231+
+ "following: proc, etc, boot, tmp, home.")));
208232
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3")));
209233
}
210234

@@ -278,6 +302,7 @@ void shouldRaiseAlertIfResponseHasPasswdFileContentAndPayloadIsNullByteBased()
278302
// Then
279303
assertThat(alertsRaised, hasSize(1));
280304
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-2")));
305+
assertThat(alertsRaised.get(0).getOtherInfo(), is(emptyString()));
281306
}
282307

283308
@Test
@@ -294,6 +319,7 @@ void shouldRaiseAlertIfResponseHasSystemINIFileContentAndPayloadIsNullByteBased(
294319
// Then
295320
assertThat(alertsRaised, hasSize(1));
296321
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-1")));
322+
assertThat(alertsRaised.get(0).getOtherInfo(), is(emptyString()));
297323
}
298324

299325
@ParameterizedTest
@@ -314,6 +340,7 @@ void shouldAlertOnCheckFiveBelowHighThresholdUnderValidConditions(AlertThreshold
314340
assertThat(alertsRaised, hasSize(1));
315341
assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_LOW)));
316342
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-5")));
343+
assertThat(alertsRaised.get(0).getOtherInfo(), is(emptyString()));
317344
}
318345

319346
@Test
@@ -362,6 +389,47 @@ void shouldNotAlertOnCheckFiveAtLowThresholdUnderInvalidConditions(String errorT
362389
assertThat(alertsRaised, hasSize(0));
363390
}
364391

392+
@ParameterizedTest
393+
@ValueSource(strings = {"/styles.css", "/code.js"})
394+
void shouldSkipIfReqPathIsCssOrJs(String path) throws Exception {
395+
// Given
396+
rule.init(getHttpMessage(path + "?p=v"), parent);
397+
// When
398+
rule.scan();
399+
// Then
400+
assertThat(httpMessagesSent, hasSize(equalTo(0)));
401+
}
402+
403+
@ParameterizedTest
404+
@CsvSource({"/styles/, text/css", "/code, text/javascript"})
405+
void shouldSkipIfResponseIsCssOfJs(String path, String contentType) throws Exception {
406+
// Given
407+
HttpMessage msg = getHttpMessage(path + "?p=v");
408+
msg.getResponseHeader().setHeader(HttpResponseHeader.CONTENT_TYPE, contentType);
409+
rule.init(msg, parent);
410+
// When
411+
rule.scan();
412+
// Then
413+
assertThat(httpMessagesSent, hasSize(equalTo(0)));
414+
}
415+
416+
@ParameterizedTest
417+
@ValueSource(
418+
strings = {
419+
"etc root tmp bin boot dev home mnt opt proc",
420+
"Program Files Users Windows"
421+
})
422+
void shouldSkipIfResponseHasEvidenceToStartWith(String content) throws Exception {
423+
// Given
424+
HttpMessage msg = getHttpMessage("/?p=v");
425+
msg.setResponseBody(content);
426+
rule.init(msg, parent);
427+
// When
428+
rule.scan();
429+
// Then
430+
assertThat(httpMessagesSent, hasSize(equalTo(0)));
431+
}
432+
365433
private abstract static class ListDirsOnAttack extends NanoServerHandler {
366434

367435
private final String param;

0 commit comments

Comments
 (0)