Skip to content

Commit 866d93b

Browse files
Copilotowen-mc
Copilot
and
owen-mc
authored
Keep launcher sanitization conservative across SSA and container aliases
Co-authored-by: owen-mc <62447351+owen-mc@users.noreply.github.com>
1 parent edb67fe commit 866d93b

4 files changed

Lines changed: 367 additions & 94 deletions

File tree

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

Lines changed: 114 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,8 @@ predicate isSafeCommandArgument(Expr ex) {
2121
not isDelegatingLauncher(ai.getInit(0))
2222
or
2323
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-
)
24+
not writesCommandPrefix(_, ai, i) and
25+
not arrayEscapes(ai)
3426
)
3527
)
3628
or
@@ -55,7 +47,12 @@ predicate isSafeCommandArgument(Expr ex) {
5547
(
5648
not isDelegatingLauncher(caa.getFirstElement())
5749
or
58-
TMutableArguments(caa).(CommandArguments).hasSafeArgument(i)
50+
TMutableArguments(caa).(CommandArguments).hasSafeArgument(i) and
51+
not arrayEscapes(containerOrigin(caa.getDefiningExpr())) and
52+
not exists(ArrayAccess access |
53+
writesCommandPrefix(access, containerOrigin(caa.getDefiningExpr()), i) and
54+
not access.getArray() = caa.getARead()
55+
)
5956
)
6057
)
6158
)
@@ -65,22 +62,7 @@ predicate isSafeCommandArgument(Expr ex) {
6562
* Holds if the given expression is the name of a shell command such as bash or python
6663
*/
6764
private predicate isShell(Expr ex) {
68-
exists(string cmd | cmd = ex.(StringLiteral).getValue() |
69-
cmd.regexpMatch(".*(sh|javac?|python[23]?|osascript|cmd)(\\.exe)?$")
70-
)
71-
or
72-
exists(SsaDefinition ssa, SsaExplicitWrite def |
73-
ex = ssa.getARead() and
74-
isShell(def.getDefiningExpr())
75-
|
76-
ssa.getAnUltimateDefinition() = def
77-
or
78-
ssa.(SsaCapturedDefinition).getAnUltimateCapturedDefinition() = def
79-
)
80-
or
81-
isShell(ex.(Assignment).getRhs())
82-
or
83-
isShell(ex.(LocalVariableDeclExpr).getInit())
65+
commandName(ex).regexpMatch(".*(sh|javac?|python[23]?|osascript|cmd)(\\.exe)?$")
8466
}
8567

8668
/**
@@ -95,20 +77,45 @@ private predicate isDelegatingLauncher(Expr ex) {
9577
private string commandName(Expr ex) {
9678
result = ex.(StringLiteral).getValue()
9779
or
98-
exists(SsaDefinition ssa, SsaExplicitWrite def |
80+
exists(SsaDefinition ssa |
9981
ex = ssa.getARead() and
100-
result = commandName(def.getDefiningExpr())
101-
|
102-
ssa.getAnUltimateDefinition() = def
103-
or
104-
ssa.(SsaCapturedDefinition).getAnUltimateCapturedDefinition() = def
82+
result = commandName(ultimateDefinition(ssa).(SsaExplicitWrite).getDefiningExpr())
10583
)
10684
or
10785
result = commandName(ex.(Assignment).getRhs())
10886
or
10987
result = commandName(ex.(LocalVariableDeclExpr).getInit())
11088
}
11189

90+
private SsaDefinition ultimateDefinition(SsaDefinition ssa) {
91+
result = ssa.getAnUltimateDefinition() and
92+
not result instanceof SsaCapturedDefinition
93+
or
94+
result =
95+
ultimateDefinition(ssa.getAnUltimateDefinition()
96+
.(SsaCapturedDefinition)
97+
.getAnUltimateCapturedDefinition())
98+
}
99+
100+
/** Gets a command name only when every reaching definition resolves to the same constant. */
101+
private string fixedCommandName(Expr ex) {
102+
result = ex.(StringLiteral).getValue()
103+
or
104+
exists(SsaDefinition ssa |
105+
ex = ssa.getARead() and
106+
result =
107+
fixedCommandName(any(SsaExplicitWrite def | def = ultimateDefinition(ssa)).getDefiningExpr())
108+
|
109+
forall(SsaDefinition def | def = ultimateDefinition(ssa) |
110+
fixedCommandName(def.(SsaExplicitWrite).getDefiningExpr()) = result
111+
)
112+
)
113+
or
114+
result = fixedCommandName(ex.(Assignment).getRhs())
115+
or
116+
result = fixedCommandName(ex.(LocalVariableDeclExpr).getInit())
117+
}
118+
112119
private newtype TCommandArguments =
113120
TLiteralArguments(ArrayInit init) or
114121
TListArguments(CommandArgumentList list) or
@@ -123,6 +130,7 @@ private class CommandArguments extends TCommandArguments {
123130
or
124131
exists(CommandArgumentList list |
125132
this = TListArguments(list) and
133+
not listEscapes(list) and
126134
forall(MethodCall add | add = list.getAnAdd() |
127135
add.getMethod().hasName("add") and add.getNumArgument() = 1
128136
) and
@@ -146,11 +154,8 @@ private class CommandArguments extends TCommandArguments {
146154

147155
/** Gets a constant shared by every possible value at this position. */
148156
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-
)
157+
result = fixedCommandName(this.getArgument(index)) and
158+
forall(Expr value | value = this.getArgument(index) | fixedCommandName(value) = result)
154159
}
155160

156161
/**
@@ -225,14 +230,82 @@ private class CommandArguments extends TCommandArguments {
225230
predicate hasSafeArgument(int index) { this.safeAfter(0, index) }
226231
}
227232

228-
private ArrayInit arrayInitializerOf(Expr ex) {
233+
private ArrayInit arrayInitializerOf(Expr ex) { result = containerOrigin(ex).(ArrayInit) }
234+
235+
/** Gets a container's allocation or initializer, including through local aliases. */
236+
private Expr containerOrigin(Expr ex) {
229237
result = ex.(ArrayInit)
230238
or
239+
result = ex.(ClassInstanceExpr)
240+
or
241+
ex.getUnderlyingExpr() != ex and
242+
result = containerOrigin(ex.getUnderlyingExpr())
243+
or
244+
result = containerOrigin(ex.(ConditionalExpr).getBranchExpr(_))
245+
or
231246
result = ex.(ArrayCreationExpr).getInit()
232247
or
233-
result = arrayInitializerOf(ex.(LocalVariableDeclExpr).getInit())
248+
ex instanceof ArrayCreationExpr and
249+
not exists(ex.(ArrayCreationExpr).getInit()) and
250+
result = ex
234251
or
235-
result = arrayInitializerOf(ex.(Assignment).getRhs())
252+
result = containerOrigin(ex.(LocalVariableDeclExpr).getInit())
253+
or
254+
result = containerOrigin(ex.(Assignment).getRhs())
255+
or
256+
exists(SsaDefinition ssa |
257+
ex = ssa.getARead() and
258+
result = containerOrigin(ultimateDefinition(ssa).(SsaExplicitWrite).getDefiningExpr())
259+
)
260+
}
261+
262+
/** Holds if the array may be mutated through a use not covered by the local alias analysis. */
263+
private predicate arrayEscapes(Expr origin) {
264+
containerIsPassedOut(origin)
265+
or
266+
containerOrigin(any(MethodCall call).getQualifier()) = origin
267+
}
268+
269+
private predicate listEscapes(CommandArgumentList list) {
270+
containerIsPassedOut(containerOrigin(list.getDefiningExpr()))
271+
or
272+
exists(MethodCall call |
273+
containerOrigin(call.getQualifier()) = containerOrigin(list.getDefiningExpr()) and
274+
not call = list.getAnAdd()
275+
)
276+
}
277+
278+
private predicate containerIsPassedOut(Expr origin) {
279+
exists(Call call |
280+
containerOrigin(call.getAnArgument()) = origin and
281+
not (
282+
call.getCallee().getDeclaringType().hasQualifiedName("java.lang", "ProcessBuilder") and
283+
(call.getCallee() instanceof Constructor or call.getCallee().hasName("command"))
284+
or
285+
call.getCallee().getDeclaringType().hasQualifiedName("java.lang", "Runtime") and
286+
call.getCallee().hasName("exec")
287+
)
288+
)
289+
or
290+
containerOrigin(any(ReturnStmt ret).getResult()) = origin
291+
or
292+
containerOrigin(any(ArrayInit init).getAnInit()) = origin
293+
or
294+
exists(Assignment assignment |
295+
containerOrigin(assignment.getRhs()) = origin and
296+
(assignment.getDest() instanceof FieldAccess or assignment.getDest() instanceof ArrayAccess)
297+
)
298+
}
299+
300+
bindingset[origin, index]
301+
private predicate writesCommandPrefix(ArrayAccess access, Expr origin, int index) {
302+
containerOrigin(access.getArray()) = origin and
303+
arrayVarWrite(access) and
304+
(
305+
not access.getIndexExpr() instanceof CompileTimeConstantExpr
306+
or
307+
access.getIndexExpr().(CompileTimeConstantExpr).getIntValue() < index
308+
)
236309
}
237310

238311
/** The width of an understood launcher option, including a separate option operand. */

‎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:174:46:174:49 | "ls" | Command with a relative path 'ls' is executed. |
12+
| Test.java:271:46:271:49 | "ls" | Command with a relative path 'ls' is executed. |

0 commit comments

Comments
 (0)