Skip to content

Commit 534725f

Browse files
atorralbaatorralba
committed
Add command injection sink kind
1 parent aeaeade commit 534725f

File tree

6 files changed

+76
-45
lines changed

6 files changed

+76
-45
lines changed

‎java/ql/lib/change-notes/2023-03-28-cmdi-queries-models-as-data.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* A new models as data sink kind `command-injection` has been added.
5+
* The queries `java/command-line-injection` and `java/concatenated-command-line` now can be extended using the `command-injection` models as data sink kind.

‎java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,7 @@ module ModelValidation {
272272
"groovy", "xss", "ognl-injection", "intent-start", "pending-intent-sent",
273273
"url-open-stream", "url-redirect", "create-file", "read-file", "write-file",
274274
"set-hostname-verifier", "header-splitting", "information-leak", "xslt", "jexl",
275-
"bean-validation", "ssti", "fragment-injection"
275+
"bean-validation", "ssti", "fragment-injection", "command-injection"
276276
] and
277277
not kind.matches("regex-use%") and
278278
not kind.matches("qltest%") and

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

Lines changed: 67 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -7,30 +7,44 @@
77
* query.
88
*/
99

10-
import semmle.code.java.dataflow.FlowSources
11-
import semmle.code.java.security.ExternalProcess
12-
import semmle.code.java.security.CommandArguments
10+
import java
11+
private import semmle.code.java.dataflow.FlowSources
12+
private import semmle.code.java.dataflow.ExternalFlow
13+
private import semmle.code.java.security.ExternalProcess
14+
private import semmle.code.java.security.CommandArguments
15+
16+
/** A sink for command injection vulnerabilities. */
17+
abstract class CommandInjectionSink extends DataFlow::Node { }
18+
19+
/** A sanitizer for command injection vulnerabilities. */
20+
abstract class CommandInjectionSanitizer extends DataFlow::Node { }
1321

1422
/**
15-
* DEPRECATED: Use `RemoteUserInputToArgumentToExecFlow` instead.
23+
* A unit class for adding additional taint steps.
1624
*
17-
* A taint-tracking configuration for unvalidated user input that is used to run an external process.
25+
* Extend this class to add additional taint steps that should apply to configurations related to command injection.
1826
*/
19-
deprecated class RemoteUserInputToArgumentToExecFlowConfig extends TaintTracking::Configuration {
20-
RemoteUserInputToArgumentToExecFlowConfig() {
21-
this = "ExecCommon::RemoteUserInputToArgumentToExecFlowConfig"
22-
}
23-
24-
override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
27+
class CommandInjectionAdditionalTaintStep extends Unit {
28+
/**
29+
* Holds if the step from `node1` to `node2` should be considered a taint
30+
* step for configurations related to command injection.
31+
*/
32+
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
33+
}
2534

26-
override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof ArgumentToExec }
35+
private class DefaultCommandInjectionSink extends CommandInjectionSink {
36+
DefaultCommandInjectionSink() {
37+
this.asExpr() instanceof ArgumentToExec or sinkNode(this, "command-injection")
38+
}
39+
}
2740

28-
override predicate isSanitizer(DataFlow::Node node) {
29-
node.getType() instanceof PrimitiveType
41+
private class DefaultCommandInjectionSanitizer extends CommandInjectionSanitizer {
42+
DefaultCommandInjectionSanitizer() {
43+
this.getType() instanceof PrimitiveType
3044
or
31-
node.getType() instanceof BoxedType
45+
this.getType() instanceof BoxedType
3246
or
33-
isSafeCommandArgument(node.asExpr())
47+
isSafeCommandArgument(this.asExpr())
3448
}
3549
}
3650

@@ -40,14 +54,12 @@ deprecated class RemoteUserInputToArgumentToExecFlowConfig extends TaintTracking
4054
module RemoteUserInputToArgumentToExecFlowConfig implements DataFlow::ConfigSig {
4155
predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
4256

43-
predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof ArgumentToExec }
57+
predicate isSink(DataFlow::Node sink) { sink instanceof CommandInjectionSink }
4458

45-
predicate isBarrier(DataFlow::Node node) {
46-
node.getType() instanceof PrimitiveType
47-
or
48-
node.getType() instanceof BoxedType
49-
or
50-
isSafeCommandArgument(node.asExpr())
59+
predicate isBarrier(DataFlow::Node node) { node instanceof CommandInjectionSanitizer }
60+
61+
predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
62+
any(CommandInjectionAdditionalTaintStep s).step(n1, n2)
5163
}
5264
}
5365

@@ -58,29 +70,48 @@ module RemoteUserInputToArgumentToExecFlow =
5870
TaintTracking::Global<RemoteUserInputToArgumentToExecFlowConfig>;
5971

6072
/**
61-
* DEPRECATED: Use `execIsTainted` instead.
62-
*
6373
* Implementation of `ExecTainted.ql`. It is extracted to a QLL
6474
* so that it can be excluded from `ExecUnescaped.ql` to avoid
6575
* reporting overlapping results.
6676
*/
67-
deprecated predicate execTainted(
68-
DataFlow::PathNode source, DataFlow::PathNode sink, ArgumentToExec execArg
77+
predicate execIsTainted(
78+
RemoteUserInputToArgumentToExecFlow::PathNode source,
79+
RemoteUserInputToArgumentToExecFlow::PathNode sink, Expr execArg
6980
) {
70-
exists(RemoteUserInputToArgumentToExecFlowConfig conf |
71-
conf.hasFlowPath(source, sink) and sink.getNode() = DataFlow::exprNode(execArg)
72-
)
81+
RemoteUserInputToArgumentToExecFlow::flowPath(source, sink) and
82+
sink.getNode().asExpr() = execArg
7383
}
7484

7585
/**
86+
* DEPRECATED: Use `execIsTainted` instead.
87+
*
7688
* Implementation of `ExecTainted.ql`. It is extracted to a QLL
7789
* so that it can be excluded from `ExecUnescaped.ql` to avoid
7890
* reporting overlapping results.
7991
*/
80-
predicate execIsTainted(
81-
RemoteUserInputToArgumentToExecFlow::PathNode source,
82-
RemoteUserInputToArgumentToExecFlow::PathNode sink, ArgumentToExec execArg
83-
) {
84-
RemoteUserInputToArgumentToExecFlow::flowPath(source, sink) and
85-
sink.getNode() = DataFlow::exprNode(execArg)
92+
deprecated predicate execTainted(DataFlow::PathNode source, DataFlow::PathNode sink, Expr execArg) {
93+
exists(RemoteUserInputToArgumentToExecFlowConfig conf |
94+
conf.hasFlowPath(source, sink) and sink.getNode().asExpr() = execArg
95+
)
96+
}
97+
98+
/**
99+
* DEPRECATED: Use `RemoteUserInputToArgumentToExecFlow` instead.
100+
*
101+
* A taint-tracking configuration for unvalidated user input that is used to run an external process.
102+
*/
103+
deprecated class RemoteUserInputToArgumentToExecFlowConfig extends TaintTracking::Configuration {
104+
RemoteUserInputToArgumentToExecFlowConfig() {
105+
this = "ExecCommon::RemoteUserInputToArgumentToExecFlowConfig"
106+
}
107+
108+
override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
109+
110+
override predicate isSink(DataFlow::Node sink) { sink instanceof CommandInjectionSink }
111+
112+
override predicate isSanitizer(DataFlow::Node node) { node instanceof CommandInjectionSanitizer }
113+
114+
override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
115+
any(CommandInjectionAdditionalTaintStep s).step(n1, n2)
116+
}
86117
}

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,12 @@
1313
*/
1414

1515
import java
16-
import semmle.code.java.dataflow.FlowSources
17-
import semmle.code.java.security.ExternalProcess
1816
import semmle.code.java.security.CommandLineQuery
1917
import RemoteUserInputToArgumentToExecFlow::PathGraph
2018

2119
from
2220
RemoteUserInputToArgumentToExecFlow::PathNode source,
23-
RemoteUserInputToArgumentToExecFlow::PathNode sink, ArgumentToExec execArg
21+
RemoteUserInputToArgumentToExecFlow::PathNode sink, Expr execArg
2422
where execIsTainted(source, sink, execArg)
2523
select execArg, source, sink, "This command line depends on a $@.", source.getNode(),
2624
"user-provided value"

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
*/
1414

1515
import java
16-
import semmle.code.java.security.ExternalProcess
1716
import semmle.code.java.security.CommandLineQuery
1817

1918
/**

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,16 +13,14 @@
1313
*/
1414

1515
import java
16-
import semmle.code.java.dataflow.FlowSources
17-
import semmle.code.java.security.ExternalProcess
1816
import semmle.code.java.security.CommandLineQuery
19-
import JSchOSInjection
2017
import RemoteUserInputToArgumentToExecFlow::PathGraph
18+
import JSchOSInjection
2119

2220
// This is a clone of query `java/command-line-injection` that also includes experimental sinks.
2321
from
2422
RemoteUserInputToArgumentToExecFlow::PathNode source,
25-
RemoteUserInputToArgumentToExecFlow::PathNode sink, ArgumentToExec execArg
23+
RemoteUserInputToArgumentToExecFlow::PathNode sink, Expr execArg
2624
where execIsTainted(source, sink, execArg)
2725
select execArg, source, sink, "This command line depends on a $@.", source.getNode(),
2826
"user-provided value"

0 commit comments

Comments
 (0)