Skip to content

Commit fb7073e

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 <kingthorin@users.noreply.github.com>
1 parent 3159ed2 commit fb7073e

File tree

4 files changed

+154
-37
lines changed

4 files changed

+154
-37
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
## [71] - 2025-03-04
1011
### Fixed

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

+75-34
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
8181
*/
8282
private static final ContentsMatcher WIN_PATTERN =
8383
new PatternContentsMatcher(Pattern.compile("\\[drivers\\]"));
84+
private static final String WIN_DIR_EVIDENCE = "Windows";
8485
private static final String[] WIN_LOCAL_FILE_TARGETS = {
8586
// Absolute Windows file retrieval (we suppose C:\\)
8687
"c:/Windows/system.ini",
@@ -130,6 +131,7 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
130131
// Dot used to match 'x' or '!' (used in AIX)
131132
private static final ContentsMatcher NIX_PATTERN =
132133
new PatternContentsMatcher(Pattern.compile("root:.:0:0"));
134+
private static final String NIX_DIR_EVIDENCE = "etc";
133135
private static final String[] NIX_LOCAL_FILE_TARGETS = {
134136
// Absolute file retrieval
135137
"/etc/passwd",
@@ -191,6 +193,8 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
191193
*/
192194
private static final String[] LOCAL_FILE_RELATIVE_PREFIXES = {"", "/", "\\"};
193195

196+
private static final List<String> DIR_EVIDENCE_LIST =
197+
List.of(NIX_DIR_EVIDENCE, WIN_DIR_EVIDENCE);
194198
/*
195199
* details of the vulnerability which we are attempting to find
196200
*/
@@ -341,7 +345,6 @@ public void scan(HttpMessage msg, String param, String value) {
341345

342346
// Check 2: Start detection for *NIX patterns
343347
if (inScope(Tech.Linux) || inScope(Tech.MacOS)) {
344-
345348
for (int h = 0; h < nixCount; h++) {
346349

347350
// Check if a there was a finding or the scan has been stopped
@@ -382,8 +385,11 @@ public void scan(HttpMessage msg, String param, String value) {
382385

383386
// Check 3: Detect if this page is a directory browsing component
384387
if (inScope(Tech.Linux) || inScope(Tech.MacOS)) {
385-
for (int h = 0; h < nixDirCount; h++) {
386-
388+
int h = 0;
389+
if (!isGoodCandidate(getBaseMsg(), Tech.Linux)) {
390+
h = nixDirCount;
391+
}
392+
for (; h < nixDirCount; h++) {
387393
// Check if a there was a finding or the scan has been stopped
388394
// if yes dispose resources and exit
389395
if (sendAndCheckPayload(param, NIX_LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3)
@@ -395,7 +401,11 @@ public void scan(HttpMessage msg, String param, String value) {
395401
}
396402
}
397403
if (inScope(Tech.Windows)) {
398-
for (int h = 0; h < winDirCount; h++) {
404+
int h = 0;
405+
if (!isGoodCandidate(getBaseMsg(), Tech.Windows)) {
406+
h = winDirCount;
407+
}
408+
for (; h < winDirCount; h++) {
399409
if (sendAndCheckPayload(param, WIN_LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3)
400410
|| isStop()) {
401411
// Dispose all resources
@@ -569,6 +579,19 @@ public void scan(HttpMessage msg, String param, String value) {
569579
}
570580
}
571581

582+
private static boolean isGoodCandidate(HttpMessage msg, Tech targetTech) {
583+
// It is a good candidate if it doesn't have evidence to start with
584+
if (targetTech == Tech.Windows) {
585+
return DirNamesContentsMatcher.matchWinDirectories(msg.getResponseBody().toString())
586+
== null;
587+
}
588+
if (targetTech == Tech.Linux) {
589+
return DirNamesContentsMatcher.matchNixDirectories(msg.getResponseBody().toString())
590+
== null;
591+
}
592+
return false;
593+
}
594+
572595
private boolean sendAndCheckPayload(
573596
String param, String newValue, ContentsMatcher contentsMatcher, int check)
574597
throws IOException {
@@ -658,12 +681,23 @@ private AlertBuilder createUnmatchedAlert(String param, String attack) {
658681

659682
private AlertBuilder createMatchedAlert(
660683
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);
684+
AlertBuilder builder =
685+
newAlert()
686+
.setConfidence(Alert.CONFIDENCE_MEDIUM)
687+
.setParam(param)
688+
.setAttack(attack)
689+
.setEvidence(evidence)
690+
.setAlertRef(getId() + "-" + check);
691+
if (DIR_EVIDENCE_LIST.contains(evidence)) {
692+
builder.setOtherInfo(
693+
Constant.messages.getString(
694+
MESSAGE_PREFIX + "info",
695+
evidence,
696+
evidence.equals(WIN_DIR_EVIDENCE)
697+
? DirNamesContentsMatcher.WIN_MATCHES
698+
: DirNamesContentsMatcher.NIX_MATCHES));
699+
}
700+
return builder;
667701
}
668702

669703
@Override
@@ -705,6 +739,26 @@ public String match(String contents) {
705739

706740
private static class DirNamesContentsMatcher implements ContentsMatcher {
707741

742+
private static final String NIX_MATCHES =
743+
String.join(", ", List.of("proc", NIX_DIR_EVIDENCE, "boot", "tmp", "home"));
744+
private static final String WIN_MATCHES =
745+
String.join(", ", List.of(WIN_DIR_EVIDENCE, "Program Files"));
746+
private static final Pattern PROC_PATT =
747+
Pattern.compile(
748+
"(?:^|\\W)proc(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
749+
private static final Pattern ETC_PATT =
750+
Pattern.compile(
751+
"(?:^|\\W)etc(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
752+
private static final Pattern BOOT_PATT =
753+
Pattern.compile(
754+
"(?:^|\\W)boot(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
755+
private static final Pattern TMP_PATT =
756+
Pattern.compile(
757+
"(?:^|\\W)tmp(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
758+
private static final Pattern HOME_PATT =
759+
Pattern.compile(
760+
"(?:^|\\W)home(?:\\W|$)", Pattern.CASE_INSENSITIVE | Pattern.MULTILINE);
761+
708762
@Override
709763
public String match(String contents) {
710764
String result = matchNixDirectories(contents);
@@ -715,36 +769,23 @@ public String match(String contents) {
715769
}
716770

717771
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";
772+
if (PROC_PATT.matcher(contents).find()
773+
&& ETC_PATT.matcher(contents).find()
774+
&& BOOT_PATT.matcher(contents).find()
775+
&& TMP_PATT.matcher(contents).find()
776+
&& HOME_PATT.matcher(contents).find()) {
777+
return NIX_DIR_EVIDENCE;
739778
}
740779

741780
return null;
742781
}
743782

744783
private static String matchWinDirectories(String contents) {
745-
if (contents.contains("Windows")
746-
&& Pattern.compile("Program\\sFiles").matcher(contents).find()) {
747-
return "Windows";
784+
if (contents.contains(WIN_DIR_EVIDENCE)
785+
&& Pattern.compile("Program\\sFiles", Pattern.CASE_INSENSITIVE)
786+
.matcher(contents)
787+
.find()) {
788+
return WIN_DIR_EVIDENCE;
748789
}
749790

750791
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

+76-2
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;
@@ -31,10 +32,14 @@
3132
import fi.iki.elonen.NanoHTTPD.Response;
3233
import java.util.List;
3334
import java.util.Map;
35+
import java.util.stream.Stream;
3436
import org.apache.commons.lang3.ArrayUtils;
3537
import org.junit.jupiter.api.Test;
3638
import org.junit.jupiter.params.ParameterizedTest;
39+
import org.junit.jupiter.params.provider.Arguments;
40+
import org.junit.jupiter.params.provider.CsvSource;
3741
import org.junit.jupiter.params.provider.EnumSource;
42+
import org.junit.jupiter.params.provider.MethodSource;
3843
import org.junit.jupiter.params.provider.ValueSource;
3944
import org.parosproxy.paros.core.scanner.Alert;
4045
import org.parosproxy.paros.core.scanner.Plugin;
@@ -169,6 +174,13 @@ void shouldAlertIfAttackResponseListsWindowsDirectories() throws Exception {
169174
assertThat(alertsRaised.get(0).getAttack(), is(equalTo("c:/")));
170175
assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH)));
171176
assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM)));
177+
assertThat(
178+
alertsRaised.get(0).getOtherInfo(),
179+
is(
180+
equalTo(
181+
"While the evidence field indicates Windows, the rule actually "
182+
+ "checked that the response contains matches for all of the "
183+
+ "following: Windows, Program Files.")));
172184
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3")));
173185
}
174186

@@ -180,13 +192,20 @@ void shouldAlertIfAttackResponseListsLinuxDirectories() throws Exception {
180192
// When
181193
rule.scan();
182194
// Then
183-
assertThat(httpMessagesSent, hasSize(greaterThan(1)));
195+
assertThat(httpMessagesSent, hasSize(equalTo(7)));
184196
assertThat(alertsRaised, hasSize(1));
185197
assertThat(alertsRaised.get(0).getEvidence(), is(equalTo("etc")));
186198
assertThat(alertsRaised.get(0).getParam(), is(equalTo("p")));
187199
assertThat(alertsRaised.get(0).getAttack(), is(equalTo("/")));
188200
assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH)));
189201
assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM)));
202+
assertThat(
203+
alertsRaised.get(0).getOtherInfo(),
204+
is(
205+
equalTo(
206+
"While the evidence field indicates etc, the rule actually "
207+
+ "checked that the response contains matches for all of the "
208+
+ "following: proc, etc, boot, tmp, home.")));
190209
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3")));
191210
}
192211

@@ -198,13 +217,20 @@ void shouldAlertIfAttackResponseListsLinuxDirectoriesInPlainText() throws Except
198217
// When
199218
rule.scan();
200219
// Then
201-
assertThat(httpMessagesSent, hasSize(greaterThan(1)));
220+
assertThat(httpMessagesSent, hasSize(equalTo(7)));
202221
assertThat(alertsRaised, hasSize(1));
203222
assertThat(alertsRaised.get(0).getEvidence(), is(equalTo("etc")));
204223
assertThat(alertsRaised.get(0).getParam(), is(equalTo("p")));
205224
assertThat(alertsRaised.get(0).getAttack(), is(equalTo("/")));
206225
assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH)));
207226
assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM)));
227+
assertThat(
228+
alertsRaised.get(0).getOtherInfo(),
229+
is(
230+
equalTo(
231+
"While the evidence field indicates etc, the rule actually "
232+
+ "checked that the response contains matches for all of the "
233+
+ "following: proc, etc, boot, tmp, home.")));
208234
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3")));
209235
}
210236

@@ -278,6 +304,7 @@ void shouldRaiseAlertIfResponseHasPasswdFileContentAndPayloadIsNullByteBased()
278304
// Then
279305
assertThat(alertsRaised, hasSize(1));
280306
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-2")));
307+
assertThat(alertsRaised.get(0).getOtherInfo(), is(emptyString()));
281308
}
282309

283310
@Test
@@ -294,6 +321,7 @@ void shouldRaiseAlertIfResponseHasSystemINIFileContentAndPayloadIsNullByteBased(
294321
// Then
295322
assertThat(alertsRaised, hasSize(1));
296323
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-1")));
324+
assertThat(alertsRaised.get(0).getOtherInfo(), is(emptyString()));
297325
}
298326

299327
@ParameterizedTest
@@ -314,6 +342,7 @@ void shouldAlertOnCheckFiveBelowHighThresholdUnderValidConditions(AlertThreshold
314342
assertThat(alertsRaised, hasSize(1));
315343
assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_LOW)));
316344
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-5")));
345+
assertThat(alertsRaised.get(0).getOtherInfo(), is(emptyString()));
317346
}
318347

319348
@Test
@@ -362,6 +391,51 @@ void shouldNotAlertOnCheckFiveAtLowThresholdUnderInvalidConditions(String errorT
362391
assertThat(alertsRaised, hasSize(0));
363392
}
364393

394+
@ParameterizedTest
395+
@CsvSource({
396+
// Windows will always happen before ignoring Linux, if only Linux pre-check matches
397+
"foo etc root tmp bin boot dev home mnt opt proc bar, 11",
398+
"foo Program Files Users Windows bar, 11",
399+
"foo etc root tmp bin boot dev home mnt opt proc Program Files Users Windows bar, 11"
400+
})
401+
void shouldSkipDirChecksIfResponseHasDirEvidenceToStartWith(String content, int expected)
402+
throws Exception {
403+
// Given
404+
HttpMessage msg = getHttpMessage("/?p=v");
405+
msg.setResponseBody(content);
406+
rule.init(msg, parent);
407+
// When
408+
rule.scan();
409+
// Then
410+
assertThat(httpMessagesSent, hasSize(equalTo(expected)));
411+
}
412+
413+
private static Stream<Arguments> handlersProvider() {
414+
return Stream.of(
415+
// Content is opposite of handler to ensure no skippage
416+
Arguments.of(
417+
new ListWinDirsOnAttack("/", "p", "c:/"),
418+
"foo etc root tmp bin boot dev home mnt opt proc bar"),
419+
Arguments.of(
420+
new ListLinuxDirsOnAttackPlainText("/", "p", "/"),
421+
"foo Program Files Users Windows bar"));
422+
}
423+
424+
@ParameterizedTest
425+
@MethodSource("handlersProvider")
426+
void shouldNotSkipIfGoodCandidateForOnlyOneTech(NanoServerHandler handler, String body)
427+
throws Exception {
428+
// Given
429+
nano.addHandler(handler);
430+
HttpMessage msg = getHttpMessage("/?p=v");
431+
msg.setResponseBody(body);
432+
rule.init(msg, parent);
433+
// When
434+
rule.scan();
435+
// Then
436+
assertThat(httpMessagesSent, hasSize(greaterThan(2)));
437+
}
438+
365439
private abstract static class ListDirsOnAttack extends NanoServerHandler {
366440

367441
private final String param;

0 commit comments

Comments
 (0)