Skip to content

Commit 3b50c83

Browse files
kingthorinkingthorin
authored
Merge pull request #6605 from kingthorin/inj-maint
ascanrules: SAST (SonarLint) Fixes
2 parents 41c0a0e + 195c211 commit 3b50c83

19 files changed

+384
-602
lines changed

‎addOns/ascanrules/CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
1515
- SQL Injection - SQLite
1616
- SQL Injection - PostgreSQL
1717
- The Remote OS Command Injection scan rule has been broken into two rules; one feedback based, and one time based (Issue 7341). This includes assigning the time based rule ID 90037.
18+
- For Alerts raised by the SQL Injection scan rules the Attack field values are now simply the payload, not an assembled description.
19+
- Maintenance changes.
1820

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

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

Lines changed: 16 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@
5656
public class CommandInjectionScanRule extends AbstractAppParamPlugin
5757
implements CommonActiveScanRuleInfo {
5858

59-
/** Prefix for internationalised messages used by this rule */
6059
static final String MESSAGE_PREFIX = "ascanrules.commandinjection.";
6160

6261
// *NIX OS Command constants
@@ -220,10 +219,8 @@ public class CommandInjectionScanRule extends AbstractAppParamPlugin
220219
NIX_OS_PAYLOADS.put("&&" + insertedCMD + NULL_BYTE_CHARACTER, NIX_CTRL_PATTERN);
221220
}
222221

223-
// Logger instance
224222
private static final Logger LOGGER = LogManager.getLogger(CommandInjectionScanRule.class);
225223

226-
// Get WASC Vulnerability description
227224
private static final Vulnerability VULN = Vulnerabilities.getDefault().get("wasc_31");
228225

229226
@Override
@@ -238,12 +235,9 @@ public String getName() {
238235

239236
@Override
240237
public boolean targets(TechSet technologies) {
241-
if (technologies.includes(Tech.Linux)
238+
return technologies.includes(Tech.Linux)
242239
|| technologies.includes(Tech.MacOS)
243-
|| technologies.includes(Tech.Windows)) {
244-
return true;
245-
}
246-
return false;
240+
|| technologies.includes(Tech.Windows);
247241
}
248242

249243
@Override
@@ -295,40 +289,23 @@ public int getRisk() {
295289
*/
296290
@Override
297291
public void scan(HttpMessage msg, String paramName, String value) {
298-
299-
// Begin scan rule execution
300292
LOGGER.debug(
301293
"Checking [{}][{}], parameter [{}] for OS Command Injection Vulnerabilities",
302294
msg.getRequestHeader().getMethod(),
303295
msg.getRequestHeader().getURI(),
304296
paramName);
305297

306-
// Number of targets to try
307-
int targetCount = 0;
308-
309-
switch (this.getAttackStrength()) {
310-
case LOW:
311-
targetCount = 3;
312-
break;
313-
314-
case MEDIUM:
315-
targetCount = 7;
316-
break;
317-
318-
case HIGH:
319-
targetCount = 13;
320-
break;
321-
322-
case INSANE:
323-
targetCount =
324-
Math.max(
325-
PS_PAYLOADS.size(),
326-
(Math.max(NIX_OS_PAYLOADS.size(), WIN_OS_PAYLOADS.size())));
327-
break;
328-
329-
default:
330-
// Default to off
331-
}
298+
int targetCount =
299+
switch (this.getAttackStrength()) {
300+
case LOW -> 3;
301+
case MEDIUM -> 7;
302+
case HIGH -> 13;
303+
case INSANE ->
304+
Math.max(
305+
PS_PAYLOADS.size(),
306+
Math.max(NIX_OS_PAYLOADS.size(), WIN_OS_PAYLOADS.size()));
307+
default -> 0; // Default to "off"
308+
};
332309

333310
if (inScope(Tech.Linux) || inScope(Tech.MacOS)) {
334311
if (testCommandInjection(paramName, value, targetCount, NIX_OS_PAYLOADS)) {
@@ -392,7 +369,6 @@ private boolean testCommandInjection(
392369
LOGGER.debug("Testing [{}] = [{}]", paramName, paramValue);
393370

394371
try {
395-
// Send the request and retrieve the response
396372
try {
397373
sendAndReceive(msg, false);
398374
} catch (SocketException ex) {
@@ -413,8 +389,6 @@ private boolean testCommandInjection(
413389

414390
Matcher matcher = osPayloads.get(payload).matcher(content);
415391
if (matcher.find()) {
416-
// We Found IT!
417-
// First do logging
418392
LOGGER.debug(
419393
"[OS Command Injection Found] on parameter [{}] with value [{}]",
420394
paramName,
@@ -435,8 +409,6 @@ private boolean testCommandInjection(
435409
ex);
436410
}
437411
if (isStop()) {
438-
// Dispose all resources
439-
// Exit the scan rule
440412
return false;
441413
}
442414
}
@@ -458,11 +430,11 @@ static String insertUninitVar(String cmd) {
458430
for (int i = 1; i < varLength; ++i) {
459431
array[i] = (char) ThreadLocalRandom.current().nextInt(97, 123);
460432
}
461-
String var = new String(array);
433+
String variable = new String(array);
462434

463435
// insert variable before each space and '/' in the path
464-
return cmd.replaceAll("\\s", Matcher.quoteReplacement(var + " "))
465-
.replaceAll("\\/", Matcher.quoteReplacement(var + "/"));
436+
return cmd.replaceAll("\\s", Matcher.quoteReplacement(variable + " "))
437+
.replaceAll("\\/", Matcher.quoteReplacement(variable + "/"));
466438
}
467439

468440
AlertBuilder buildAlert(String param, String attack, String evidence, HttpMessage msg) {

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import java.util.LinkedList;
2828
import java.util.List;
2929
import java.util.Map;
30+
import java.util.concurrent.TimeUnit;
3031
import java.util.concurrent.atomic.AtomicReference;
3132
import org.apache.commons.configuration.ConversionException;
3233
import org.apache.logging.log4j.LogManager;
@@ -268,7 +269,7 @@ private boolean testCommandInjection(
268269
LOGGER.debug("Testing [{}] = [{}]", paramName, finalPayload);
269270

270271
sendAndReceive(msg, false);
271-
return msg.getTimeElapsedMillis() / 1000.0;
272+
return TimeUnit.MILLISECONDS.toSeconds(msg.getTimeElapsedMillis());
272273
};
273274

274275
boolean isInjectable;

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

Lines changed: 36 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import java.util.Iterator;
2727
import java.util.List;
2828
import java.util.Map;
29+
import java.util.concurrent.TimeUnit;
2930
import java.util.concurrent.atomic.AtomicReference;
3031
import org.apache.commons.configuration.ConversionException;
3132
import org.apache.logging.log4j.LogManager;
@@ -70,6 +71,9 @@
7071
public class SqlInjectionHypersonicTimingScanRule extends AbstractAppParamPlugin
7172
implements CommonActiveScanRuleInfo {
7273

74+
private static final String DEFAULT_FROM_AND_WHERE =
75+
" from INFORMATION_SCHEMA.SYSTEM_COLUMNS where TABLE_NAME = 'SYSTEM_COLUMNS' and COLUMN_NAME = 'TABLE_NAME'";
76+
7377
/** Hypersonic one-line comment */
7478
public static final String SQL_ONE_LINE_COMMENT = " -- ";
7579

@@ -100,19 +104,19 @@ public class SqlInjectionHypersonicTimingScanRule extends AbstractAppParamPlugin
100104
List.of(
101105
"; select "
102106
+ SQL_HYPERSONIC_TIME_FUNCTION
103-
+ " from INFORMATION_SCHEMA.SYSTEM_COLUMNS where TABLE_NAME = 'SYSTEM_COLUMNS' and COLUMN_NAME = 'TABLE_NAME'"
107+
+ DEFAULT_FROM_AND_WHERE
104108
+ SQL_ONE_LINE_COMMENT,
105109
"'; select "
106110
+ SQL_HYPERSONIC_TIME_FUNCTION
107-
+ " from INFORMATION_SCHEMA.SYSTEM_COLUMNS where TABLE_NAME = 'SYSTEM_COLUMNS' and COLUMN_NAME = 'TABLE_NAME'"
111+
+ DEFAULT_FROM_AND_WHERE
108112
+ SQL_ONE_LINE_COMMENT,
109113
"\"; select "
110114
+ SQL_HYPERSONIC_TIME_FUNCTION
111-
+ " from INFORMATION_SCHEMA.SYSTEM_COLUMNS where TABLE_NAME = 'SYSTEM_COLUMNS' and COLUMN_NAME = 'TABLE_NAME'"
115+
+ DEFAULT_FROM_AND_WHERE
112116
+ SQL_ONE_LINE_COMMENT,
113117
"); select "
114118
+ SQL_HYPERSONIC_TIME_FUNCTION
115-
+ " from INFORMATION_SCHEMA.SYSTEM_COLUMNS where TABLE_NAME = 'SYSTEM_COLUMNS' and COLUMN_NAME = 'TABLE_NAME'"
119+
+ DEFAULT_FROM_AND_WHERE
116120
+ SQL_ONE_LINE_COMMENT,
117121
SQL_HYPERSONIC_TIME_FUNCTION,
118122
ORIG_VALUE_TOKEN + " / " + SQL_HYPERSONIC_TIME_FUNCTION + " ",
@@ -121,42 +125,50 @@ public class SqlInjectionHypersonicTimingScanRule extends AbstractAppParamPlugin
121125
ORIG_VALUE_TOKEN
122126
+ " and exists ( select "
123127
+ SQL_HYPERSONIC_TIME_FUNCTION
124-
+ " from INFORMATION_SCHEMA.SYSTEM_COLUMNS where TABLE_NAME = 'SYSTEM_COLUMNS' and COLUMN_NAME = 'TABLE_NAME')"
128+
+ DEFAULT_FROM_AND_WHERE
129+
+ ")"
125130
+ SQL_ONE_LINE_COMMENT, // Param in WHERE clause somewhere
126131
ORIG_VALUE_TOKEN
127132
+ "' and exists ( select "
128133
+ SQL_HYPERSONIC_TIME_FUNCTION
129-
+ " from INFORMATION_SCHEMA.SYSTEM_COLUMNS where TABLE_NAME = 'SYSTEM_COLUMNS' and COLUMN_NAME = 'TABLE_NAME')"
134+
+ DEFAULT_FROM_AND_WHERE
135+
+ ")"
130136
+ SQL_ONE_LINE_COMMENT, // Param in WHERE clause somewhere
131137
ORIG_VALUE_TOKEN
132138
+ "\" and exists ( select "
133139
+ SQL_HYPERSONIC_TIME_FUNCTION
134-
+ " from INFORMATION_SCHEMA.SYSTEM_COLUMNS where TABLE_NAME = 'SYSTEM_COLUMNS' and COLUMN_NAME = 'TABLE_NAME')"
140+
+ DEFAULT_FROM_AND_WHERE
141+
+ ")"
135142
+ SQL_ONE_LINE_COMMENT, // Param in WHERE clause somewhere
136143
ORIG_VALUE_TOKEN
137144
+ ") and exists ( select "
138145
+ SQL_HYPERSONIC_TIME_FUNCTION
139-
+ " from INFORMATION_SCHEMA.SYSTEM_COLUMNS where TABLE_NAME = 'SYSTEM_COLUMNS' and COLUMN_NAME = 'TABLE_NAME')"
146+
+ DEFAULT_FROM_AND_WHERE
147+
+ ")"
140148
+ SQL_ONE_LINE_COMMENT, // Param in WHERE clause somewhere
141149
ORIG_VALUE_TOKEN
142150
+ " or exists ( select "
143151
+ SQL_HYPERSONIC_TIME_FUNCTION
144-
+ " from INFORMATION_SCHEMA.SYSTEM_COLUMNS where TABLE_NAME = 'SYSTEM_COLUMNS' and COLUMN_NAME = 'TABLE_NAME')"
152+
+ DEFAULT_FROM_AND_WHERE
153+
+ ")"
145154
+ SQL_ONE_LINE_COMMENT, // Param in WHERE clause somewhere
146155
ORIG_VALUE_TOKEN
147156
+ "' or exists ( select "
148157
+ SQL_HYPERSONIC_TIME_FUNCTION
149-
+ " from INFORMATION_SCHEMA.SYSTEM_COLUMNS where TABLE_NAME = 'SYSTEM_COLUMNS' and COLUMN_NAME = 'TABLE_NAME')"
158+
+ DEFAULT_FROM_AND_WHERE
159+
+ ")"
150160
+ SQL_ONE_LINE_COMMENT, // Param in WHERE clause somewhere
151161
ORIG_VALUE_TOKEN
152162
+ "\" or exists ( select "
153163
+ SQL_HYPERSONIC_TIME_FUNCTION
154-
+ " from INFORMATION_SCHEMA.SYSTEM_COLUMNS where TABLE_NAME = 'SYSTEM_COLUMNS' and COLUMN_NAME = 'TABLE_NAME')"
164+
+ DEFAULT_FROM_AND_WHERE
165+
+ ")"
155166
+ SQL_ONE_LINE_COMMENT, // Param in WHERE clause somewhere
156167
ORIG_VALUE_TOKEN
157168
+ ") or exists ( select "
158169
+ SQL_HYPERSONIC_TIME_FUNCTION
159-
+ " from INFORMATION_SCHEMA.SYSTEM_COLUMNS where TABLE_NAME = 'SYSTEM_COLUMNS' and COLUMN_NAME = 'TABLE_NAME')"
170+
+ DEFAULT_FROM_AND_WHERE
171+
+ ")"
160172
+ SQL_ONE_LINE_COMMENT // Param in WHERE clause somewhere
161173
);
162174

@@ -288,14 +300,17 @@ public void scan(HttpMessage originalMessage, String paramName, String paramValu
288300
sleepPayload
289301
.replace(ORIG_VALUE_TOKEN, paramValue)
290302
// Time in milliseconds for the SQL function.
291-
.replace(SLEEP_TOKEN, Integer.toString(((int) x) * 1000));
303+
.replace(
304+
SLEEP_TOKEN,
305+
String.valueOf(
306+
TimeUnit.SECONDS.toMillis((long) x)));
292307

293308
setParameter(msg, paramName, finalPayload);
294309
LOGGER.debug("Testing [{}] = [{}]", paramName, finalPayload);
295310
attack.compareAndSet(null, finalPayload);
296311

297312
sendAndReceive(msg, false);
298-
return msg.getTimeElapsedMillis() / 1000.0;
313+
return TimeUnit.MILLISECONDS.toSeconds(msg.getTimeElapsedMillis());
299314
};
300315

301316
try {
@@ -313,21 +328,18 @@ public void scan(HttpMessage originalMessage, String paramName, String paramValu
313328
paramName,
314329
attack.get());
315330

316-
String extraInfo =
317-
Constant.messages.getString(
318-
"ascanrules.sqlinjection.alert.timebased.extrainfo",
319-
attack.get(),
320-
message.get().getTimeElapsedMillis(),
321-
paramValue,
322-
getBaseMsg().getTimeElapsedMillis());
323-
324331
newAlert()
325332
.setConfidence(Alert.CONFIDENCE_MEDIUM)
326-
.setName(getName() + " - Time Based")
327333
.setUri(getBaseMsg().getRequestHeader().getURI().toString())
328334
.setParam(paramName)
329335
.setAttack(attack.get())
330-
.setOtherInfo(extraInfo)
336+
.setOtherInfo(
337+
Constant.messages.getString(
338+
"ascanrules.sqlinjection.alert.timebased.extrainfo",
339+
attack.get(),
340+
message.get().getTimeElapsedMillis(),
341+
paramValue,
342+
getBaseMsg().getTimeElapsedMillis()))
331343
.setMessage(message.get())
332344
.raise();
333345
break;

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

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,6 @@ public class SqlInjectionMsSqlTimingScanRule extends AbstractAppParamPlugin
133133
private static final double TIME_CORRELATION_ERROR_RANGE = 0.15;
134134
private static final double TIME_SLOPE_ERROR_RANGE = 0.30;
135135

136-
/** for logging. */
137136
private static final Logger LOGGER =
138137
LogManager.getLogger(SqlInjectionMsSqlTimingScanRule.class);
139138

@@ -201,7 +200,6 @@ public String getReference() {
201200
public void init() {
202201
LOGGER.debug("Initialising");
203202

204-
// set up what we are allowed to do, depending on the attack strength that was set.
205203
if (this.getAttackStrength() == AttackStrength.LOW) {
206204
blindTargetCount = 6;
207205
} else if (this.getAttackStrength() == AttackStrength.MEDIUM) {
@@ -212,7 +210,6 @@ public void init() {
212210
blindTargetCount = SQL_MSSQL_TIME_REPLACEMENTS.size();
213211
}
214212

215-
// Read the sleep value from the configs
216213
try {
217214
this.timeSleepSeconds =
218215
this.getConfig()
@@ -225,7 +222,6 @@ public void init() {
225222
LOGGER.debug("Sleep set to {} seconds", timeSleepSeconds);
226223
}
227224

228-
/** scans for SQL Injection vulnerabilities, using MsSQL specific syntax. */
229225
@Override
230226
public void scan(HttpMessage originalMessage, String paramName, String paramValue) {
231227
LOGGER.debug(
@@ -255,7 +251,7 @@ public void scan(HttpMessage originalMessage, String paramName, String paramValu
255251
attack.compareAndSet(null, finalPayload);
256252

257253
sendAndReceive(msg, false);
258-
return msg.getTimeElapsedMillis() / 1000.0;
254+
return TimeUnit.MILLISECONDS.toSeconds(msg.getTimeElapsedMillis());
259255
};
260256

261257
try {

0 commit comments

Comments
 (0)