Skip to content

Commit edb67fe

Browse files
Copilotowen-mc
Copilot
and
owen-mc
authored
Make delegated launcher sanitization position-aware
Co-authored-by: owen-mc <62447351+owen-mc@users.noreply.github.com>
1 parent 8848042 commit edb67fe

4 files changed

Lines changed: 559 additions & 36 deletions

File tree

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

Lines changed: 241 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,19 +17,47 @@ predicate isSafeCommandArgument(Expr ex) {
1717
ex = ai.getInit(i) and
1818
i > 0 and
1919
not isShell(ai.getInit(0)) and
20-
not isDelegatingLauncher(ai.getInit(0))
20+
(
21+
not isDelegatingLauncher(ai.getInit(0))
22+
or
23+
TLiteralArguments(ai).(CommandArguments).hasSafeArgument(i) and
24+
not exists(SsaExplicitWrite array, ArrayAccess access |
25+
arrayInitializerOf(array.getDefiningExpr()) = ai and
26+
access.getArray() = array.getARead() and
27+
arrayVarWrite(access) and
28+
(
29+
not access.getIndexExpr() instanceof CompileTimeConstantExpr
30+
or
31+
access.getIndexExpr().(CompileTimeConstantExpr).getIntValue() < i
32+
)
33+
)
34+
)
2135
)
2236
or
2337
exists(CommandArgumentList cal |
2438
not cal.isShell() and
25-
not isDelegatingLauncher(cal.getAFirstAdd().getArgument(0)) and
26-
ex = cal.getASubsequentAdd().getArgument(0)
39+
ex = cal.getASubsequentAdd().getArgument(0) and
40+
(
41+
not isDelegatingLauncher(cal.getAFirstAdd().getArgument(0))
42+
or
43+
exists(int i |
44+
ex = TListArguments(cal).(CommandArguments).getArgument(i) and
45+
TListArguments(cal).(CommandArguments).hasSafeArgument(i)
46+
)
47+
)
2748
)
2849
or
2950
exists(CommandArgArrayImmutableFirst caa |
3051
not caa.isShell() and
31-
not isDelegatingLauncher(caa.getFirstElement()) and
32-
ex = caa.getAWrite(any(int i | i > 0))
52+
exists(int i |
53+
i > 0 and
54+
ex = caa.getAWrite(i) and
55+
(
56+
not isDelegatingLauncher(caa.getFirstElement())
57+
or
58+
TMutableArguments(caa).(CommandArguments).hasSafeArgument(i)
59+
)
60+
)
3361
)
3462
}
3563

@@ -60,22 +88,203 @@ private predicate isShell(Expr ex) {
6088
* one of its arguments, rather than interpreting that command itself like a shell.
6189
*/
6290
private predicate isDelegatingLauncher(Expr ex) {
63-
exists(string cmd | cmd = ex.(StringLiteral).getValue() |
64-
cmd.regexpMatch("(.*/)?(chroot|env|nice|nohup|setsid|stdbuf|sudo|timeout|xargs)$")
65-
)
91+
commandName(ex)
92+
.regexpMatch("(.*/)?(busybox|chroot|doas|env|flock|ionice|nice|nohup|runuser|script|setsid|stdbuf|su|sudo|taskset|time|timeout|unbuffer|watch|xargs)$")
93+
}
94+
95+
private string commandName(Expr ex) {
96+
result = ex.(StringLiteral).getValue()
6697
or
6798
exists(SsaDefinition ssa, SsaExplicitWrite def |
6899
ex = ssa.getARead() and
69-
isDelegatingLauncher(def.getDefiningExpr())
100+
result = commandName(def.getDefiningExpr())
70101
|
71102
ssa.getAnUltimateDefinition() = def
72103
or
73104
ssa.(SsaCapturedDefinition).getAnUltimateCapturedDefinition() = def
74105
)
75106
or
76-
isDelegatingLauncher(ex.(Assignment).getRhs())
107+
result = commandName(ex.(Assignment).getRhs())
108+
or
109+
result = commandName(ex.(LocalVariableDeclExpr).getInit())
110+
}
111+
112+
private newtype TCommandArguments =
113+
TLiteralArguments(ArrayInit init) or
114+
TListArguments(CommandArgumentList list) or
115+
TMutableArguments(CommandArgArrayImmutableFirst array)
116+
117+
/** An ordered argument sequence whose fixed command prefixes can be inspected. */
118+
private class CommandArguments extends TCommandArguments {
119+
string toString() { result = this.getArgument(0).toString() }
120+
121+
Expr getArgument(int index) {
122+
exists(ArrayInit init | this = TLiteralArguments(init) | result = init.getInit(index))
123+
or
124+
exists(CommandArgumentList list |
125+
this = TListArguments(list) and
126+
forall(MethodCall add | add = list.getAnAdd() |
127+
add.getMethod().hasName("add") and add.getNumArgument() = 1
128+
) and
129+
result = list.getAnAddAtIndex(index).getArgument(0)
130+
)
131+
or
132+
exists(CommandArgArrayImmutableFirst array |
133+
this = TMutableArguments(array) and
134+
(
135+
index = 0 and result = array.getFirstElement()
136+
or
137+
index > 0 and
138+
(
139+
result = array.getAWrite(index)
140+
or
141+
result = arrayInitializerOf(array.getDefiningExpr()).getInit(index)
142+
)
143+
)
144+
)
145+
}
146+
147+
/** Gets a constant shared by every possible value at this position. */
148+
private string getFixedArgument(int index) {
149+
result = commandName(this.getArgument(index)) and
150+
forall(Expr value | value = this.getArgument(index) |
151+
commandName(value) = result and
152+
not exists(string other | commandName(value) = other and other != result)
153+
)
154+
}
155+
156+
/**
157+
* Gets a command operand after skipping only understood options and required operands.
158+
* Shell-interpreting forms (such as `su -c`, `watch`, and `script -c`) are not skipped.
159+
*/
160+
private int commandOperand(string launcher, int index, boolean options) {
161+
launcher = this.getFixedArgument(_).regexpCapture("(.*/)?([^/]+)", 2) and
162+
options = [true, false] and
163+
exists(Expr arg | arg = this.getArgument(index) |
164+
(options = false or not this.getFixedArgument(index).matches("-%")) and
165+
not (launcher = "env" and this.getFixedArgument(index).matches("%=%")) and
166+
result = index
167+
or
168+
options = true and
169+
this.getFixedArgument(index) = "--" and
170+
result = this.commandOperand(launcher, index + 1, false)
171+
or
172+
options = true and
173+
exists(int width |
174+
launcherOption(launcher, this.getFixedArgument(index), width) and
175+
(width = 1 or exists(this.getArgument(index + 1))) and
176+
result = this.commandOperand(launcher, index + width, true)
177+
)
178+
or
179+
launcher = "env" and
180+
this.getFixedArgument(index).matches("%=%") and
181+
result = this.commandOperand(launcher, index + 1, options)
182+
)
183+
}
184+
185+
private int delegatedCommand(int index) {
186+
exists(string launcher |
187+
launcher = this.getFixedArgument(index).regexpCapture("(.*/)?([^/]+)", 2) and
188+
isDelegatingLauncher(this.getArgument(index)) and
189+
(
190+
exists(int start |
191+
launcher =
192+
[
193+
"busybox", "doas", "env", "ionice", "nice", "nohup", "setsid", "stdbuf", "sudo",
194+
"time", "unbuffer", "xargs"
195+
] and
196+
start = index + 1
197+
or
198+
launcher = "runuser" and
199+
this.getFixedArgument(index + 1) = ["-u", "--user"] and
200+
start = index + 3
201+
|
202+
result = this.commandOperand(launcher, start, true)
203+
)
204+
or
205+
launcher = ["chroot", "flock", "taskset", "timeout"] and
206+
// Locate the required root, lock file, CPU mask, or duration first.
207+
result = this.commandOperand(launcher, index + 1, true) + 1 and
208+
not this.getFixedArgument(result).matches("-%")
209+
)
210+
)
211+
}
212+
213+
private predicate safeAfter(int command, int argument) {
214+
command < argument and
215+
exists(this.getArgument(argument)) and
216+
(
217+
exists(this.getFixedArgument(command)) and
218+
not isShell(this.getArgument(command)) and
219+
not isDelegatingLauncher(this.getArgument(command))
220+
or
221+
this.safeAfter(this.delegatedCommand(command), argument)
222+
)
223+
}
224+
225+
predicate hasSafeArgument(int index) { this.safeAfter(0, index) }
226+
}
227+
228+
private ArrayInit arrayInitializerOf(Expr ex) {
229+
result = ex.(ArrayInit)
230+
or
231+
result = ex.(ArrayCreationExpr).getInit()
232+
or
233+
result = arrayInitializerOf(ex.(LocalVariableDeclExpr).getInit())
234+
or
235+
result = arrayInitializerOf(ex.(Assignment).getRhs())
236+
}
237+
238+
/** The width of an understood launcher option, including a separate option operand. */
239+
bindingset[launcher, option]
240+
private predicate launcherOption(string launcher, string option, int width) {
241+
width = 1 and
242+
(
243+
launcher = "env" and option = ["-i", "--ignore-environment", "-0", "--null"]
244+
or
245+
launcher = "nice" and option.regexpMatch("-[0-9]+")
246+
or
247+
launcher = "setsid" and option = ["-c", "-f", "-w", "--ctty", "--fork", "--wait"]
248+
or
249+
launcher = "stdbuf" and option.regexpMatch("-[ioe].+")
250+
or
251+
launcher = "sudo" and option = ["-n", "-E", "-H", "--non-interactive"]
252+
or
253+
launcher = "doas" and option = ["-n", "-L"]
254+
or
255+
launcher = "time" and option = ["-p", "-v", "--portability", "--verbose"]
256+
or
257+
launcher = "taskset" and option = ["-a", "-c", "--all-tasks", "--cpu-list"]
258+
or
259+
launcher = "flock" and
260+
option = ["-s", "-x", "-n", "-o", "--shared", "--exclusive", "--nonblock", "--close"]
261+
or
262+
launcher = "timeout" and option = ["--foreground", "--preserve-status", "-v", "--verbose"]
263+
or
264+
launcher = "xargs" and option = ["-0", "-r", "-t", "--null", "--no-run-if-empty", "--verbose"]
265+
)
77266
or
78-
isDelegatingLauncher(ex.(LocalVariableDeclExpr).getInit())
267+
width = 2 and
268+
(
269+
launcher = "env" and option = ["-u", "--unset", "-C", "--chdir"]
270+
or
271+
launcher = "nice" and option = ["-n", "--adjustment"]
272+
or
273+
launcher = "ionice" and option = ["-c", "-n", "--class", "--classdata"]
274+
or
275+
launcher = "sudo" and option = ["-u", "-g", "--user", "--group"]
276+
or
277+
launcher = "doas" and option = ["-u"]
278+
or
279+
launcher = "time" and option = ["-o", "-f", "--output", "--format"]
280+
or
281+
launcher = "flock" and option = ["-w", "-E", "--timeout", "--conflict-exit-code"]
282+
or
283+
launcher = "timeout" and option = ["-s", "-k", "--signal", "--kill-after"]
284+
or
285+
launcher = "xargs" and
286+
option = ["-n", "-P", "-I", "-d", "--max-args", "--max-procs", "--replace", "--delimiter"]
287+
)
79288
}
80289

81290
/**
@@ -133,6 +342,27 @@ private class CommandArgumentList extends SsaExplicitWrite {
133342
not result = this.getAFirstAdd()
134343
}
135344

345+
/** Gets a single-element append in a straight-line prefix of additions. */
346+
MethodCall getAnAddAtIndex(int index) {
347+
result = this.getAnAdd() and
348+
result.getMethod().hasName("add") and
349+
result.getNumArgument() = 1 and
350+
index in [0 .. count(this.getAnAdd())] and
351+
(
352+
index = 0 and
353+
result = this.getAFirstAdd() and
354+
forall(MethodCall first | first = this.getAFirstAdd() | first = result)
355+
or
356+
exists(MethodCall previous |
357+
previous = this.getAnAddAtIndex(index - 1) and
358+
adjacentUseUse(previous.getQualifier(), result.getQualifier()) and
359+
forall(VarRead use | adjacentUseUse(use, result.getQualifier()) |
360+
use = previous.getQualifier()
361+
)
362+
)
363+
)
364+
}
365+
136366
/** Holds if the first element of this list is a shell command. */
137367
predicate isShell() {
138368
exists(MethodCall ma | ma = this.getAFirstAdd() and isShell(ma.getArgument(0)))

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,4 +9,4 @@
99
| Test.java:59:24:59:48 | new String[] | Command with a relative path 'sudo' is executed. |
1010
| Test.java:60:24:60:57 | new String[] | Command with a relative path 'timeout' is executed. |
1111
| Test.java:61:24:61:49 | new String[] | Command with a relative path 'xargs' is executed. |
12-
| Test.java:65:46:65:49 | "ls" | Command with a relative path 'ls' is executed. |
12+
| Test.java:174:46:174:49 | "ls" | Command with a relative path 'ls' is executed. |

0 commit comments

Comments
 (0)