Skip to content

Commit e992d81

Browse files
RitaChen609RitaChen609
authored
MLE-30237 fix serverStartStop to prevent OS command injection (#1946)
* MLE-30237 fix string comparison operator in serverStartStop to prevent OS command injection * MLE-30237 add -- after ssh to terminate option parsing
1 parent 523b444 commit e992d81

3 files changed

Lines changed: 179 additions & 22 deletions

File tree

‎marklogic-client-api-functionaltests/src/test/java/com/marklogic/client/datamovement/functionaltests/QBFailover.java‎

Lines changed: 34 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2010-2025 Progress Software Corporation and/or its subsidiaries or affiliates. All Rights Reserved.
2+
* Copyright (c) 2010-2026 Progress Software Corporation and/or its subsidiaries or affiliates. All Rights Reserved.
33
*/
44
package com.marklogic.client.datamovement.functionaltests;
55

@@ -55,6 +55,24 @@ public class QBFailover extends BasicJavaClientREST {
5555
private static String[] hostNames;
5656
private static JobTicket ticket;
5757
private static final String TEST_DIR_PREFIX = "/WriteHostBatcher-testdata/";
58+
// Hostname must start with alphanumeric to prevent leading-dash SSH option injection
59+
private static final Pattern HOSTNAME_PATTERN = Pattern.compile("[A-Za-z0-9][A-Za-z0-9._-]*");
60+
private static final ProcessStarter DEFAULT_PROCESS_STARTER =
61+
(server, commandToRun) -> new ProcessBuilder("ssh", "--", server, commandToRun).start();
62+
private static ProcessStarter processStarter = DEFAULT_PROCESS_STARTER;
63+
64+
@FunctionalInterface
65+
interface ProcessStarter {
66+
Process start(String server, String commandToRun) throws Exception;
67+
}
68+
69+
static void setProcessStarterForTest(ProcessStarter starter) {
70+
processStarter = starter;
71+
}
72+
73+
static void resetProcessStarterForTest() {
74+
processStarter = DEFAULT_PROCESS_STARTER;
75+
}
5876

5977
@BeforeAll
6078
public static void setUpBeforeClass() throws Exception {
@@ -999,21 +1017,26 @@ public void processEvent(QueryBatcher batcher) {
9991017

10001018
private void serverStartStop(String server, String command) throws Exception {
10011019
System.out.println("Preparing to " + command + " " + server);
1002-
String commandtoRun = null;
1020+
if (command == null) {
1021+
throw new IllegalArgumentException("Invalid command: null");
1022+
}
1023+
if (server == null || !HOSTNAME_PATTERN.matcher(server).matches()) {
1024+
throw new IllegalArgumentException("Suspicious hostname: " + server);
1025+
}
10031026

1004-
if (OS.indexOf("win") >= 0) {
1005-
commandtoRun = "net " + command.toLowerCase() + " MarkLogic";
1006-
} else {
1007-
commandtoRun = "sudo mladmin " + command.toLowerCase();
1027+
String cmd = command.toLowerCase(Locale.ROOT);
1028+
if (!cmd.equals("start") && !cmd.equals("stop")) {
1029+
throw new IllegalArgumentException("Invalid command: " + command);
10081030
}
10091031

1010-
if (command.toLowerCase() != "start" && command.toLowerCase() != "stop") {
1011-
System.out.println("Invalid Command");
1012-
return;
1032+
String commandtoRun;
10131033

1034+
if (OS.indexOf("win") >= 0) {
1035+
commandtoRun = "net " + cmd + " MarkLogic";
1036+
} else {
1037+
commandtoRun = "sudo mladmin " + cmd;
10141038
}
1015-
String[] temp = { "sh", "-c", "ssh " + server + " " + commandtoRun };
1016-
Process proc = Runtime.getRuntime().exec(temp);
1039+
Process proc = processStarter.start(server, commandtoRun);
10171040
BufferedReader stdOut = new BufferedReader(new InputStreamReader(proc.getInputStream()));
10181041
BufferedReader stdError = new BufferedReader(new InputStreamReader(proc.getErrorStream()));
10191042
String s = null;

‎marklogic-client-api-functionaltests/src/test/java/com/marklogic/client/datamovement/functionaltests/ServerStartStopSecurityTest.java‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
/*
2+
* Copyright (c) 2010-2026 Progress Software Corporation and/or its subsidiaries or affiliates. All Rights Reserved.
3+
*/
4+
package com.marklogic.client.datamovement.functionaltests;
5+
6+
import org.junit.jupiter.api.AfterEach;
7+
import org.junit.jupiter.api.Test;
8+
9+
import java.lang.reflect.InvocationTargetException;
10+
import java.lang.reflect.Method;
11+
import java.util.concurrent.atomic.AtomicInteger;
12+
13+
import static org.junit.jupiter.api.Assertions.assertEquals;
14+
import static org.junit.jupiter.api.Assertions.assertThrows;
15+
16+
public class ServerStartStopSecurityTest {
17+
18+
@AfterEach
19+
void resetProcessStarters() {
20+
WBFailover.resetProcessStarterForTest();
21+
QBFailover.resetProcessStarterForTest();
22+
}
23+
24+
@Test
25+
void wbServerStartStopRejectsSuspiciousHostnameBeforeProcessStart() {
26+
AtomicInteger processStartCount = new AtomicInteger(0);
27+
WBFailover.setProcessStarterForTest((server, commandToRun) -> {
28+
processStartCount.incrementAndGet();
29+
throw new AssertionError("Process creation should not be attempted for invalid hostname");
30+
});
31+
32+
assertThrows(IllegalArgumentException.class,
33+
() -> invokeServerStartStop(new WBFailover(), "host; echo pwned", "start"));
34+
assertEquals(0, processStartCount.get());
35+
}
36+
37+
@Test
38+
void wbServerStartStopRejectsInvalidCommandBeforeProcessStart() {
39+
AtomicInteger processStartCount = new AtomicInteger(0);
40+
WBFailover.setProcessStarterForTest((server, commandToRun) -> {
41+
processStartCount.incrementAndGet();
42+
throw new AssertionError("Process creation should not be attempted for invalid command");
43+
});
44+
45+
assertThrows(IllegalArgumentException.class,
46+
() -> invokeServerStartStop(new WBFailover(), "host", "reboot"));
47+
assertEquals(0, processStartCount.get());
48+
}
49+
50+
@Test
51+
void qbServerStartStopRejectsSuspiciousHostnameBeforeProcessStart() {
52+
AtomicInteger processStartCount = new AtomicInteger(0);
53+
QBFailover.setProcessStarterForTest((server, commandToRun) -> {
54+
processStartCount.incrementAndGet();
55+
throw new AssertionError("Process creation should not be attempted for invalid hostname");
56+
});
57+
58+
assertThrows(IllegalArgumentException.class,
59+
() -> invokeServerStartStop(new QBFailover(), "host; echo pwned", "start"));
60+
assertEquals(0, processStartCount.get());
61+
}
62+
63+
@Test
64+
void qbServerStartStopRejectsInvalidCommandBeforeProcessStart() {
65+
AtomicInteger processStartCount = new AtomicInteger(0);
66+
QBFailover.setProcessStarterForTest((server, commandToRun) -> {
67+
processStartCount.incrementAndGet();
68+
throw new AssertionError("Process creation should not be attempted for invalid command");
69+
});
70+
71+
assertThrows(IllegalArgumentException.class,
72+
() -> invokeServerStartStop(new QBFailover(), "host", "reboot"));
73+
assertEquals(0, processStartCount.get());
74+
}
75+
76+
@Test
77+
void wbServerStartStopRejectsLeadingDashHostnameBeforeProcessStart() {
78+
AtomicInteger processStartCount = new AtomicInteger(0);
79+
WBFailover.setProcessStarterForTest((server, commandToRun) -> {
80+
processStartCount.incrementAndGet();
81+
throw new AssertionError("Process creation should not be attempted for leading-dash hostname");
82+
});
83+
84+
assertThrows(IllegalArgumentException.class,
85+
() -> invokeServerStartStop(new WBFailover(), "-oProxyCommand=id", "start"));
86+
assertEquals(0, processStartCount.get());
87+
}
88+
89+
@Test
90+
void qbServerStartStopRejectsLeadingDashHostnameBeforeProcessStart() {
91+
AtomicInteger processStartCount = new AtomicInteger(0);
92+
QBFailover.setProcessStarterForTest((server, commandToRun) -> {
93+
processStartCount.incrementAndGet();
94+
throw new AssertionError("Process creation should not be attempted for leading-dash hostname");
95+
});
96+
97+
assertThrows(IllegalArgumentException.class,
98+
() -> invokeServerStartStop(new QBFailover(), "-oProxyCommand=id", "start"));
99+
assertEquals(0, processStartCount.get());
100+
}
101+
102+
private void invokeServerStartStop(Object target, String server, String command) throws Throwable {
103+
Method method = target.getClass().getDeclaredMethod("serverStartStop", String.class, String.class);
104+
method.setAccessible(true);
105+
try {
106+
method.invoke(target, server, command);
107+
} catch (InvocationTargetException ex) {
108+
throw ex.getCause();
109+
}
110+
}
111+
}

‎marklogic-client-api-functionaltests/src/test/java/com/marklogic/client/datamovement/functionaltests/WBFailover.java‎

Lines changed: 34 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2010-2025 Progress Software Corporation and/or its subsidiaries or affiliates. All Rights Reserved.
2+
* Copyright (c) 2010-2026 Progress Software Corporation and/or its subsidiaries or affiliates. All Rights Reserved.
33
*/
44
package com.marklogic.client.datamovement.functionaltests;
55

@@ -31,6 +31,8 @@ public class WBFailover extends BasicJavaClientREST {
3131
private static String dbName = "WBFailover";
3232
private static DataMovementManager dmManager;
3333
private static final String OS = System.getProperty("os.name").toLowerCase();
34+
// Hostname must start with alphanumeric to prevent leading-dash SSH option injection
35+
private static final Pattern HOSTNAME_PATTERN = Pattern.compile("[A-Za-z0-9][A-Za-z0-9._-]*");
3436

3537
private static DatabaseClient dbClient;
3638
private static DatabaseClient evalClient;
@@ -45,6 +47,22 @@ public class WBFailover extends BasicJavaClientREST {
4547
private static JobTicket writeTicket;
4648
final String query1 = "fn:count(fn:doc())";
4749
private static List<String> hostLists;
50+
private static final ProcessStarter DEFAULT_PROCESS_STARTER =
51+
(server, commandToRun) -> new ProcessBuilder("ssh", "--", server, commandToRun).start();
52+
private static ProcessStarter processStarter = DEFAULT_PROCESS_STARTER;
53+
54+
@FunctionalInterface
55+
interface ProcessStarter {
56+
Process start(String server, String commandToRun) throws Exception;
57+
}
58+
59+
static void setProcessStarterForTest(ProcessStarter starter) {
60+
processStarter = starter;
61+
}
62+
63+
static void resetProcessStarterForTest() {
64+
processStarter = DEFAULT_PROCESS_STARTER;
65+
}
4866

4967
@BeforeAll
5068
public static void setUpBeforeClass() throws Exception {
@@ -732,21 +750,26 @@ public void testWhiteBlackListNPE() {
732750

733751
private void serverStartStop(String server, String command) throws Exception {
734752
System.out.println("Preparing to " + command + " " + server);
735-
String commandtoRun = null;
753+
if (command == null) {
754+
throw new IllegalArgumentException("Invalid command: null");
755+
}
756+
if (server == null || !HOSTNAME_PATTERN.matcher(server).matches()) {
757+
throw new IllegalArgumentException("Suspicious hostname: " + server);
758+
}
736759

737-
if (OS.indexOf("win") >= 0) {
738-
commandtoRun = "net " + command.toLowerCase() + " MarkLogic";
739-
} else {
740-
commandtoRun = "sudo mladmin " + command.toLowerCase();
760+
String cmd = command.toLowerCase(Locale.ROOT);
761+
if (!cmd.equals("start") && !cmd.equals("stop")) {
762+
throw new IllegalArgumentException("Invalid command: " + command);
741763
}
742764

743-
if (command.toLowerCase() != "start" && command.toLowerCase() != "stop") {
744-
System.out.println("Invalid Command");
745-
return;
765+
String commandtoRun;
746766

767+
if (OS.indexOf("win") >= 0) {
768+
commandtoRun = "net " + cmd + " MarkLogic";
769+
} else {
770+
commandtoRun = "sudo mladmin " + cmd;
747771
}
748-
String[] temp = { "sh", "-c", "ssh " + server + " " + commandtoRun };
749-
Process proc = Runtime.getRuntime().exec(temp);
772+
Process proc = processStarter.start(server, commandtoRun);
750773
BufferedReader stdOut = new BufferedReader(new InputStreamReader(proc.getInputStream()));
751774
BufferedReader stdError = new BufferedReader(new InputStreamReader(proc.getErrorStream()));
752775
String s = null;

0 commit comments

Comments
 (0)