Skip to content

Commit a7c2a25

Browse files
atorralbaatorralba
authored
Merge pull request github#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

Lines changed: 4 additions & 0 deletions
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

Lines changed: 6 additions & 0 deletions
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

Lines changed: 16 additions & 16 deletions
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

Lines changed: 11 additions & 0 deletions
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

Lines changed: 6 additions & 6 deletions
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

Lines changed: 0 additions & 29 deletions
This file was deleted.

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

Lines changed: 4 additions & 6 deletions
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

Lines changed: 17 additions & 14 deletions
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

Lines changed: 7 additions & 4 deletions
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

Lines changed: 1 addition & 0 deletions
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

0 commit comments

Comments
 (0)