Command Injection with ProcessBuilder?

ProcessBuilder is a Java class used to create Operating System processes. Therefore, needless to say, when coded insecurely it leads to serious security risks. In this post, we will go over an example code block utilizing ProcessBuilder and reveal a few security code best & bad practices through the analysis.

So, without further ado here’s the code block;

String path = "C:\\Windows\\system32\\cmd.exe";
ProcessBuilder pb = new ProcessBuilder(path);
pb.command().add("/c");
pb.command().add("ping.exe");
pb.command().add(userInput);
Process pingProcess = pb.start();

Let’s read and comment on this simple code. It takes a user input (userInput) as an argument and executes ping command with it.

Let’s see what Windows help documentation tells about the /c option given to cmd.exe;

Carries out the command specified by string and then terminates.

That’s cool. Before JDK 5.0, the popular way to code the same thing is something like this;

process = Runtime.getRuntime().exec(path + " /c ping.exe " + userInput);

Just looking both of the code pieces, we can easily resemblance the second one to inline SQL query executing APIs, while the first one is more like a PreparedStatement way of doing the same stuff.

We know that, Prepared Statements or Binding Variables are the secure way of executing SQL queries than dynamic SQL construction and execution. This is life or death situation especially involving with a user input.

Security analysis of the code

So, is the first code secure? Or let me put differently. Is it possible for a malicious user to execute a random OS command given that he can manipulate the userInput variable to his hearts content. When possible, this is called Command Injection, a popular and serious security weakness.

Well, what I like about dynamic testing is that you can always feed dangerous and smart strings to the running target code block and check the responses. Let’s list some of the strings that a tester can employ in order to understand if the original code leads to running the Windows calculator executable.

127.0.0.1 && calc.exe
127.0.0.1 & calc.exe
127.0.0.1 | calc.exe
127.0.0.1 || calc.exe
127.0.0.1 > calc.exe

But what I don’t like about dynamic testing is that, when all these fuzzing lead to nowhere, does that really mean we are secure?

Most of the time the same question is valid for static testing, too, but not this time since there are no dynamic string concatenation when building the command to execute. Still, does that really mean we are secure?

I hardly think so…

Why is our code immune to Command Injection?

Let’s first see why it’s impossible to run Windows calculator through this code.

In order to understand why, we have to delve into Java’s source code for ProcessBuilder. It’s somewhat a tedious job, however, here’s what it boils down to;

for (int i = 1; i < cmd.length; ++i) {
    cmdbuf.append(' ');
    String s = cmd[i];
    if (needsEscaping(verificationType, s)) {
       cmdbuf.append('"');
    ...

Here, the Java code appends double quotes to an array element, if it contains a space. The elements of the array I’m talking about here are;

[0] -> C:\\Windows\\system32\\cmd.exe
[1] -> /c
[2] -> ping.exe
[3] -> userInput

When the userInput contains spaces, the code wraps it inside double quotes, efficiently escaping it, so that it doesn’t escape the ping.exe :) So, the last array item “127.0.0.1 && calc.exe” will be passed to ping.exe as a whole parameter.

Here’s the screenshot when we debug the code with the malicious input and look at the value of the toString version of the array.

Of course, there are more than one control applied in ProcessBuilder, but this one is enough for us to understand.

While it’s impossible here to inject a command through the cmd.exe, it may still be possible to cause this code to behave malicious through the ping.exe. For example, a hacker can provide an input like “-n 1000” in order the command to run quite some time that is more than it should, causing perhaps a denial of service situation.

By the way -n argument for ping is the “Number of echo requests to send”. Here’s a simulation which obviously takes quite sometime to finish when provided with higher -n number.

C:\Users\CodeThreat>ping.exe -n 1000 127.0.0.1
Pinging 127.0.0.1 with 32 bytes of data:
Reply from 127.0.0.1: bytes=32 time<1ms TTL=128
Reply from 127.0.0.1: bytes=32 time<1ms TTL=128
Reply from 127.0.0.1: bytes=32 time<1ms TTL=128
Reply from 127.0.0.1: bytes=32 time<1ms TTL=128
Reply from 127.0.0.1: bytes=32 time<1ms TTL=128
Reply from 127.0.0.1: bytes=32 time<1ms TTL=128
Reply from 127.0.0.1: bytes=32 time<1ms TTL=128

As a side note, even if we pass some default hard coded arguments to ping.exe in the code, malicious user input may override those parameters and create risks. This is much like HTTP parameter pollution weakness.

Is ProcessBuilder a secure API, then?

Here’s food for thought. Is it possible for us to write an insecure code using ProcessBuilder? I’m always amazed how we developers can find ways to write insecure code using pretty decent APIs.

So, sure we can! Here it is;

String path = "C:\\Windows\\system32\\cmd.exe";
pb = new ProcessBuilder(path, "/c", "ping.exe " + userInput);
Process pingProcess = pb.start();

This time the Windows calculator works since the last array item becomes the whole command “ping.exe 127.0.0.1 && calc.exe”. This will be double quoted by the Java source code shown above and the shell will interpret it just fine.

This is not the only way. Here’s another one, but this time not so obvious;

String path = "C:\\Windows\\system32\\cmd.exe";
ProcessBuilder pb = new ProcessBuilder(path);
pb.command().add("/c");
pb.command().add("ping.exe");
pb.command().addAll(Arrays.asList(userInput.split("\\s")));
Process pingProcess = pb.start();

This last one is a killer. I’ll let you analyze why it’s insecure.

So, what to do with ProcessBuilder?

It’s wiser not using any shell support, cmd or sh or any other interpreter, when feeding the ProcessBuilder or Runtime.exec. However, as the commands that should be run gets more complex, we tend to choose the sophisticated shell and not write our own interpreter…

To this end, ProcessBuilder brings a more granular way of pulling this task off, however, still, as we can see from two examples we have given above, it’s easy to write vulnerable code. Ultimately, that means the responsibility of writing secure code is still on our shoulders. Check out this great SO answer to understand this dilemma.

What I’ll suggest in order to minimize the security risks here is using whitelisting as soon as we get the user input from outside of our trust boundary. I specifically did not use the word “Users” here, since the actor that sends this input might be a configuration file, a database row or even a response field of a B2B web service call…

By applying a whitelist we can easily validate an IP address and dodge all the fuzzy stuff I’ve gone through above!