fix(java): catch chained ProcessBuilder command injection

[Eljees]

Fixes #3814

Problem

The existing rule misses chained calls where the command itself is dynamic:

code	before	after
new ProcessBuilder().command(userInput)	missed	reported
new ProcessBuilder().command(userInput.split(" "))	missed	reported
new ProcessBuilder(userInput) (control)	reported	reported
new ProcessBuilder("ls", userInput)	not reported	not reported
builder.command("java", "-jar", userInput)	not reported	not reported
new ProcessBuilder("ls", "-la")	not reported	not reported

The command() branch requires pattern-inside: $TYPE $PB = new ProcessBuilder(...), so a chained call on a fresh builder has no matching assignment.

Change

Add a chained-receiver branch for new ProcessBuilder().command(...), carrying the same literal/Arrays.asList/String[] exclusions as the existing branch.

After review, the proposed later-argument branch was removed: ProcessBuilder passes separate arguments directly to the child process, so a dynamic later argument is not command injection unless the executable is itself a shell. The existing sh -c / cmd /c branches continue to cover those cases. Tests now explicitly mark ordinary later dynamic arguments as ok to prevent that false positive from returning.

Tests

semgrep --test java/lang/security/audit/command-injection-process-builder.java --config java/lang/security/audit/command-injection-process-builder.yaml
1/1: ✓ All tests passed

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c1feef21a7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Eljees]

This is a fair challenge and I want to be straight about the trade-off rather than defend the patch reflexively.

The security point is correct: ProcessBuilder does not go through a shell, so each argument is passed to execve as-is. new ProcessBuilder("ls", "-al", targetDirectory) is not command injection — no metacharacter in targetDirectory will start a second command. Genuine injection through a later argument needs a shell in front (sh -c, cmd /c), and the rule already has a dedicated branch for exactly that.

What a dynamic later argument can be is argument injection: a value that starts with - or -- can change the invoked program's behaviour (--output=/etc/..., -o ProxyCommand=... for ssh, and so on). That is a real and commonly-missed weakness, but it is a different class from CWE-78, and this rule's message and metadata are about command injection.

So there are two defensible outcomes and I'd rather you pick:

1. Keep the branch, on the grounds that the rule lives in audit/ with confidence: LOW — it's a "look at this call" signal, and the issue reporter listed these exact shapes as expected detections.
2. Drop the later-argument branch from this PR and keep only the chained-receiver fix (cases 1 and 2 of [Java ] command-injection-process-builder missing common injection patterns such as chained call #3814), which is unambiguous: new ProcessBuilder().command(userInput) puts the dynamic value in the command position, which the rule already treats as a finding when written as new ProcessBuilder(userInput).

I lean towards 2 — narrower and free of the semantic objection above — and I'm happy to push that immediately if you agree. Cases 3 and 4 would then be better served by a separate argument-injection rule with its own CWE, rather than being folded into this one.

[Eljees]

Ping — open since 28 July, no review yet. Checks are green on c76a179.

This catches new ProcessBuilder().command(userInput), which the rule misses today because the command() branch requires pattern-inside: $TYPE $PB = new ProcessBuilder(...). The shapes that should stay silent are in the test file as controls.

One caveat I would rather state than bury: the review point about later arguments is correct. ProcessBuilder does not go through a shell, so a dynamic later argument is argument injection, not command injection, and this PR does not claim otherwise — the shell case already has its own branch. Detail in the thread. Happy to rework or close it.