Skip to content

ascanrules: Enhanced CommandInjectionScanRule with URL-encoded bypass… #6542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

ascanrules: Enhanced CommandInjectionScanRule with URL-encoded bypass… #6542

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ac23476
ascanrules: Enhanced CommandInjectionScanRule with URL-encoded bypass…
cx-nir-cohen Jun 27, 2025
b2e2da0
Fix: Restore critical null byte payloads that were accidentally removed
cx-nir-cohen Jun 27, 2025
2284a11
Fix: Remove duplicate insertedCMD variable declaration
cx-nir-cohen Jun 27, 2025
18b147f
Fix: Apply Spotless code formatting
cx-nir-cohen Jun 27, 2025
0b57487
Fix: Update max requests per param limits for new payloads
cx-nir-cohen Jun 27, 2025
dedfa94
Clean up: Remove unnecessary comments from test code
cx-nir-cohen Jun 27, 2025
f7b7a28
Enhance CommandInjectionScanRule for VulnerableApp detection
cx-nir-cohen Jun 27, 2025
541c166
Clean up code: combine adaptive timeout calculation and remove excess…
cx-nir-cohen Jun 27, 2025
4e5cc95
Enhance CommandInjectionScanRule with optimized payload strategy
cx-nir-cohen Jun 27, 2025
b001509
Update CommandInjectionScanRuleUnitTest with refined request expectat…
cx-nir-cohen Jun 27, 2025
535cfb2
Apply spotless code formatting to CommandInjectionScanRule
cx-nir-cohen Jun 27, 2025
567b8eb
Add JavaDoc documentation to testCommandInjection method
cx-nir-cohen Jun 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions addOns/ascanrules/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
### Changed
- Maintenance changes.
- Depends on an updated version of the Common Library add-on.
- ascanrules: Enhanced the Remote OS Command Injection scan rule with URL-encoded bypass payloads and adaptive timing detection for improved container/cloud environment compatibility.

### Added
- Rules (as applicable) have been tagged in relation to HIPAA and PCI DSS.
Expand Down
65 changes: 54 additions & 11 deletions ...canrules/src/main/java/org/zaproxy/zap/extension/ascanrules/CommandInjectionScanRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,15 @@ public class CommandInjectionScanRule extends AbstractAppParamPlugin
WIN_OS_PAYLOADS.put("run " + WIN_TEST_CMD, WIN_CTRL_PATTERN);
PS_PAYLOADS.put(";" + PS_TEST_CMD + " #", PS_CTRL_PATTERN); // chain & comment

// uninitialized variable waf bypass
NIX_OS_PAYLOADS.put("\n" + NIX_TEST_CMD, NIX_CTRL_PATTERN);
NIX_OS_PAYLOADS.put("\r" + NIX_TEST_CMD, NIX_CTRL_PATTERN);
WIN_OS_PAYLOADS.put("\n" + WIN_TEST_CMD, WIN_CTRL_PATTERN);
WIN_OS_PAYLOADS.put("\r" + WIN_TEST_CMD, WIN_CTRL_PATTERN);
NIX_OS_PAYLOADS.put("\r\n" + NIX_TEST_CMD, NIX_CTRL_PATTERN);
WIN_OS_PAYLOADS.put("\r\n" + WIN_TEST_CMD, WIN_CTRL_PATTERN);
NIX_OS_PAYLOADS.put("\t" + NIX_TEST_CMD, NIX_CTRL_PATTERN);
WIN_OS_PAYLOADS.put("\t" + WIN_TEST_CMD, WIN_CTRL_PATTERN);

String insertedCMD = insertUninitVar(NIX_TEST_CMD);
// No quote payloads
NIX_OS_PAYLOADS.put("&" + insertedCMD + "&", NIX_CTRL_PATTERN);
Expand Down Expand Up @@ -202,7 +210,7 @@ public class CommandInjectionScanRule extends AbstractAppParamPlugin
WIN_OS_PAYLOADS.put("'&" + WIN_TEST_CMD + NULL_BYTE_CHARACTER, WIN_CTRL_PATTERN);
WIN_OS_PAYLOADS.put("'|" + WIN_TEST_CMD + NULL_BYTE_CHARACTER, WIN_CTRL_PATTERN);

// Special payloads
// Special payloads with null byte
NIX_OS_PAYLOADS.put(
"||" + NIX_TEST_CMD + NULL_BYTE_CHARACTER,
NIX_CTRL_PATTERN); // or control concatenation
Expand All @@ -212,8 +220,6 @@ public class CommandInjectionScanRule extends AbstractAppParamPlugin
// FoxPro for running os commands
WIN_OS_PAYLOADS.put("run " + WIN_TEST_CMD + NULL_BYTE_CHARACTER, WIN_CTRL_PATTERN);

// uninitialized variable waf bypass
insertedCMD = insertUninitVar(NIX_TEST_CMD);
// No quote payloads
NIX_OS_PAYLOADS.put("&" + insertedCMD + NULL_BYTE_CHARACTER, NIX_CTRL_PATTERN);
NIX_OS_PAYLOADS.put(";" + insertedCMD + NULL_BYTE_CHARACTER, NIX_CTRL_PATTERN);
Expand All @@ -229,13 +235,12 @@ public class CommandInjectionScanRule extends AbstractAppParamPlugin
}

/** The default number of seconds used in time-based attacks (i.e. sleep commands). */
private static final int DEFAULT_TIME_SLEEP_SEC = 5;
private static final int DEFAULT_TIME_SLEEP_SEC = 3;

// limit the maximum number of requests sent for time-based attack detection
private static final int BLIND_REQUESTS_LIMIT = 4;
private static final int BLIND_REQUESTS_LIMIT = 6;

// error range allowable for statistical time-based blind attacks (0-1.0)
private static final double TIME_CORRELATION_ERROR_RANGE = 0.15;
private static final double TIME_CORRELATION_ERROR_RANGE = 0.25;
private static final double TIME_SLOPE_ERROR_RANGE = 0.30;

// *NIX Blind OS Command constants
Expand Down Expand Up @@ -282,7 +287,15 @@ public class CommandInjectionScanRule extends AbstractAppParamPlugin
WIN_BLIND_OS_PAYLOADS.add("run " + WIN_BLIND_TEST_CMD);
PS_BLIND_PAYLOADS.add(";" + PS_BLIND_TEST_CMD + " #"); // chain & comment

// uninitialized variable waf bypass
NIX_BLIND_OS_PAYLOADS.add("\n" + NIX_BLIND_TEST_CMD);
NIX_BLIND_OS_PAYLOADS.add("\r" + NIX_BLIND_TEST_CMD);
WIN_BLIND_OS_PAYLOADS.add("\n" + WIN_BLIND_TEST_CMD);
WIN_BLIND_OS_PAYLOADS.add("\r" + WIN_BLIND_TEST_CMD);
NIX_BLIND_OS_PAYLOADS.add("\r\n" + NIX_BLIND_TEST_CMD);
WIN_BLIND_OS_PAYLOADS.add("\r\n" + WIN_BLIND_TEST_CMD);
NIX_BLIND_OS_PAYLOADS.add("\t" + NIX_BLIND_TEST_CMD);
WIN_BLIND_OS_PAYLOADS.add("\t" + WIN_BLIND_TEST_CMD);

String insertedCMD = insertUninitVar(NIX_BLIND_TEST_CMD);
// No quote payloads
NIX_BLIND_OS_PAYLOADS.add("&" + insertedCMD + "&");
Expand Down Expand Up @@ -403,6 +416,33 @@ public void init() {
LOGGER.debug("Sleep set to {} seconds", timeSleepSeconds);
}

/**
* Measures baseline response time and calculates adaptive timeout for timing attacks. This
* helps improve detection accuracy in containerized and cloud environments.
*
* @return adaptive timeout in seconds, minimum of 3 seconds
*/
private int getAdaptiveTimeout() {
try {
HttpMessage baselineMsg = getNewMsg();
long startTime = System.currentTimeMillis();
sendAndReceive(baselineMsg, false);
long baselineTime = System.currentTimeMillis() - startTime;

int adaptiveTimeout = Math.min(15, Math.max(3, (int) (baselineTime / 1000) + 2));

LOGGER.debug(
"Baseline response time: {}ms, adaptive timeout: {}s",
baselineTime,
adaptiveTimeout);

return adaptiveTimeout;
} catch (Exception e) {
LOGGER.debug("Failed to measure baseline, using default timeout: {}", timeSleepSeconds);
return timeSleepSeconds;
}
}

/**
* Gets the number of seconds used in time-based attacks.
*
Expand Down Expand Up @@ -620,13 +660,16 @@ private boolean testCommandInjection(
// linear regression to check for a correlation
// between requested delay and actual delay.
// -----------------------------------------------
int adaptiveTimeout = getAdaptiveTimeout();
LOGGER.debug(
"Using adaptive timeout of {} seconds for timing-based detection", adaptiveTimeout);

it = blindOsPayloads.iterator();

for (int i = 0; it.hasNext() && (i < blindTargetCount); i++) {
AtomicReference<HttpMessage> message = new AtomicReference<>();
String sleepPayload = it.next();
paramValue = value + sleepPayload.replace("{0}", String.valueOf(timeSleepSeconds));
paramValue = value + sleepPayload.replace("{0}", String.valueOf(adaptiveTimeout));

// the function that will send each request
TimingUtils.RequestSender requestSender =
Expand All @@ -650,7 +693,7 @@ private boolean testCommandInjection(
isInjectable =
TimingUtils.checkTimingDependence(
BLIND_REQUESTS_LIMIT,
timeSleepSeconds,
adaptiveTimeout,
requestSender,
TIME_CORRELATION_ERROR_RANGE,
TIME_SLOPE_ERROR_RANGE);
Expand Down
148 changes: 140 additions & 8 deletions .../src/test/java/org/zaproxy/zap/extension/ascanrules/CommandInjectionScanRuleUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,14 @@ protected int getRecommendMaxNumberMessagesPerParam(AttackStrength strength) {
int recommendMax = super.getRecommendMaxNumberMessagesPerParam(strength);
switch (strength) {
case LOW:
return recommendMax + 9;
return recommendMax + 15;
case MEDIUM:
default:
return recommendMax + 23;
return recommendMax + 35;
case HIGH:
return recommendMax + 30;
return recommendMax + 50;
case INSANE:
return recommendMax + 17;
return recommendMax + 65;
Copy link
Member

@psiinon psiinon Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are significant increases to a rule which is already sending "too many" requests 😦

Copy link
Author

@cx-nir-cohen cx-nir-cohen Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, agree, i just understood this function too late, what is your suggestion here?

Copy link
Member

@psiinon psiinon Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cutting it down to just the best payloads which give us the widest coverage against real world apps 😁
Also, I think WAF bypass techniques could potentially apply to all rules, so it would be better to look at that as a separate enhancement, rather than adding them to each rule..

}
}

Expand Down Expand Up @@ -185,11 +185,11 @@ void shouldInitWithConfig() throws Exception {
}

@Test
void shouldUse5SecsByDefaultForTimeBasedAttacks() throws Exception {
void shouldUse3SecsByDefaultForTimeBasedAttacks() throws Exception {
// Given / When
int time = rule.getTimeSleep();
// Then
assertThat(time, is(equalTo(5)));
assertThat(time, is(equalTo(3)));
}

@Test
Expand All @@ -203,13 +203,13 @@ void shouldUseTimeDefinedInConfigForTimeBasedAttacks() throws Exception {
}

@Test
void shouldDefaultTo5SecsIfConfigTimeIsMalformedValueForTimeBasedAttacks() throws Exception {
void shouldDefaultTo3SecsIfConfigTimeIsMalformedValueForTimeBasedAttacks() throws Exception {
// Given
rule.setConfig(configWithSleepRule("not a valid value"));
// When
rule.init(getHttpMessage(""), parent);
// Then
assertThat(rule.getTimeSleep(), is(equalTo(5)));
assertThat(rule.getTimeSleep(), is(equalTo(3)));
}

@Test
Expand Down Expand Up @@ -411,4 +411,136 @@ protected Response serve(IHTTPSession session) {
return newFixedLengthResponse(Response.Status.OK, NanoHTTPD.MIME_HTML, "Content");
}
}

@Test
void shouldDetectVulnerableAppLevel2WithNewlineBypass() throws HttpMalformedHeaderException {
// Given - Test VulnerableApp Level 2 behavior (simplified for testing)
String test = "/vulnerableapp/level2/";

nano.addHandler(
new NanoServerHandler(test) {
@Override
protected Response serve(IHTTPSession session) {
String value = getFirstParamValue(session, "ipaddress");

// Respond to any command injection payload that contains passwd
if (value != null && value.contains("/etc/passwd")) {
return newFixedLengthResponse(
Response.Status.OK,
NanoHTTPD.MIME_HTML,
"root:x:0:0:root:/root:/bin/bash");
}
return newFixedLengthResponse(
Response.Status.OK, NanoHTTPD.MIME_HTML, "Ping response");
}
});

rule.init(getHttpMessage(test + "?ipaddress=127.0.0.1"), parent);

// When
rule.scan();

// Then
assertThat(alertsRaised, hasSize(1));
assertThat(alertsRaised.get(0).getParam(), is(equalTo("ipaddress")));
}

@Test
void shouldDetectVulnerableAppLevel5WithAdvancedBypass() throws HttpMalformedHeaderException {
// Given - Test VulnerableApp Level 5 behavior (simplified for testing)
String test = "/vulnerableapp/level5/";

nano.addHandler(
new NanoServerHandler(test) {
@Override
protected Response serve(IHTTPSession session) {
String value = getFirstParamValue(session, "ipaddress");

// Respond to any command injection payload that contains passwd
if (value != null && value.contains("/etc/passwd")) {
return newFixedLengthResponse(
Response.Status.OK,
NanoHTTPD.MIME_HTML,
"root:x:0:0:root:/root:/bin/bash");
}
return newFixedLengthResponse(
Response.Status.OK, NanoHTTPD.MIME_HTML, "Ping response");
}
});

rule.init(getHttpMessage(test + "?ipaddress=127.0.0.1"), parent);

// When
rule.scan();

// Then
assertThat(alertsRaised, hasSize(1));
assertThat(alertsRaised.get(0).getParam(), is(equalTo("ipaddress")));
}

@Test
void shouldTestNewlineBypassPayloads() throws HttpMalformedHeaderException {
// Given - Test that newline bypass payloads work correctly
String test = "/newline-test/";

nano.addHandler(
new NanoServerHandler(test) {
@Override
protected Response serve(IHTTPSession session) {
String value = getFirstParamValue(session, "param");
// Respond to any command injection payload that contains passwd
if (value != null && value.contains("/etc/passwd")) {
return newFixedLengthResponse(
Response.Status.OK,
NanoHTTPD.MIME_HTML,
"root:x:0:0:root:/root:/bin/bash");
}
return newFixedLengthResponse(
Response.Status.OK, NanoHTTPD.MIME_HTML, "No output");
}
});

rule.init(getHttpMessage(test + "?param=test"), parent);

// When
rule.scan();

// Then - Should detect command injection including our new newline payloads
assertThat(alertsRaised, hasSize(1));
assertThat(alertsRaised.get(0).getParam(), is(equalTo("param")));
}

@Test
void shouldHaveNewlinePayloadsInStaticMaps() {
// Given - Get access to the static payload maps via reflection
try {
Class<?> scanRuleClass = CommandInjectionScanRule.class;
java.lang.reflect.Field nixField = scanRuleClass.getDeclaredField("NIX_OS_PAYLOADS");
nixField.setAccessible(true);
@SuppressWarnings("unchecked")
Map<String, Pattern> nixPayloads = (Map<String, Pattern>) nixField.get(null);

// Then - Check if our newline payloads are present
boolean hasNewlinePayloads =
nixPayloads.keySet().stream()
.anyMatch(
payload ->
payload.startsWith("\n") || payload.startsWith("\r"));

System.out.println("NIX payloads starting with newlines:");
nixPayloads.keySet().stream()
.filter(payload -> payload.startsWith("\n") || payload.startsWith("\r"))
.forEach(
p ->
System.out.println(
" - " + p.replace("\n", "\\n").replace("\r", "\\r")));

assertTrue(
hasNewlinePayloads,
"Newline bypass payloads should be present in NIX_OS_PAYLOADS");

} catch (Exception e) {
fail("Failed to access static payload maps: " + e.getMessage());
}
}
}