Checkmarx command line injection

Question

We are using ProcessBuilder in Java to execute Python program on a Linux machine. We added all validation suggested by Checkmarx and then we tried to execute the below sample provided by Checkmarx.

public String executeSystemCommand_WithParameters(HttpServletRequest request)
            throws ServletException, IOException {
        String commandResult = "";
        String userCommand = request.getParameter("Command");
        userCommand = userCommand.replaceAll("[^A-Za-z0-9]", "");
        try {
            ProcessBuilder builder = new ProcessBuilder("/bin/sh/","-c", PROGRAM_NAME,
                    userCommand);
            Map<String, String> environ = builder.environment();
            setEnvironmentVars(environ);
            Process subProc = builder.start();
            BufferedReader irProcOutput = new BufferedReader(new
                    InputStreamReader(subProc.getInputStream()));
            String line = null;
            while ((line = irProcOutput.readLine()) != null)
                commandResult += line;
            irProcOutput.close();
        } catch (Exception ex) {
            handleExceptions(ex);
        }
        return commandResult;
    }

However, this sample provided by Checkmarx as a resolution also produces the same defect. Please advise what can be changed in the below program so it will resolve the command line injection vulnerability.

Answer 1

In reviewing the queries, replaceAll is not considered a sanitizer for command injection. The documentation may be out of date.

While in this case replaceAll would sanitize the command injection, the problem is that static analysis won't understand what is filtered from the string based on the regex specification. The regex in the example would remove non-alphanumeric characters, but what if it were considered a sanitizer and someone removed the regex?

Consider the following code:

import java.io.IOException;
import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.lang.ProcessBuilder;
import java.util.Map;
import org.owasp.esapi.Encoder;
import org.owasp.esapi.ESAPI;
import org.owasp.esapi.codecs.UnixCodec;
import org.owasp.esapi.codecs.Codec;

class Main {

  private static Encoder _enc = ESAPI.encoder();
  private static Codec _codec = new UnixCodec();

  public static String executeSystemCommand_sortofsafe(HttpServletRequest request) {
    String commandResult = "";
    String userCommand = request.getParameter("Command");
    userCommand = userCommand.replaceAll("[^A-Za-z0-9]", "");
    try {
      ProcessBuilder builder = new ProcessBuilder("/bin/sh", "-c", userCommand);
      Process subProc = builder.start();
      BufferedReader irProcOutput = new BufferedReader(new InputStreamReader(subProc.getInputStream()));
      String line = null;
      while ((line = irProcOutput.readLine()) != null)
        commandResult += line;
      irProcOutput.close();
    } catch (Exception ex) {
      System.out.println(ex.getMessage());
    }
    return commandResult;
  }

  public static String executeSystemCommand_samebutnotsafe(HttpServletRequest request) {
    String commandResult = "";
    String userCommand = request.getParameter("Command");
    // modify the regex to thwart sanitization
    userCommand = userCommand.replaceAll("", "");
    try {
      ProcessBuilder builder = new ProcessBuilder("/bin/sh", "-c", userCommand);
      Process subProc = builder.start();
      BufferedReader irProcOutput = new BufferedReader(new InputStreamReader(subProc.getInputStream()));
      String line = null;
      while ((line = irProcOutput.readLine()) != null)
        commandResult += line;
      irProcOutput.close();
    } catch (Exception ex) {
      System.out.println(ex.getMessage());
    }
    return commandResult;
  }

  public static String executeSystemCommand_safe(HttpServletRequest request) {
    String commandResult = "";
    String userCommand = request.getParameter("Command");
    // This is verifiable by static analysis, not subject to regex changes.
    userCommand = _enc.encodeForOS(_codec, userCommand);
    try {
      ProcessBuilder builder = new ProcessBuilder("/bin/sh", "-c", userCommand);
      Process subProc = builder.start();
      BufferedReader irProcOutput = new BufferedReader(new InputStreamReader(subProc.getInputStream()));
      String line = null;
      while ((line = irProcOutput.readLine()) != null)
        commandResult += line;
      irProcOutput.close();
    } catch (Exception ex) {
      System.out.println(ex.getMessage());
    }
    return commandResult;
  }

}

The method executeSystemCommand_sortofsafe is an exact copy of the documented example. In this case, it is a false-positive.

Now consider the method executeSystemCommand_samebutnotsafe. The code is the same, but the regex is removed. This causes the input to not be sanitized. If replaceAll was considered a sanitizer, this would be a false-negative. Static analysis doesn't run the code with sample payloads to know how the regex mutates the input string.

One method of sanitizing the code is shown in method executeSystemCommand_safe. The ESAPI library was used to produce a verifiable sanitization method. The ESAPI library is not subject to local code changes that would change the logic as would be the case when the regex is incorrectly (or even maliciously) modified.

If you scan this code example, you'll see 2 command injection results that flow through replaceAll but you won't see a result for the example using ESAPI. I verified that the queries find Encoder.encodeForOS as a sanitizer.