Skip to content

Commit d8bc6a1

Browse files
thc202thc202
authored
Merge pull request #5824 from kingthorin/pathtrav-details
ascanrules: Path Traversal add details for dir match Alerts
2 parents 7bed6b9 + a2d4edd commit d8bc6a1

File tree

4 files changed

+179
-40
lines changed

4 files changed

+179
-40
lines changed

‎addOns/ascanrules/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
1818
- The External Redirect scan rule payload were slightly re-ordered to prioritize HTTPS variants.
1919
- For Alerts raised by the SQL Injection scan rules the Attack field values are now simply the payload, not an assembled description.
2020
- The Cross Site Scripting (Reflected) scan rule was updated to address potential false negatives when the injection context is a tag name and there is some filtering.
21+
- The Path Traversal scan rule now includes further details when directory matches are made (Issue 8379).
2122

2223
### Added
2324
- Rules (as applicable) have been tagged in relation to HIPAA and PCI DSS.

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

Lines changed: 66 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
8484
*/
8585
private static final ContentsMatcher WIN_PATTERN =
8686
new PatternContentsMatcher(Pattern.compile("\\[drivers\\]"));
87+
private static final String WIN_DIR_EVIDENCE = "Windows";
8788
private static final String[] WIN_LOCAL_FILE_TARGETS = {
8889
// Absolute Windows file retrieval (we suppose C:\\)
8990
"c:/Windows/system.ini",
@@ -133,6 +134,7 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
133134
// Dot used to match 'x' or '!' (used in AIX)
134135
private static final ContentsMatcher NIX_PATTERN =
135136
new PatternContentsMatcher(Pattern.compile("root:.:0:0"));
137+
private static final String NIX_DIR_EVIDENCE = "etc";
136138
private static final String[] NIX_LOCAL_FILE_TARGETS = {
137139
// Absolute file retrieval
138140
"/etc/passwd",
@@ -158,10 +160,9 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
158160
// ..%252F..%252F..%252F..%252F..%252F..%252F..%252F..%252F..%252F..%252Fetc%252Fpasswd%2500.jpg
159161
};
160162

161-
/*
162-
* Windows/Unix/Linux/etc. local directory targets and detection pattern
163-
*/
164-
private static final ContentsMatcher DIR_PATTERN = new DirNamesContentsMatcher();
163+
private static final ContentsMatcher NIX_DIR_MATCHER = new DirNamesContentsMatcher(Tech.Linux);
164+
private static final ContentsMatcher WIN_DIR_MATCHER =
165+
new DirNamesContentsMatcher(Tech.Windows);
165166
private static final String[] WIN_LOCAL_DIR_TARGETS = {
166167
"c:/",
167168
"c:\\",
@@ -194,6 +195,8 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin
194195
*/
195196
private static final String[] LOCAL_FILE_RELATIVE_PREFIXES = {"", "/", "\\"};
196197

198+
private static final List<String> DIR_EVIDENCE_LIST =
199+
List.of(NIX_DIR_EVIDENCE, WIN_DIR_EVIDENCE);
197200
/*
198201
* details of the vulnerability which we are attempting to find
199202
*/
@@ -389,7 +392,7 @@ public void scan(HttpMessage msg, String param, String value) {
389392

390393
// Check if a there was a finding or the scan has been stopped
391394
// if yes dispose resources and exit
392-
if (sendAndCheckPayload(param, NIX_LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3)
395+
if (sendAndCheckPayload(param, NIX_LOCAL_DIR_TARGETS[h], NIX_DIR_MATCHER, 3)
393396
|| isStop()) {
394397
// Dispose all resources
395398
// Exit the scan rule
@@ -399,7 +402,7 @@ public void scan(HttpMessage msg, String param, String value) {
399402
}
400403
if (inScope(Tech.Windows)) {
401404
for (int h = 0; h < winDirCount; h++) {
402-
if (sendAndCheckPayload(param, WIN_LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3)
405+
if (sendAndCheckPayload(param, WIN_LOCAL_DIR_TARGETS[h], WIN_DIR_MATCHER, 3)
403406
|| isStop()) {
404407
// Dispose all resources
405408
// Exit the scan rule
@@ -661,12 +664,23 @@ private AlertBuilder createUnmatchedAlert(String param, String attack) {
661664

662665
private AlertBuilder createMatchedAlert(
663666
String param, String attack, String evidence, int check) {
664-
return newAlert()
665-
.setConfidence(Alert.CONFIDENCE_MEDIUM)
666-
.setParam(param)
667-
.setAttack(attack)
668-
.setEvidence(evidence)
669-
.setAlertRef(getId() + "-" + check);
667+
AlertBuilder builder =
668+
newAlert()
669+
.setConfidence(Alert.CONFIDENCE_MEDIUM)
670+
.setParam(param)
671+
.setAttack(attack)
672+
.setEvidence(evidence)
673+
.setAlertRef(getId() + "-" + check);
674+
if (DIR_EVIDENCE_LIST.contains(evidence)) {
675+
builder.setOtherInfo(
676+
Constant.messages.getString(
677+
MESSAGE_PREFIX + "info",
678+
evidence,
679+
evidence.equals(WIN_DIR_EVIDENCE)
680+
? DirNamesContentsMatcher.WIN_MATCHES
681+
: DirNamesContentsMatcher.NIX_MATCHES));
682+
}
683+
return builder;
670684
}
671685

672686
@Override
@@ -708,49 +722,61 @@ public String match(String contents) {
708722

709723
private static class DirNamesContentsMatcher implements ContentsMatcher {
710724

725+
private static final String NIX_MATCHES =
726+
String.join(", ", List.of("proc", NIX_DIR_EVIDENCE, "boot", "tmp", "home"));
727+
728+
private static final Pattern PROC_PATT = createNixPattern("proc");
729+
private static final Pattern ETC_PATT = createNixPattern("etc");
730+
private static final Pattern BOOT_PATT = createNixPattern("boot");
731+
private static final Pattern TMP_PATT = createNixPattern("tmp");
732+
private static final Pattern HOME_PATT = createNixPattern("home");
733+
734+
private static final String WIN_MATCHES =
735+
String.join(", ", List.of(WIN_DIR_EVIDENCE, "Program Files"));
736+
737+
private static final Pattern PROGRAM_FILES_PATT =
738+
Pattern.compile("Program\\sFiles", Pattern.CASE_INSENSITIVE);
739+
740+
private final Tech tech;
741+
742+
public DirNamesContentsMatcher(Tech tech) {
743+
this.tech = tech;
744+
}
745+
711746
@Override
712747
public String match(String contents) {
713-
String result = matchNixDirectories(contents);
714-
if (result != null) {
715-
return result;
748+
if (this.tech == Tech.Linux) {
749+
return matchNixDirectories(contents);
716750
}
717-
return matchWinDirectories(contents);
751+
if (this.tech == Tech.Windows) {
752+
return matchWinDirectories(contents);
753+
}
754+
return null;
718755
}
719756

720757
private static String matchNixDirectories(String contents) {
721-
Pattern procPattern =
722-
Pattern.compile("(?:^|\\W)proc(?:\\W|$)", Pattern.CASE_INSENSITIVE);
723-
Pattern etcPattern = Pattern.compile("(?:^|\\W)etc(?:\\W|$)", Pattern.CASE_INSENSITIVE);
724-
Pattern bootPattern =
725-
Pattern.compile("(?:^|\\W)boot(?:\\W|$)", Pattern.CASE_INSENSITIVE);
726-
Pattern tmpPattern = Pattern.compile("(?:^|\\W)tmp(?:\\W|$)", Pattern.CASE_INSENSITIVE);
727-
Pattern homePattern =
728-
Pattern.compile("(?:^|\\W)home(?:\\W|$)", Pattern.CASE_INSENSITIVE);
729-
730-
Matcher procMatcher = procPattern.matcher(contents);
731-
Matcher etcMatcher = etcPattern.matcher(contents);
732-
Matcher bootMatcher = bootPattern.matcher(contents);
733-
Matcher tmpMatcher = tmpPattern.matcher(contents);
734-
Matcher homeMatcher = homePattern.matcher(contents);
735-
736-
if (procMatcher.find()
737-
&& etcMatcher.find()
738-
&& bootMatcher.find()
739-
&& tmpMatcher.find()
740-
&& homeMatcher.find()) {
741-
return "etc";
758+
if (PROC_PATT.matcher(contents).find()
759+
&& ETC_PATT.matcher(contents).find()
760+
&& BOOT_PATT.matcher(contents).find()
761+
&& TMP_PATT.matcher(contents).find()
762+
&& HOME_PATT.matcher(contents).find()) {
763+
return NIX_DIR_EVIDENCE;
742764
}
743765

744766
return null;
745767
}
746768

747769
private static String matchWinDirectories(String contents) {
748-
if (contents.contains("Windows")
749-
&& Pattern.compile("Program\\sFiles").matcher(contents).find()) {
750-
return "Windows";
770+
if (contents.contains(WIN_DIR_EVIDENCE)
771+
&& PROGRAM_FILES_PATT.matcher(contents).find()) {
772+
return WIN_DIR_EVIDENCE;
751773
}
752774

753775
return null;
754776
}
777+
778+
private static Pattern createNixPattern(String subPatt) {
779+
return Pattern.compile("(?:^|\\W)" + subPatt + "(?:\\W|$)", Pattern.CASE_INSENSITIVE);
780+
}
755781
}
756782
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ ascanrules.parametertamper.desc = Parameter manipulation caused an error page or
116116
ascanrules.parametertamper.name = Parameter Tampering
117117
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.
118118

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

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

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

Lines changed: 111 additions & 0 deletions
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;
@@ -172,6 +177,13 @@ void shouldAlertIfAttackResponseListsWindowsDirectories() throws Exception {
172177
assertThat(alertsRaised.get(0).getAttack(), is(equalTo("c:/")));
173178
assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH)));
174179
assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM)));
180+
assertThat(
181+
alertsRaised.get(0).getOtherInfo(),
182+
is(
183+
equalTo(
184+
"While the evidence field indicates Windows, the rule actually "
185+
+ "checked that the response contains matches for all of the "
186+
+ "following: Windows, Program Files.")));
175187
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3")));
176188
}
177189

@@ -190,6 +202,13 @@ void shouldAlertIfAttackResponseListsLinuxDirectories() throws Exception {
190202
assertThat(alertsRaised.get(0).getAttack(), is(equalTo("/")));
191203
assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH)));
192204
assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM)));
205+
assertThat(
206+
alertsRaised.get(0).getOtherInfo(),
207+
is(
208+
equalTo(
209+
"While the evidence field indicates etc, the rule actually "
210+
+ "checked that the response contains matches for all of the "
211+
+ "following: proc, etc, boot, tmp, home.")));
193212
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3")));
194213
}
195214

@@ -208,6 +227,13 @@ void shouldAlertIfAttackResponseListsLinuxDirectoriesInPlainText() throws Except
208227
assertThat(alertsRaised.get(0).getAttack(), is(equalTo("/")));
209228
assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH)));
210229
assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM)));
230+
assertThat(
231+
alertsRaised.get(0).getOtherInfo(),
232+
is(
233+
equalTo(
234+
"While the evidence field indicates etc, the rule actually "
235+
+ "checked that the response contains matches for all of the "
236+
+ "following: proc, etc, boot, tmp, home.")));
211237
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3")));
212238
}
213239

@@ -281,6 +307,7 @@ void shouldRaiseAlertIfResponseHasPasswdFileContentAndPayloadIsNullByteBased()
281307
// Then
282308
assertThat(alertsRaised, hasSize(1));
283309
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-2")));
310+
assertThat(alertsRaised.get(0).getOtherInfo(), is(emptyString()));
284311
}
285312

286313
@Test
@@ -297,6 +324,7 @@ void shouldRaiseAlertIfResponseHasSystemINIFileContentAndPayloadIsNullByteBased(
297324
// Then
298325
assertThat(alertsRaised, hasSize(1));
299326
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-1")));
327+
assertThat(alertsRaised.get(0).getOtherInfo(), is(emptyString()));
300328
}
301329

302330
@ParameterizedTest
@@ -317,6 +345,7 @@ void shouldAlertOnCheckFiveBelowHighThresholdUnderValidConditions(AlertThreshold
317345
assertThat(alertsRaised, hasSize(1));
318346
assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_LOW)));
319347
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-5")));
348+
assertThat(alertsRaised.get(0).getOtherInfo(), is(emptyString()));
320349
}
321350

322351
@Test
@@ -365,6 +394,88 @@ void shouldNotAlertOnCheckFiveAtLowThresholdUnderInvalidConditions(String errorT
365394
assertThat(alertsRaised, hasSize(0));
366395
}
367396

397+
@ParameterizedTest
398+
@CsvSource({
399+
// Windows will always happen before ignoring Linux, if only Linux pre-check matches
400+
"foo etc root tmp bin boot dev home mnt opt proc bar, 13",
401+
"foo Program Files Users Windows bar, 13",
402+
"foo etc root tmp bin boot dev home mnt opt proc Program Files Users Windows bar, 11"
403+
})
404+
void shouldSkipDirChecksIfResponseHasDirEvidenceToStartWith(String content, int expected)
405+
throws Exception {
406+
// Given
407+
HttpMessage msg = getHttpMessage("/?p=v");
408+
msg.setResponseBody(content);
409+
rule.init(msg, parent);
410+
// When
411+
rule.scan();
412+
// Then
413+
assertThat(httpMessagesSent, hasSize(equalTo(expected)));
414+
}
415+
416+
private static Stream<Arguments> handlersProvider() {
417+
return Stream.of(
418+
// List Linux dirs on attack, but skip windiws payloads since it has windows
419+
// evidence to start with
420+
Arguments.of(
421+
new ListLinuxDirsOnAttack("/", "p", "/"),
422+
"foo Program Files Users Windows bar",
423+
List.of(
424+
"c:/",
425+
"c:\\",
426+
"..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\",
427+
"\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\",
428+
"file:///c:/",
429+
"file:///c:\\",
430+
"file:\\\\\\c:\\",
431+
"file:\\\\\\c:/",
432+
"file:\\\\\\",
433+
"d:\\",
434+
"d:/",
435+
"file:///d:/",
436+
"file:///d:\\",
437+
"file:\\\\\\d:\\",
438+
"file:\\\\\\d:/")),
439+
// List win dirs on attack, but skip linux payloads since it has linux
440+
// evidence to start with
441+
Arguments.of(
442+
new ListWinDirsOnAttack("/", "p", "c:/"),
443+
"foo etc root tmp bin boot dev home mnt opt proc bar",
444+
List.of(
445+
"/",
446+
"../../../../../../../../../../../../../../../../",
447+
"/../../../../../../../../../../../../../../../../",
448+
"file:///")));
449+
}
450+
451+
@ParameterizedTest
452+
@MethodSource("handlersProvider")
453+
void shouldNotSkipIfGoodCandidateForOnlyOneTech(
454+
NanoServerHandler handler, String baseBody, List<String> skippedPayloads)
455+
throws Exception {
456+
// Given
457+
nano.addHandler(handler);
458+
HttpMessage msg = getHttpMessage("/?p=v");
459+
msg.setResponseBody(baseBody);
460+
rule.init(msg, parent);
461+
// When
462+
rule.scan();
463+
// Then
464+
assertSkippedPayloadsNotUsed(httpMessagesSent, skippedPayloads);
465+
assertThat(alertsRaised, hasSize(equalTo(1)));
466+
assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3")));
467+
}
468+
469+
private static void assertSkippedPayloadsNotUsed(
470+
List<HttpMessage> httpMessagesSent, List<String> skippedPayloads) {
471+
for (HttpMessage m : httpMessagesSent) {
472+
String paramVal = m.getUrlParams().first().getValue();
473+
for (String payload : skippedPayloads) {
474+
assertThat(paramVal.equals(payload), is(equalTo(false)));
475+
}
476+
}
477+
}
478+
368479
private abstract static class ListDirsOnAttack extends NanoServerHandler {
369480

370481
private final String param;

0 commit comments

Comments
 (0)