Skip to content

Commit a7c2a25

Browse files
atorralbaatorralba
authored
Merge pull request #12879 from atorralba/atorralba/java/command-injection-mad-sinks
Java: Convert all command injection sinks to MaD format
2 parents 6e20bd0 + ffe6768 commit a7c2a25

File tree

13 files changed

+91
-102
lines changed

13 files changed

+91
-102
lines changed

‎java/ql/lib/change-notes/2023-04-19-deprecated-execcallable.md

+4
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: deprecated
3+
---
4+
* The `ExecCallable` class in `ExternalProcess.qll` has been deprecated.

‎java/ql/lib/ext/experimental/com.jcraft.jsch.model.yml

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
extensions:
2+
- addsTo:
3+
pack: codeql/java-all
4+
extensible: experimentalSinkModel
5+
data:
6+
- ["com.jcraft.jsch", "ChannelExec", True, "setCommand", "", "", "Argument[0]", "command-injection", "manual", "jsch-os-injection"]

‎java/ql/lib/ext/java.lang.model.yml

+16-16
Original file line numberDiff line numberDiff line change
@@ -8,30 +8,30 @@ extensions:
88
- ["java.lang", "ClassLoader", True, "getSystemResource", "(String)", "", "Argument[0]", "path-injection", "ai-manual"]
99
- ["java.lang", "ClassLoader", True, "getSystemResourceAsStream", "(String)", "", "Argument[0]", "path-injection", "ai-manual"]
1010
- ["java.lang", "Module", True, "getResourceAsStream", "(String)", "", "Argument[0]", "path-injection", "ai-manual"]
11+
- ["java.lang", "ProcessBuilder", False, "command", "(List)", "", "Argument[0]", "command-injection", "manual"]
12+
- ["java.lang", "ProcessBuilder", False, "command", "(String[])", "", "Argument[0]", "command-injection", "ai-manual"]
13+
- ["java.lang", "ProcessBuilder", False, "directory", "(File)", "", "Argument[0]", "command-injection", "ai-manual"]
14+
- ["java.lang", "ProcessBuilder", False, "ProcessBuilder", "(List)", "", "Argument[0]", "command-injection", "ai-manual"]
15+
- ["java.lang", "ProcessBuilder", False, "ProcessBuilder", "(String[])", "", "Argument[0]", "command-injection", "ai-manual"]
16+
- ["java.lang", "Runtime", True, "exec", "(String)", "", "Argument[0]", "command-injection", "ai-manual"]
17+
- ["java.lang", "Runtime", True, "exec", "(String[])", "", "Argument[0]", "command-injection", "ai-manual"]
18+
- ["java.lang", "Runtime", True, "exec", "(String[],String[])", "", "Argument[0]", "command-injection", "ai-manual"]
19+
- ["java.lang", "Runtime", True, "exec", "(String[],String[],File)", "", "Argument[0]", "command-injection", "ai-manual"]
20+
- ["java.lang", "Runtime", True, "exec", "(String[],String[],File)", "", "Argument[2]", "command-injection", "ai-manual"]
21+
- ["java.lang", "Runtime", True, "exec", "(String,String[])", "", "Argument[0]", "command-injection", "ai-manual"]
22+
- ["java.lang", "Runtime", True, "exec", "(String,String[],File)", "", "Argument[0]", "command-injection", "ai-manual"]
23+
- ["java.lang", "Runtime", True, "exec", "(String,String[],File)", "", "Argument[2]", "command-injection", "ai-manual"]
1124
# These are potential vulnerabilities, but not for command-injection. No query for this kind of vulnerability currently exists.
1225
# - ["java.lang", "Runtime", False, "load", "(String)", "", "Argument[0]", "command-injection", "ai-manual"]
1326
# - ["java.lang", "Runtime", False, "loadLibrary", "(String)", "", "Argument[0]", "command-injection", "ai-manual"]
14-
# These are modeled in plain CodeQL. TODO: migrate them.
15-
# - ["java.lang", "ProcessBuilder", False, "command", "(String[])", "", "Argument[0]", "command-injection", "ai-manual"]
16-
# - ["java.lang", "ProcessBuilder", False, "directory", "(File)", "", "Argument[0]", "command-injection", "ai-manual"]
17-
# - ["java.lang", "ProcessBuilder", False, "ProcessBuilder", "(List)", "", "Argument[0]", "command-injection", "ai-manual"]
18-
# - ["java.lang", "ProcessBuilder", False, "ProcessBuilder", "(String[])", "", "Argument[0]", "command-injection", "ai-manual"]
19-
# - ["java.lang", "Runtime", True, "exec", "(String,String[])", "", "Argument[0]", "command-injection", "ai-manual"]
20-
# - ["java.lang", "Runtime", True, "exec", "(String[],String[])", "", "Argument[0]", "command-injection", "ai-manual"]
21-
# - ["java.lang", "Runtime", True, "exec", "(String,String[],File)", "", "Argument[0]", "command-injection", "ai-manual"]
22-
# - ["java.lang", "Runtime", True, "exec", "(String,String[],File)", "", "Argument[2]", "command-injection", "ai-manual"]
23-
# - ["java.lang", "Runtime", True, "exec", "(String)", "", "Argument[0]", "command-injection", "ai-manual"]
24-
# - ["java.lang", "Runtime", True, "exec", "(String[],String[],File)", "", "Argument[0]", "command-injection", "ai-manual"]
25-
# - ["java.lang", "Runtime", True, "exec", "(String[],String[],File)", "", "Argument[2]", "command-injection", "ai-manual"]
26-
# - ["java.lang", "Runtime", True, "exec", "(String[])", "", "Argument[0]", "command-injection", "ai-manual"]
2727
- ["java.lang", "String", False, "matches", "(String)", "", "Argument[0]", "regex-use[f-1]", "manual"]
2828
- ["java.lang", "String", False, "replaceAll", "(String,String)", "", "Argument[0]", "regex-use[-1]", "manual"]
2929
- ["java.lang", "String", False, "replaceFirst", "(String,String)", "", "Argument[0]", "regex-use[-1]", "manual"]
3030
- ["java.lang", "String", False, "split", "(String)", "", "Argument[0]", "regex-use[-1]", "manual"]
3131
- ["java.lang", "String", False, "split", "(String,int)", "", "Argument[0]", "regex-use[-1]", "manual"]
32-
# These are modeled in plain CodeQL. TODO: migrate them.
33-
# - ["java.lang", "System", False, "load", "(String)", "", "Argument[0]", "command-injection", "ai-manual"] # This is actually injecting a library.
34-
# - ["java.lang", "System", False, "loadLibrary", "(String)", "", "Argument[0]", "command-injection", "ai-manual"] # This is actually injecting a library.
32+
# These are potential vulnerabilities, but not for command-injection. No query for this kind of vulnerability currently exists.
33+
# - ["java.lang", "System", False, "load", "(String)", "", "Argument[0]", "command-injection", "ai-manual"]
34+
# - ["java.lang", "System", False, "loadLibrary", "(String)", "", "Argument[0]", "command-injection", "ai-manual"]
3535
- ["java.lang", "System$Logger", True, "log", "(Level,Object)", "", "Argument[1]", "log-injection", "manual"]
3636
- ["java.lang", "System$Logger", True, "log", "(Level,ResourceBundle,String,Object[])", "", "Argument[2..3]", "log-injection", "manual"]
3737
- ["java.lang", "System$Logger", True, "log", "(Level,ResourceBundle,String,Throwable)", "", "Argument[2]", "log-injection", "manual"]

‎java/ql/lib/ext/org.apache.commons.exec.model.yml

+11
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
extensions:
2+
- addsTo:
3+
pack: codeql/java-all
4+
extensible: sinkModel
5+
data:
6+
- ["org.apache.commons.exec", "CommandLine", True, "parse", "(String)", "", "Argument[0]", "command-injection", "manual"]
7+
- ["org.apache.commons.exec", "CommandLine", True, "parse", "(String,Map)", "", "Argument[0]", "command-injection", "manual"]
8+
- ["org.apache.commons.exec", "CommandLine", True, "addArguments", "(String)", "", "Argument[0]", "command-injection", "manual"]
9+
- ["org.apache.commons.exec", "CommandLine", True, "addArguments", "(String,boolean)", "", "Argument[0]", "command-injection", "manual"]
10+
- ["org.apache.commons.exec", "CommandLine", True, "addArguments", "(String[])", "", "Argument[0]", "command-injection", "manual"]
11+
- ["org.apache.commons.exec", "CommandLine", True, "addArguments", "(String[],boolean)", "", "Argument[0]", "command-injection", "manual"]

‎java/ql/lib/semmle/code/java/JDK.qll

+6-6
Original file line numberDiff line numberDiff line change
@@ -199,18 +199,18 @@ class TypeFile extends Class {
199199

200200
// --- Standard methods ---
201201
/**
202-
* Any constructor of class `java.lang.ProcessBuilder`.
202+
* DEPRECATED: Any constructor of class `java.lang.ProcessBuilder`.
203203
*/
204-
class ProcessBuilderConstructor extends Constructor, ExecCallable {
204+
deprecated class ProcessBuilderConstructor extends Constructor, ExecCallable {
205205
ProcessBuilderConstructor() { this.getDeclaringType() instanceof TypeProcessBuilder }
206206

207207
override int getAnExecutedArgument() { result = 0 }
208208
}
209209

210210
/**
211-
* Any of the methods named `command` on class `java.lang.ProcessBuilder`.
211+
* DEPRECATED: Any of the methods named `command` on class `java.lang.ProcessBuilder`.
212212
*/
213-
class MethodProcessBuilderCommand extends Method, ExecCallable {
213+
deprecated class MethodProcessBuilderCommand extends Method, ExecCallable {
214214
MethodProcessBuilderCommand() {
215215
this.hasName("command") and
216216
this.getDeclaringType() instanceof TypeProcessBuilder
@@ -220,9 +220,9 @@ class MethodProcessBuilderCommand extends Method, ExecCallable {
220220
}
221221

222222
/**
223-
* Any method named `exec` on class `java.lang.Runtime`.
223+
* DEPRECATED: Any method named `exec` on class `java.lang.Runtime`.
224224
*/
225-
class MethodRuntimeExec extends Method, ExecCallable {
225+
deprecated class MethodRuntimeExec extends Method, ExecCallable {
226226
MethodRuntimeExec() {
227227
this.hasName("exec") and
228228
this.getDeclaringType() instanceof TypeRuntime

‎java/ql/lib/semmle/code/java/frameworks/apache/Exec.qll

-29
This file was deleted.

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

+4-6
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@
1010
import java
1111
private import semmle.code.java.dataflow.FlowSources
1212
private import semmle.code.java.dataflow.ExternalFlow
13-
private import semmle.code.java.security.ExternalProcess
1413
private import semmle.code.java.security.CommandArguments
14+
private import semmle.code.java.security.ExternalProcess
1515

1616
/** A sink for command injection vulnerabilities. */
1717
abstract class CommandInjectionSink extends DataFlow::Node { }
@@ -33,9 +33,7 @@ class CommandInjectionAdditionalTaintStep extends Unit {
3333
}
3434

3535
private class DefaultCommandInjectionSink extends CommandInjectionSink {
36-
DefaultCommandInjectionSink() {
37-
this.asExpr() instanceof ArgumentToExec or sinkNode(this, "command-injection")
38-
}
36+
DefaultCommandInjectionSink() { sinkNode(this, "command-injection") }
3937
}
4038

4139
private class DefaultCommandInjectionSanitizer extends CommandInjectionSanitizer {
@@ -100,7 +98,7 @@ predicate execIsTainted(
10098
RemoteUserInputToArgumentToExecFlow::PathNode sink, Expr execArg
10199
) {
102100
RemoteUserInputToArgumentToExecFlow::flowPath(source, sink) and
103-
sink.getNode().asExpr() = execArg
101+
argumentToExec(execArg, sink.getNode())
104102
}
105103

106104
/**
@@ -112,7 +110,7 @@ predicate execIsTainted(
112110
*/
113111
deprecated predicate execTainted(DataFlow::PathNode source, DataFlow::PathNode sink, Expr execArg) {
114112
exists(RemoteUserInputToArgumentToExecFlowConfig conf |
115-
conf.hasFlowPath(source, sink) and sink.getNode().asExpr() = execArg
113+
conf.hasFlowPath(source, sink) and argumentToExec(execArg, sink.getNode())
116114
)
117115
}
118116

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

+17-14
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,13 @@
11
/** Definitions related to external processes. */
22

33
import semmle.code.java.Member
4-
5-
private module Instances {
6-
private import semmle.code.java.JDK
7-
private import semmle.code.java.frameworks.apache.Exec
8-
}
4+
private import semmle.code.java.dataflow.DataFlow
5+
private import semmle.code.java.security.CommandLineQuery
96

107
/**
11-
* A callable that executes a command.
8+
* DEPRECATED: A callable that executes a command.
129
*/
13-
abstract class ExecCallable extends Callable {
10+
abstract deprecated class ExecCallable extends Callable {
1411
/**
1512
* Gets the index of an argument that will be part of the command that is executed.
1613
*/
@@ -23,13 +20,19 @@ abstract class ExecCallable extends Callable {
2320
* to be executed.
2421
*/
2522
class ArgumentToExec extends Expr {
26-
ArgumentToExec() {
27-
exists(Call execCall, ExecCallable execCallable, int i |
28-
execCall.getArgument(pragma[only_bind_into](i)) = this and
29-
execCallable = execCall.getCallee() and
30-
i = execCallable.getAnExecutedArgument()
31-
)
32-
}
23+
ArgumentToExec() { argumentToExec(this, _) }
24+
}
25+
26+
/**
27+
* Holds if `e` is an expression used as an argument to a call that executes an external command.
28+
* For calls to varargs method calls, this only includes the first argument, which will be the command
29+
* to be executed.
30+
*/
31+
predicate argumentToExec(Expr e, CommandInjectionSink s) {
32+
s.asExpr() = e
33+
or
34+
e.(Argument).isNthVararg(0) and
35+
s.(DataFlow::ImplicitVarargsArray).getCall() = e.(Argument).getCall()
3336
}
3437

3538
/**

‎java/ql/src/Security/CWE/CWE-078/ExecTaintedLocal.ql

+7-4
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,14 @@
1414

1515
import java
1616
import semmle.code.java.security.CommandLineQuery
17+
import semmle.code.java.security.ExternalProcess
1718
import LocalUserInputToArgumentToExecFlow::PathGraph
1819

1920
from
2021
LocalUserInputToArgumentToExecFlow::PathNode source,
21-
LocalUserInputToArgumentToExecFlow::PathNode sink
22-
where LocalUserInputToArgumentToExecFlow::flowPath(source, sink)
23-
select sink.getNode().asExpr(), source, sink, "This command line depends on a $@.",
24-
source.getNode(), "user-provided value"
22+
LocalUserInputToArgumentToExecFlow::PathNode sink, Expr e
23+
where
24+
LocalUserInputToArgumentToExecFlow::flowPath(source, sink) and
25+
argumentToExec(e, sink.getNode())
26+
select e, source, sink, "This command line depends on a $@.", source.getNode(),
27+
"user-provided value"

‎java/ql/src/Security/CWE/CWE-078/ExecUnescaped.ql

+1
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
import java
1616
import semmle.code.java.security.CommandLineQuery
17+
import semmle.code.java.security.ExternalProcess
1718

1819
/**
1920
* Strings that are known to be sane by some simple local analysis. Such strings

‎java/ql/src/experimental/Security/CWE/CWE-078/ExecTainted.ql

+5-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,11 @@
1515
import java
1616
import semmle.code.java.security.CommandLineQuery
1717
import RemoteUserInputToArgumentToExecFlow::PathGraph
18-
import JSchOSInjection
18+
private import semmle.code.java.dataflow.ExternalFlow
19+
20+
private class ActivateModels extends ActiveExperimentalModels {
21+
ActivateModels() { this = "jsch-os-injection" }
22+
}
1923

2024
// This is a clone of query `java/command-line-injection` that also includes experimental sinks.
2125
from

‎java/ql/src/experimental/Security/CWE/CWE-078/JSchOSInjection.qll

-20
This file was deleted.

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

+14-6
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,28 @@
11
edges
2-
| Test.java:6:35:6:44 | arg : String | Test.java:7:44:7:69 | ... + ... |
2+
| Test.java:6:35:6:44 | arg : String | Test.java:7:44:7:69 | ... + ... : String |
33
| Test.java:6:35:6:44 | arg : String | Test.java:10:61:10:73 | ... + ... : String |
44
| Test.java:6:35:6:44 | arg : String | Test.java:16:13:16:25 | ... + ... : String |
55
| Test.java:6:35:6:44 | arg : String | Test.java:22:15:22:27 | ... + ... : String |
6+
| Test.java:7:25:7:70 | new ..[] { .. } : String[] [[]] : String | Test.java:7:25:7:70 | new ..[] { .. } |
7+
| Test.java:7:44:7:69 | ... + ... : String | Test.java:7:25:7:70 | new ..[] { .. } : String[] [[]] : String |
68
| Test.java:10:29:10:74 | {...} : String[] [[]] : String | Test.java:10:29:10:74 | new String[] |
79
| Test.java:10:61:10:73 | ... + ... : String | Test.java:10:29:10:74 | {...} : String[] [[]] : String |
810
| Test.java:16:5:16:7 | cmd [post update] : ArrayList [<element>] : String | Test.java:18:29:18:31 | cmd |
911
| Test.java:16:13:16:25 | ... + ... : String | Test.java:16:5:16:7 | cmd [post update] : ArrayList [<element>] : String |
1012
| Test.java:22:5:22:8 | cmd1 [post update] : String[] [[]] : String | Test.java:24:29:24:32 | cmd1 |
1113
| Test.java:22:15:22:27 | ... + ... : String | Test.java:22:5:22:8 | cmd1 [post update] : String[] [[]] : String |
12-
| Test.java:28:38:28:47 | arg : String | Test.java:29:44:29:64 | ... + ... |
14+
| Test.java:28:38:28:47 | arg : String | Test.java:29:44:29:64 | ... + ... : String |
15+
| Test.java:29:25:29:65 | new ..[] { .. } : String[] [[]] : String | Test.java:29:25:29:65 | new ..[] { .. } |
16+
| Test.java:29:44:29:64 | ... + ... : String | Test.java:29:25:29:65 | new ..[] { .. } : String[] [[]] : String |
1317
| Test.java:57:27:57:39 | args : String[] | Test.java:60:20:60:22 | arg : String |
1418
| Test.java:57:27:57:39 | args : String[] | Test.java:61:23:61:25 | arg : String |
1519
| Test.java:60:20:60:22 | arg : String | Test.java:6:35:6:44 | arg : String |
1620
| Test.java:61:23:61:25 | arg : String | Test.java:28:38:28:47 | arg : String |
1721
nodes
1822
| Test.java:6:35:6:44 | arg : String | semmle.label | arg : String |
19-
| Test.java:7:44:7:69 | ... + ... | semmle.label | ... + ... |
23+
| Test.java:7:25:7:70 | new ..[] { .. } | semmle.label | new ..[] { .. } |
24+
| Test.java:7:25:7:70 | new ..[] { .. } : String[] [[]] : String | semmle.label | new ..[] { .. } : String[] [[]] : String |
25+
| Test.java:7:44:7:69 | ... + ... : String | semmle.label | ... + ... : String |
2026
| Test.java:10:29:10:74 | new String[] | semmle.label | new String[] |
2127
| Test.java:10:29:10:74 | {...} : String[] [[]] : String | semmle.label | {...} : String[] [[]] : String |
2228
| Test.java:10:61:10:73 | ... + ... : String | semmle.label | ... + ... : String |
@@ -27,14 +33,16 @@ nodes
2733
| Test.java:22:15:22:27 | ... + ... : String | semmle.label | ... + ... : String |
2834
| Test.java:24:29:24:32 | cmd1 | semmle.label | cmd1 |
2935
| Test.java:28:38:28:47 | arg : String | semmle.label | arg : String |
30-
| Test.java:29:44:29:64 | ... + ... | semmle.label | ... + ... |
36+
| Test.java:29:25:29:65 | new ..[] { .. } | semmle.label | new ..[] { .. } |
37+
| Test.java:29:25:29:65 | new ..[] { .. } : String[] [[]] : String | semmle.label | new ..[] { .. } : String[] [[]] : String |
38+
| Test.java:29:44:29:64 | ... + ... : String | semmle.label | ... + ... : String |
3139
| Test.java:57:27:57:39 | args : String[] | semmle.label | args : String[] |
3240
| Test.java:60:20:60:22 | arg : String | semmle.label | arg : String |
3341
| Test.java:61:23:61:25 | arg : String | semmle.label | arg : String |
3442
subpaths
3543
#select
36-
| Test.java:7:44:7:69 | ... + ... | Test.java:57:27:57:39 | args : String[] | Test.java:7:44:7:69 | ... + ... | This command line depends on a $@. | Test.java:57:27:57:39 | args | user-provided value |
44+
| Test.java:7:44:7:69 | ... + ... | Test.java:57:27:57:39 | args : String[] | Test.java:7:25:7:70 | new ..[] { .. } | This command line depends on a $@. | Test.java:57:27:57:39 | args | user-provided value |
3745
| Test.java:10:29:10:74 | new String[] | Test.java:57:27:57:39 | args : String[] | Test.java:10:29:10:74 | new String[] | This command line depends on a $@. | Test.java:57:27:57:39 | args | user-provided value |
3846
| Test.java:18:29:18:31 | cmd | Test.java:57:27:57:39 | args : String[] | Test.java:18:29:18:31 | cmd | This command line depends on a $@. | Test.java:57:27:57:39 | args | user-provided value |
3947
| Test.java:24:29:24:32 | cmd1 | Test.java:57:27:57:39 | args : String[] | Test.java:24:29:24:32 | cmd1 | This command line depends on a $@. | Test.java:57:27:57:39 | args | user-provided value |
40-
| Test.java:29:44:29:64 | ... + ... | Test.java:57:27:57:39 | args : String[] | Test.java:29:44:29:64 | ... + ... | This command line depends on a $@. | Test.java:57:27:57:39 | args | user-provided value |
48+
| Test.java:29:44:29:64 | ... + ... | Test.java:57:27:57:39 | args : String[] | Test.java:29:25:29:65 | new ..[] { .. } | This command line depends on a $@. | Test.java:57:27:57:39 | args | user-provided value |

0 commit comments

Comments
 (0)