Repository navigation
Conversation
Co-authored-by: owen-mc <62447351+owen-mc@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| */ | ||
| private predicate isDelegatingLauncher(Expr ex) { | ||
| exists(string cmd | cmd = ex.(StringLiteral).getValue() | | ||
| cmd.regexpMatch("(.*/)?(chroot|env|nice|nohup|setsid|stdbuf|sudo|timeout|xargs)$") |
| i > 0 and | ||
| not isShell(ai.getInit(0)) | ||
| not isShell(ai.getInit(0)) and | ||
| not isDelegatingLauncher(ai.getInit(0)) |
There was a problem hiding this comment.
The new check disables sanitization for every argument after a launcher. For example:
new ProcessBuilder(new String[]{"env", "/bin/cp", userInput, "/tmp/destination"});
This now produces java/command-line-injection, although userInput remains a discrete argument to the fixed /bin/cp executable. This contradicts the safe direct-cp behavior tested at Test.java:48 and introduces false positives into a high-precision query.
Make launcher handling position-aware: flag the operand selecting the delegated command, then preserve normal safe-argument handling once a fixed non-shell command is known. Add GOOD cases for array literals, lists, and mutable arrays.
| */ | ||
| private predicate isDelegatingLauncher(Expr ex) { | ||
| exists(string cmd | cmd = ex.(StringLiteral).getValue() | | ||
| cmd.regexpMatch("(.*/)?(chroot|env|nice|nohup|setsid|stdbuf|sudo|timeout|xargs)$") |
There was a problem hiding this comment.
Common launchers such as flock, su, runuser, doas, and time remain unrecognized. For example, taint in {"flock", "/tmp/lock", userInput} is still sanitized even though userInput selects the executed command. This matches the existing unresolved review thread.
|
@copilot Please address the three comments on this PR. |
Co-authored-by: owen-mc <62447351+owen-mc@users.noreply.github.com>
Co-authored-by: owen-mc <62447351+owen-mc@users.noreply.github.com>
Addressed all three comments in edb67fe and 866d93b. Launcher handling now preserves safe arguments after fixed non-shell commands, recognizes the missing wrappers, and includes GOOD cases for literals, lists, and mutable arrays. Added safeguards and regressions for unresolved commands and mutated aliases. All four CWE-078 tests pass; final read-only review found no issues. The automated security scan was skipped because its database was too large. |
| ex = ssa.getARead() and | ||
| isShell(def.getDefiningExpr()) | ||
| result = | ||
| fixedCommandName(any(SsaExplicitWrite def | def = ultimateDefinition(ssa)).getDefiningExpr()) |
| * Gets a command operand after skipping only understood options and required operands. | ||
| * Shell-interpreting forms (such as `su -c`, `watch`, and `script -c`) are not skipped. | ||
| */ | ||
| private int commandOperand(string launcher, int index, boolean options) { |
| predicate hasSafeArgument(int index) { this.safeAfter(0, index) } | ||
| } | ||
|
|
||
| private ArrayInit arrayInitializerOf(Expr ex) { result = containerOrigin(ex).(ArrayInit) } |
| or | ||
| launcher = "sudo" and option = ["-u", "-g", "--user", "--group"] | ||
| or | ||
| launcher = "doas" and option = ["-u"] |
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
6 open findings
forallquantifiesSsaDefinition defbut then unconditionally castsdefto… · Newexists(...)is used with an expression (exists(this.getArgument(...))), which is not valid QL… · NewcommandOperandre-binds thelauncherparameter usingthis.getFixedArgument(_), which can… · Newexists(...)is used with an expression (exists(this.getArgument(...))), which is not valid QL… · NewisShellaccounts for Windows-style.exesuffixes, butisDelegatingLauncherdoes not. This… · NewunknownIndex[arg.length()]can throwArrayIndexOutOfBoundsExceptionfor most inputs (since the… · New
What changed in this PR
Updates the Java CWE-078 CodeQL modeling and tests to better distinguish delegating “launcher” executables (for example env, sudo, timeout) from shells, improving command-injection and relative-path detection accuracy.
Changes:
- Extend
isSafeCommandArgumentto handle delegating launchers by analyzing fixed command prefixes across arrays/lists/mutable arrays. - Add new test cases covering launcher delegation scenarios and update expected query outputs accordingly.
- Introduce new argument-sequence abstraction (
CommandArguments) and supporting utilities for alias/escape tracking.
| File | Description |
|---|---|
| java/ql/test/query-tests/security/CWE-078/Test.java | Adds new ProcessBuilder test cases for delegating launchers, argument mutation/aliasing, and fixed-vs-tainted scenarios. |
| java/ql/test/query-tests/security/CWE-078/ExecTainted.expected | Updates expected taint-tracking results to include new test cases and shifted line numbers. |
| java/ql/test/query-tests/security/CWE-078/ExecRelative.expected | Updates expected relative-path command results for newly added launcher cases and shifted line numbers. |
| java/ql/lib/semmle/code/java/security/CommandArguments.qll | Implements launcher-aware safe-argument logic, including prefix inspection and container escape tracking. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| fixedCommandName(any(SsaExplicitWrite def | def = ultimateDefinition(ssa)).getDefiningExpr()) | ||
| | | ||
| ssa.getAnUltimateDefinition() = def | ||
| forall(SsaDefinition def | def = ultimateDefinition(ssa) | |
| options = true and | ||
| exists(int width | | ||
| launcherOption(launcher, this.getFixedArgument(index), width) and | ||
| (width = 1 or exists(this.getArgument(index + 1))) and |
| launcher = this.getFixedArgument(_).regexpCapture("(.*/)?([^/]+)", 2) and | ||
| options = [true, false] and | ||
| exists(Expr arg | arg = this.getArgument(index) | | ||
| (options = false or not this.getFixedArgument(index).matches("-%")) and | ||
| not (launcher = "env" and this.getFixedArgument(index).matches("%=%")) and | ||
| result = index | ||
| or | ||
| options = true and | ||
| this.getFixedArgument(index) = "--" and | ||
| result = this.commandOperand(launcher, index + 1, false) | ||
| or | ||
| options = true and | ||
| exists(int width | | ||
| launcherOption(launcher, this.getFixedArgument(index), width) and | ||
| (width = 1 or exists(this.getArgument(index + 1))) and | ||
| result = this.commandOperand(launcher, index + width, true) | ||
| ) | ||
| or | ||
| launcher = "env" and | ||
| this.getFixedArgument(index).matches("%=%") and | ||
| result = this.commandOperand(launcher, index + 1, options) |
|
|
||
| private predicate safeAfter(int command, int argument) { | ||
| command < argument and | ||
| exists(this.getArgument(argument)) and |
| */ | ||
| private predicate isDelegatingLauncher(Expr ex) { | ||
| commandName(ex) | ||
| .regexpMatch("(.*/)?(busybox|chroot|doas|env|flock|ionice|nice|nohup|runuser|script|setsid|stdbuf|su|sudo|taskset|time|timeout|unbuffer|watch|xargs)$") |
| new ProcessBuilder(changedLiteral).start(); // $ Alert[java/command-line-injection] | ||
|
|
||
| String[] unknownIndex = new String[]{"/usr/bin/env", "/bin/cp", "-c", arg}; | ||
| unknownIndex[arg.length()] = "/bin/sh"; |
isSafeCommandArgumenttreated non-first arguments as safe whenever the executable was not a recognized shell. Delegating launchers could therefore execute attacker-controlled arguments without producing a command-line injection alert.Detection
env,sudo,xargs,chroot,nice,nohup,setsid,stdbuf, andtimeout.Coverage
cp.