Skip to content

Commit 7d6c7ee

Browse files
kingthorinkingthorin
committed
ascanrules: Tiddy up ExternalRedirectScanRule
- CHANGELOG > Add maintenance note. - ExternalRedirectScanRule > Use an enum for payloads. Extract a method for payload counts per Stength. Remove unnecessary comments. - ExternalRedirectScanRuleUnitTest > Remove unnecessary assignments. Use isEmpty vs length greater than zero. Signed-off-by: kingthorin <[email protected]>
1 parent ab5a947 commit 7d6c7ee

File tree

3 files changed

+115
-141
lines changed

3 files changed

+115
-141
lines changed

‎addOns/ascanrules/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
1212
- SQL Injection scan rule to start using ComparableResponse - part of the work to reduce False Positives.
1313
- Depends on an updated version of the Common Library add-on.
1414
- Due to it being 2025 and the mass adoption of HTTPS: De-prioritized plain HTTP payloads in the External Redirect scan rule.
15+
- Maintenance changes.
1516

1617
### Fixed
1718
- SQL Injection scan rule to treat a 500 response to an SQLi attack as a likely vulnerability.

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

Lines changed: 76 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -100,29 +100,31 @@ public class ExternalRedirectScanRule extends AbstractAppParamPlugin
100100
private static final String REDIRECT_SITE = SITE_HOST + OWASP_SUFFIX;
101101

102102
/** The various (prioritized) payload to try */
103-
private static final String[] REDIRECT_TARGETS = {
104-
REDIRECT_SITE,
105-
HttpHeader.SCHEME_HTTPS + REDIRECT_SITE,
106-
HttpHeader.SCHEME_HTTPS + REDIRECT_SITE.replace(".", "%2e"), // Double encode the dots
107-
"5;URL='https://" + REDIRECT_SITE + "'",
108-
"URL='http://" + REDIRECT_SITE + "'",
103+
private enum RedirectPayloads {
104+
PLAIN_SITE(REDIRECT_SITE, false),
105+
HTTPS_SITE(HttpHeader.SCHEME_HTTPS + REDIRECT_SITE, false),
106+
// Double encode the dots
107+
HTTPS_PERIOD_ENCODE(HttpHeader.SCHEME_HTTPS + REDIRECT_SITE.replace(".", "%2e"), false),
108+
HTTPS_REFRESH("5;URL='https://" + REDIRECT_SITE + "'", false),
109+
HTTPS_LOCATION("URL='http://" + REDIRECT_SITE + "'", false),
109110
// Simple allow list bypass, ex: https://evil.com?<original_value>
110-
// Where "original_value" is whatever the parameter value initially was, ex:
111+
// Where <original_value> is whatever the parameter value initially was, ex:
111112
// https://good.expected.com
112-
HttpHeader.SCHEME_HTTPS + REDIRECT_SITE + "/?" + ORIGINAL_VALUE_PLACEHOLDER,
113-
"5;URL='https://" + REDIRECT_SITE + "/?" + ORIGINAL_VALUE_PLACEHOLDER + "'",
114-
HttpHeader.SCHEME_HTTPS + "\\" + REDIRECT_SITE,
115-
HttpHeader.SCHEME_HTTP + "\\" + REDIRECT_SITE,
116-
HttpHeader.SCHEME_HTTP + REDIRECT_SITE,
117-
"//" + REDIRECT_SITE,
118-
"\\\\" + REDIRECT_SITE,
119-
"HtTp://" + REDIRECT_SITE,
120-
"HtTpS://" + REDIRECT_SITE,
121-
122-
// http://kotowicz.net/absolute/
123-
// I never met real cases for these
124-
// to be evaluated in the future
125-
/*
113+
HTTPS_ORIG_PARAM(
114+
HttpHeader.SCHEME_HTTPS + REDIRECT_SITE + "/?" + ORIGINAL_VALUE_PLACEHOLDER, true),
115+
HTTPS_REFRESH_ORIG_PARAM(
116+
"5;URL='https://" + REDIRECT_SITE + "/?" + ORIGINAL_VALUE_PLACEHOLDER + "'", true),
117+
HTTPS_WRONG_SLASH(HttpHeader.SCHEME_HTTPS + "\\" + REDIRECT_SITE, false),
118+
HTTP_WRONG_SLASH(HttpHeader.SCHEME_HTTP + "\\" + REDIRECT_SITE, false),
119+
HTTP(HttpHeader.SCHEME_HTTP + REDIRECT_SITE, false),
120+
NO_SCHEME("//" + REDIRECT_SITE, false),
121+
NO_SCHEME_WRONG_SLASH("\\\\" + REDIRECT_SITE, false),
122+
HTTPS_MIXED_CASE("HtTpS://" + REDIRECT_SITE, false),
123+
HTTP_MIXED_CASE("HtTp://" + REDIRECT_SITE, false);
124+
125+
/* http://kotowicz.net/absolute/
126+
I never met real cases for these
127+
to be evaluated in the future
126128
"/\\" + REDIRECT_SITE,
127129
"\\/" + REDIRECT_SITE,
128130
"\r \t//" + REDIRECT_SITE,
@@ -133,7 +135,23 @@ public class ExternalRedirectScanRule extends AbstractAppParamPlugin
133135
"://" + REDIRECT_SITE,
134136
".:." + REDIRECT_SITE
135137
*/
136-
};
138+
139+
private final String payload;
140+
private final boolean hasPlaceHolder;
141+
142+
RedirectPayloads(String payload, boolean hasPlaceHolder) {
143+
this.payload = payload;
144+
this.hasPlaceHolder = hasPlaceHolder;
145+
}
146+
147+
public String getPayload() {
148+
return payload;
149+
}
150+
151+
public boolean hasPlaceHolder() {
152+
return hasPlaceHolder;
153+
}
154+
}
137155

138156
// Get WASC Vulnerability description
139157
private static final Vulnerability VULN = Vulnerabilities.getDefault().get("wasc_38");
@@ -180,124 +198,92 @@ public String getReference() {
180198
@Override
181199
public void scan(HttpMessage msg, String param, String value) {
182200

183-
// Number of targets to try
184-
int targetCount = 0;
185-
186-
// Debug only
187201
LOGGER.debug("Attacking at Attack Strength: {}", this.getAttackStrength());
188-
189202
// Figure out how aggressively we should test
190-
switch (this.getAttackStrength()) {
191-
case LOW:
192-
// Check only for baseline targets (2 reqs / param)
193-
targetCount = 3;
194-
break;
195-
196-
case MEDIUM:
197-
// This works out as a total of 9 reqs / param
198-
targetCount = 9;
199-
break;
200-
201-
case HIGH:
202-
// This works out as a total of 15 reqs / param
203-
targetCount = REDIRECT_TARGETS.length;
204-
break;
205-
206-
case INSANE:
207-
// This works out as a total of 15 reqs / param
208-
targetCount = REDIRECT_TARGETS.length;
209-
break;
210-
211-
default:
212-
break;
213-
}
203+
int payloadCount = getPayloadCount();
214204

215205
LOGGER.debug(
216206
"Checking [{}][{}], parameter [{}] for Open Redirect Vulnerabilities",
217207
getBaseMsg().getRequestHeader().getMethod(),
218208
getBaseMsg().getRequestHeader().getURI(),
219209
param);
220210

221-
// For each target in turn
222-
// note that depending on the AttackLevel,
223-
// the number of elements that we will try changes.
224-
String payload;
225211
String redirectUrl;
212+
int payloadIdx = 0;
226213

227-
for (int h = 0; h < targetCount; h++) {
228-
if (isStop()) {
214+
// For each payload in turn
215+
// note that depending on the Strength,
216+
// the number of elements that we will try changes.
217+
for (RedirectPayloads payload : RedirectPayloads.values()) {
218+
if (isStop() || ++payloadIdx == payloadCount) {
229219
return;
230220
}
231221

232-
payload =
233-
REDIRECT_TARGETS[h].contains(ORIGINAL_VALUE_PLACEHOLDER)
234-
? REDIRECT_TARGETS[h].replace(ORIGINAL_VALUE_PLACEHOLDER, value)
235-
: REDIRECT_TARGETS[h];
222+
String injection = payload.getPayload();
223+
if (payload.hasPlaceHolder()) {
224+
injection = payload.getPayload().replace(ORIGINAL_VALUE_PLACEHOLDER, value);
225+
}
236226

237227
// Get a new copy of the original message (request only) for each parameter value to try
238228
HttpMessage testMsg = getNewMsg();
239-
setParameter(testMsg, param, payload);
229+
setParameter(testMsg, param, injection);
240230

241-
LOGGER.debug("Testing [{}] = [{}]", param, payload);
231+
LOGGER.debug("Testing [{}] = [{}]", param, injection);
242232

243233
try {
244234
// Send the request and retrieve the response
245235
// Be careful: we haven't to follow redirect
246236
sendAndReceive(testMsg, false);
247237

248238
String payloadScheme =
249-
StringUtils.containsIgnoreCase(payload, HttpHeader.HTTPS)
239+
StringUtils.containsIgnoreCase(injection, HttpHeader.HTTPS)
250240
? HttpHeader.HTTPS
251241
: HttpHeader.HTTP;
252-
// If it's a meta based injection the use the base url
242+
// If it's a meta based injection then use the base URL
253243
redirectUrl =
254-
(payload.startsWith("5;") || payload.startsWith("URL="))
244+
(injection.startsWith("5;") || injection.startsWith("URL="))
255245
? payloadScheme + "://" + REDIRECT_SITE
256-
: payload;
246+
: injection;
257247

258248
// Get back if a redirection occurs
259249
int redirectType = isRedirected(redirectUrl, testMsg);
260250

261251
if (redirectType != NO_REDIRECT) {
262-
// We Found IT!
263-
// First do logging
264252
LOGGER.debug(
265253
"[External Redirection Found] on parameter [{}] with payload [{}]",
266254
param,
267-
payload);
268-
269-
buildAlert(param, payload, redirectType, redirectUrl, testMsg).raise();
255+
injection);
270256

271-
// All done. No need to look for vulnerabilities on subsequent
272-
// parameters on the same request (to reduce performance impact)
257+
buildAlert(param, injection, redirectType, redirectUrl, testMsg).raise();
273258
return;
274259
}
275260
} catch (IOException ex) {
276-
// Do not try to internationalize this.. we need an error message in any event..
277-
// if it's in English, it's still better than not having it at all.
278261
LOGGER.warn(
279262
"External Redirect vulnerability check failed for parameter [{}] and payload [{}] due to an I/O error",
280263
param,
281-
payload,
264+
injection,
282265
ex);
283266
}
284267
}
285268
}
286269

270+
private int getPayloadCount() {
271+
return switch (this.getAttackStrength()) {
272+
case LOW -> 3;
273+
case MEDIUM -> 9;
274+
case HIGH, INSANE -> RedirectPayloads.values().length;
275+
default -> 9;
276+
};
277+
}
278+
287279
private String getAlertReference(int redirectType) {
288-
switch (redirectType) {
289-
case REDIRECT_LOCATION_HEADER:
290-
return getId() + "-1";
291-
case REDIRECT_REFRESH_HEADER:
292-
return getId() + "-2";
293-
case REDIRECT_LOCATION_META:
294-
case REDIRECT_REFRESH_META:
295-
return getId() + "-3";
296-
case REDIRECT_JAVASCRIPT:
297-
return getId() + "-4";
298-
default:
299-
return "";
300-
}
280+
return switch (redirectType) {
281+
case REDIRECT_LOCATION_HEADER -> getId() + "-1";
282+
case REDIRECT_REFRESH_HEADER -> getId() + "-2";
283+
case REDIRECT_LOCATION_META, REDIRECT_REFRESH_META -> getId() + "-3";
284+
case REDIRECT_JAVASCRIPT -> getId() + "-4";
285+
default -> "";
286+
};
301287
}
302288

303289
private AlertBuilder buildAlert(

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

Lines changed: 38 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ private enum PayloadHandling {
8282
ALLOW_LIST,
8383
CONCAT_PARAM,
8484
CONCAT_PATH
85-
};
85+
}
8686

8787
private static NanoServerHandler createHttpRedirectHandler(String path, String header) {
8888
return createHttpRedirectHandler(path, header, PayloadHandling.NEITHER);
@@ -130,7 +130,7 @@ protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) {
130130
}
131131
return response;
132132
}
133-
if (site != null && site.length() > 0) {
133+
if (site != null && !site.isEmpty()) {
134134
return redirectResponse;
135135
}
136136
return response;
@@ -319,7 +319,7 @@ void shouldReportDoubleEncodedRedirect() throws Exception {
319319
@Override
320320
protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) {
321321
String site = getFirstParamValue(session, "site");
322-
if (site != null && site.length() > 0 && !site.contains(".")) {
322+
if (site != null && !site.isEmpty() && !site.contains(".")) {
323323
Response response =
324324
newFixedLengthResponse(
325325
NanoHTTPD.Response.Status.REDIRECT,
@@ -328,8 +328,7 @@ protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) {
328328
response.addHeader(HttpFieldsNames.LOCATION, site);
329329
return response;
330330
}
331-
String response = "<html><body></body></html>";
332-
return newFixedLengthResponse(response);
331+
return newFixedLengthResponse("<html><body></body></html>");
333332
}
334333
});
335334
HttpMessage msg = getHttpMessage(test + "?site=xxx");
@@ -353,20 +352,17 @@ void shouldReportRedirectWithMetaLocationOrRefresh(String type) throws Exception
353352
@Override
354353
protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) {
355354
String site = getFirstParamValue(session, "site");
356-
if (site != null && site.length() > 0) {
357-
Response response =
358-
newFixedLengthResponse(
359-
NanoHTTPD.Response.Status.OK,
360-
NanoHTTPD.MIME_HTML,
361-
"<html><head><meta http-equiv=\""
362-
+ type
363-
+ "\" content=\""
364-
+ site
365-
+ "\"></head><body><H1>Redirect></H1></body></html>");
366-
return response;
355+
if (site != null && !site.isEmpty()) {
356+
return newFixedLengthResponse(
357+
NanoHTTPD.Response.Status.OK,
358+
NanoHTTPD.MIME_HTML,
359+
"<html><head><meta http-equiv=\""
360+
+ type
361+
+ "\" content=\""
362+
+ site
363+
+ "\"></head><body><H1>Redirect></H1></body></html>");
367364
}
368-
String response = "<html><body></body></html>";
369-
return newFixedLengthResponse(response);
365+
return newFixedLengthResponse("<html><body></body></html>");
370366
}
371367
});
372368
HttpMessage msg = getHttpMessage(test + "?site=xxx");
@@ -385,18 +381,15 @@ private static NanoServerHandler createMetaHandler(String test, String type, Str
385381
@Override
386382
protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) {
387383
String site = getFirstParamValue(session, "site");
388-
if (site != null && site.length() > 0) {
389-
Response response =
390-
newFixedLengthResponse(
391-
NanoHTTPD.Response.Status.OK,
392-
NanoHTTPD.MIME_HTML,
393-
META_TEMPLATE
394-
.replace(TYPE_TOKEN, type)
395-
.replace(CONTENT_TOKEN, content + site));
396-
return response;
384+
if (site != null && !site.isEmpty()) {
385+
return newFixedLengthResponse(
386+
NanoHTTPD.Response.Status.OK,
387+
NanoHTTPD.MIME_HTML,
388+
META_TEMPLATE
389+
.replace(TYPE_TOKEN, type)
390+
.replace(CONTENT_TOKEN, content + site));
397391
}
398-
String response = "<html><body></body></html>";
399-
return newFixedLengthResponse(response);
392+
return newFixedLengthResponse("<html><body></body></html>");
400393
}
401394
};
402395
}
@@ -445,18 +438,15 @@ private static NanoServerHandler createJsVariableHandler(
445438
@Override
446439
protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) {
447440
String site = getFirstParamValue(session, "site");
448-
if (site != null && site.length() > 0) {
449-
Response response =
450-
newFixedLengthResponse(
451-
NanoHTTPD.Response.Status.OK,
452-
NanoHTTPD.MIME_HTML,
453-
JS_VAR_TEMPLATE
454-
.replace(JS_VAR_TOKEN, jsVar)
455-
.replace(CONTENT_TOKEN, content + site));
456-
return response;
441+
if (site != null && !site.isEmpty()) {
442+
return newFixedLengthResponse(
443+
NanoHTTPD.Response.Status.OK,
444+
NanoHTTPD.MIME_HTML,
445+
JS_VAR_TEMPLATE
446+
.replace(JS_VAR_TOKEN, jsVar)
447+
.replace(CONTENT_TOKEN, content + site));
457448
}
458-
String response = "<html><body></body></html>";
459-
return newFixedLengthResponse(response);
449+
return newFixedLengthResponse("<html><body></body></html>");
460450
}
461451
};
462452
}
@@ -510,18 +500,15 @@ private static NanoServerHandler createJsMethodHandler(
510500
@Override
511501
protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) {
512502
String site = getFirstParamValue(session, "site");
513-
if (site != null && site.length() > 0) {
514-
Response response =
515-
newFixedLengthResponse(
516-
NanoHTTPD.Response.Status.OK,
517-
NanoHTTPD.MIME_HTML,
518-
JS_METHOD_TEMPLATE
519-
.replace(JS_METHOD_TOKEN, jsMethod)
520-
.replace(CONTENT_TOKEN, content + site));
521-
return response;
503+
if (site != null && !site.isEmpty()) {
504+
return newFixedLengthResponse(
505+
NanoHTTPD.Response.Status.OK,
506+
NanoHTTPD.MIME_HTML,
507+
JS_METHOD_TEMPLATE
508+
.replace(JS_METHOD_TOKEN, jsMethod)
509+
.replace(CONTENT_TOKEN, content + site));
522510
}
523-
String response = "<html><body></body></html>";
524-
return newFixedLengthResponse(response);
511+
return newFixedLengthResponse("<html><body></body></html>");
525512
}
526513
};
527514
}

0 commit comments

Comments
 (0)