Skip to content

Commit 8f39336

Browse files
Copilotowen-mc
andauthored
Address launcher review feedback
Co-authored-by: owen-mc <62447351+owen-mc@users.noreply.github.com>
1 parent 866d93b commit 8f39336

2 files changed

Lines changed: 11 additions & 7 deletions

File tree

‎java/ql/lib/semmle/code/java/security/CommandArguments.qll‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ private predicate isShell(Expr ex) {
7171
*/
7272
private predicate isDelegatingLauncher(Expr ex) {
7373
commandName(ex)
74-
.regexpMatch("(.*/)?(busybox|chroot|doas|env|flock|ionice|nice|nohup|runuser|script|setsid|stdbuf|su|sudo|taskset|time|timeout|unbuffer|watch|xargs)$")
74+
.regexpMatch("(.*/)?(busybox|chroot|doas|env|flock|ionice|nice|nohup|runuser|script|setsid|stdbuf|su|sudo|taskset|time|timeout|unbuffer|watch|xargs)(\\.exe)?$")
7575
}
7676

7777
private string commandName(Expr ex) {
@@ -104,9 +104,10 @@ private string fixedCommandName(Expr ex) {
104104
exists(SsaDefinition ssa |
105105
ex = ssa.getARead() and
106106
result =
107-
fixedCommandName(any(SsaExplicitWrite def | def = ultimateDefinition(ssa)).getDefiningExpr())
107+
fixedCommandName(any(ultimateDefinition(ssa).(SsaExplicitWrite)).getDefiningExpr())
108108
|
109109
forall(SsaDefinition def | def = ultimateDefinition(ssa) |
110+
def instanceof SsaExplicitWrite and
110111
fixedCommandName(def.(SsaExplicitWrite).getDefiningExpr()) = result
111112
)
112113
)
@@ -161,9 +162,11 @@ private class CommandArguments extends TCommandArguments {
161162
/**
162163
* Gets a command operand after skipping only understood options and required operands.
163164
* Shell-interpreting forms (such as `su -c`, `watch`, and `script -c`) are not skipped.
165+
* @param launcher The basename of the launcher.
166+
* @param index The argument index to inspect.
167+
* @param options Whether launcher options may be skipped.
164168
*/
165169
private int commandOperand(string launcher, int index, boolean options) {
166-
launcher = this.getFixedArgument(_).regexpCapture("(.*/)?([^/]+)", 2) and
167170
options = [true, false] and
168171
exists(Expr arg | arg = this.getArgument(index) |
169172
(options = false or not this.getFixedArgument(index).matches("-%")) and
@@ -189,7 +192,7 @@ private class CommandArguments extends TCommandArguments {
189192

190193
private int delegatedCommand(int index) {
191194
exists(string launcher |
192-
launcher = this.getFixedArgument(index).regexpCapture("(.*/)?([^/]+)", 2) and
195+
launcher = this.getFixedArgument(index).regexpCapture("(.*/)?([^/.]+)(\\.exe)?$", 2) and
193196
isDelegatingLauncher(this.getArgument(index)) and
194197
(
195198
exists(int start |
@@ -230,7 +233,7 @@ private class CommandArguments extends TCommandArguments {
230233
predicate hasSafeArgument(int index) { this.safeAfter(0, index) }
231234
}
232235

233-
private ArrayInit arrayInitializerOf(Expr ex) { result = containerOrigin(ex).(ArrayInit) }
236+
private ArrayInit arrayInitializerOf(Expr ex) { result = containerOrigin(ex) }
234237

235238
/** Gets a container's allocation or initializer, including through local aliases. */
236239
private Expr containerOrigin(Expr ex) {
@@ -347,7 +350,7 @@ private predicate launcherOption(string launcher, string option, int width) {
347350
or
348351
launcher = "sudo" and option = ["-u", "-g", "--user", "--group"]
349352
or
350-
launcher = "doas" and option = ["-u"]
353+
launcher = "doas" and option = "-u"
351354
or
352355
launcher = "time" and option = ["-o", "-f", "--output", "--format"]
353356
or

‎java/ql/test/query-tests/security/CWE-078/Test.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ public static void nonShellCommand(String arg) throws java.io.IOException {
5252
public static void delegatingLaunchers(String arg) throws java.io.IOException {
5353
new ProcessBuilder(new String[]{"chroot", "/tmp/root", arg}).start(); // $ Alert[java/command-line-injection] Alert[java/relative-path-command]
5454
new ProcessBuilder(new String[]{"env", arg}).start(); // $ Alert[java/command-line-injection] Alert[java/relative-path-command]
55+
new ProcessBuilder(new String[]{"env.exe", arg}).start(); // $ Alert[java/command-line-injection] Alert[java/relative-path-command]
5556
new ProcessBuilder(new String[]{"nice", arg}).start(); // $ Alert[java/command-line-injection] Alert[java/relative-path-command]
5657
new ProcessBuilder(new String[]{"nohup", arg}).start(); // $ Alert[java/command-line-injection] Alert[java/relative-path-command]
5758
new ProcessBuilder(new String[]{"setsid", arg}).start(); // $ Alert[java/command-line-injection] Alert[java/relative-path-command]
@@ -131,7 +132,7 @@ public static void launcherArguments(String arg) throws java.io.IOException {
131132
new ProcessBuilder(changedLiteral).start(); // $ Alert[java/command-line-injection]
132133

133134
String[] unknownIndex = new String[]{"/usr/bin/env", "/bin/cp", "-c", arg};
134-
unknownIndex[arg.length()] = "/bin/sh";
135+
unknownIndex[arg.length() % unknownIndex.length] = "/bin/sh";
135136
new ProcessBuilder(unknownIndex).start(); // $ Alert[java/command-line-injection]
136137

137138
String[] conditionalArray = new String[]{"/usr/bin/env", "/bin/sh", "-c", "<command>"};

0 commit comments

Comments
 (0)