Skip to content

Commit 7fcc548

Browse files
erik-krogherik-krogh
committed
add py/shell-command-constructed-from-input, but without a source.
It's a very direct port from Ruby, with only minor adjustments to fit the Python APIs
1 parent 187cfd7 commit 7fcc548

File tree

6 files changed

+301
-0
lines changed

6 files changed

+301
-0
lines changed

‎python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* shell command constructed from library input vulnerabilities, as
4+
* well as extension points for adding your own.
5+
*/
6+
7+
private import python
8+
import semmle.python.dataflow.new.DataFlow
9+
import semmle.python.dataflow.new.TaintTracking
10+
import CommandInjectionCustomizations::CommandInjection as CommandInjection
11+
private import semmle.python.Concepts as Concepts
12+
13+
/**
14+
* Module containing sources, sinks, and sanitizers for shell command constructed from library input.
15+
*/
16+
module UnsafeShellCommandConstruction {
17+
/** A source for shell command constructed from library input vulnerabilities. */
18+
abstract class Source extends DataFlow::Node { }
19+
20+
/** An input parameter to a gem seen as a source. */
21+
private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode {
22+
LibraryInputAsSource() {
23+
none() // TODO: Do something here, put it in a shared library.
24+
}
25+
}
26+
27+
/** A sink for shell command constructed from library input vulnerabilities. */
28+
abstract class Sink extends DataFlow::Node {
29+
/** Gets a description of how the string in this sink was constructed. */
30+
abstract string describe();
31+
32+
/** Gets the dataflow node where the string is constructed. */
33+
DataFlow::Node getStringConstruction() { result = this }
34+
35+
/** Gets the dataflow node that executed the string as a shell command. */
36+
abstract DataFlow::Node getCommandExecution();
37+
}
38+
39+
/** Holds if the string constructed at `source` is executed at `shellExec` */
40+
predicate isUsedAsShellCommand(DataFlow::Node source, Concepts::SystemCommandExecution shellExec) {
41+
source = backtrackShellExec(TypeTracker::TypeBackTracker::end(), shellExec)
42+
}
43+
44+
import semmle.python.dataflow.new.TypeTracker as TypeTracker
45+
46+
private DataFlow::LocalSourceNode backtrackShellExec(
47+
TypeTracker::TypeBackTracker t, Concepts::SystemCommandExecution shellExec
48+
) {
49+
t.start() and
50+
result = any(DataFlow::Node n | shellExec.isShellInterpreted(n)).getALocalSource()
51+
or
52+
exists(TypeTracker::TypeBackTracker t2 |
53+
result = backtrackShellExec(t2, shellExec).backtrack(t2, t)
54+
)
55+
}
56+
57+
/**
58+
* A string constructed from a string-literal (e.g. `f'foo {sink}'`),
59+
* where the resulting string ends up being executed as a shell command.
60+
*/
61+
class StringInterpolationAsSink extends Sink {
62+
// TODO: Add test.
63+
Concepts::SystemCommandExecution s;
64+
Fstring fstring;
65+
66+
StringInterpolationAsSink() {
67+
isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr() = fstring), s) and
68+
this.asExpr() = fstring.getASubExpression()
69+
}
70+
71+
override string describe() { result = "string construction" }
72+
73+
override DataFlow::Node getCommandExecution() { result = s }
74+
75+
override DataFlow::Node getStringConstruction() { result.asExpr() = fstring }
76+
}
77+
78+
/**
79+
* A component of a string-concatenation (e.g. `"foo " + sink`),
80+
* where the resulting string ends up being executed as a shell command.
81+
*/
82+
class StringConcatAsSink extends Sink {
83+
// TODO: Add test.
84+
Concepts::SystemCommandExecution s;
85+
BinaryExpr add;
86+
87+
StringConcatAsSink() {
88+
add.getOp() instanceof Add and
89+
isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr() = add), s) and
90+
this.asExpr() = add.getASubExpression()
91+
}
92+
93+
override DataFlow::Node getCommandExecution() { result = s }
94+
95+
override string describe() { result = "string concatenation" }
96+
97+
override DataFlow::Node getStringConstruction() { result.asExpr() = add }
98+
}
99+
100+
/**
101+
* A string constructed using a `.join(" ")` call, where the resulting string ends up being executed as a shell command.
102+
*/
103+
class ArrayJoin extends Sink {
104+
// TODO: Add test.
105+
Concepts::SystemCommandExecution s;
106+
DataFlow::MethodCallNode call;
107+
108+
ArrayJoin() {
109+
call.getMethodName() = "join" and
110+
unique( | | call.getArg(_)).asExpr().(Str).getText() = " " and
111+
isUsedAsShellCommand(call, s) and
112+
(
113+
this = call.getObject() and
114+
not call.getObject().asExpr() instanceof List
115+
or
116+
this.asExpr() = call.getObject().asExpr().(List).getASubExpression()
117+
)
118+
}
119+
120+
override string describe() { result = "array" }
121+
122+
override DataFlow::Node getCommandExecution() { result = s }
123+
124+
override DataFlow::Node getStringConstruction() { result = call }
125+
}
126+
127+
/**
128+
* A string constructed from a format call,
129+
* where the resulting string ends up being executed as a shell command.
130+
* Either a call to `.format(..)` or a string-interpolation with a `%` operator.
131+
*/
132+
class TaintedFormatStringAsSink extends Sink {
133+
// TODO: Test
134+
Concepts::SystemCommandExecution s;
135+
DataFlow::Node formatCall;
136+
137+
TaintedFormatStringAsSink() {
138+
(
139+
formatCall.asExpr().(BinaryExpr).getOp() instanceof Mod and
140+
this.asExpr() = formatCall.asExpr().(BinaryExpr).getASubExpression()
141+
or
142+
formatCall.(DataFlow::MethodCallNode).getMethodName() = "format" and
143+
this =
144+
[
145+
formatCall.(DataFlow::MethodCallNode).getArg(_),
146+
formatCall.(DataFlow::MethodCallNode).getObject()
147+
]
148+
) and
149+
isUsedAsShellCommand(formatCall, s)
150+
}
151+
152+
override string describe() { result = "formatted string" }
153+
154+
override DataFlow::Node getCommandExecution() { result = s }
155+
156+
override DataFlow::Node getStringConstruction() { result = formatCall }
157+
}
158+
}

‎python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionQuery.qll

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about shell command
3+
* constructed from library input vulnerabilities
4+
*
5+
* Note, for performance reasons: only import this file if `Configuration` is needed,
6+
* otherwise `UnsafeShellCommandConstructionCustomizations` should be imported instead.
7+
*/
8+
9+
import python
10+
import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction
11+
import semmle.python.dataflow.new.DataFlow
12+
import semmle.python.dataflow.new.TaintTracking
13+
import CommandInjectionCustomizations::CommandInjection as CommandInjection
14+
import semmle.python.dataflow.new.BarrierGuards
15+
16+
/**
17+
* A taint-tracking configuration for detecting shell command constructed from library input vulnerabilities.
18+
*/
19+
class Configuration extends TaintTracking::Configuration {
20+
Configuration() { this = "UnsafeShellCommandConstruction" }
21+
22+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
23+
24+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
25+
26+
override predicate isSanitizer(DataFlow::Node node) {
27+
node instanceof CommandInjection::Sanitizer or // using all sanitizers from `rb/command-injection`
28+
node instanceof StringConstCompareBarrier
29+
}
30+
31+
// override to require the path doesn't have unmatched return steps
32+
override DataFlow::FlowFeature getAFeature() {
33+
result instanceof DataFlow::FeatureHasSourceCallContext
34+
}
35+
}

‎python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
Dynamically constructing a shell command with inputs from exported
8+
functions may inadvertently change the meaning of the shell command.
9+
10+
Clients using the exported function may use inputs containing
11+
characters that the shell interprets in a special way, for instance
12+
quotes and spaces.
13+
14+
This can result in the shell command misbehaving, or even
15+
allowing a malicious user to execute arbitrary commands on the system.
16+
</p>
17+
18+
19+
</overview>
20+
<recommendation>
21+
22+
<p>
23+
If possible, provide the dynamic arguments to the shell as an array
24+
to APIs such as <code>system(..)</code> to avoid interpretation by the shell.
25+
</p>
26+
27+
<p>
28+
Alternatively, if the shell command must be constructed
29+
dynamically, then add code to ensure that special characters
30+
do not alter the shell command unexpectedly.
31+
</p>
32+
33+
</recommendation>
34+
<example>
35+
36+
<p>
37+
The following example shows a dynamically constructed shell
38+
command that downloads a file from a remote URL.
39+
</p>
40+
41+
<sample src="examples/unsafe-shell-command-construction.py" />
42+
43+
<p>
44+
The shell command will, however, fail to work as intended if the
45+
input contains spaces or other special characters interpreted in a
46+
special way by the shell.
47+
</p>
48+
49+
<p>
50+
Even worse, a client might pass in user-controlled
51+
data, not knowing that the input is interpreted as a shell command.
52+
This could allow a malicious user to provide the input <code>http://example.org; cat /etc/passwd</code>
53+
in order to execute the command <code>cat /etc/passwd</code>.
54+
</p>
55+
56+
<p>
57+
To avoid such potentially catastrophic behaviors, provide the
58+
input from exported functions as an argument that does not
59+
get interpreted by a shell:
60+
</p>
61+
62+
<sample src="examples/unsafe-shell-command-construction_fixed.py" />
63+
64+
</example>
65+
<references>
66+
67+
<li>
68+
OWASP:
69+
<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
70+
</li>
71+
72+
</references>
73+
</qhelp>

‎python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.ql

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
/**
2+
* @name Unsafe shell command constructed from library input
3+
* @description Using externally controlled strings in a command line may allow a malicious
4+
* user to change the meaning of the command.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 6.3
8+
* @precision high
9+
* @id py/shell-command-constructed-from-input
10+
* @tags correctness
11+
* security
12+
* external/cwe/cwe-078
13+
* external/cwe/cwe-088
14+
* external/cwe/cwe-073
15+
*/
16+
17+
import python
18+
import semmle.python.security.dataflow.UnsafeShellCommandConstructionQuery
19+
import DataFlow::PathGraph
20+
21+
from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
22+
where
23+
config.hasFlowPath(source, sink) and
24+
sinkNode = sink.getNode()
25+
select sinkNode.getStringConstruction(), source, sink,
26+
"This " + sinkNode.describe() + " which depends on $@ is later used in a $@.", source.getNode(),
27+
"library input", sinkNode.getCommandExecution(), "shell command"

‎python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
import os
2+
3+
def download (path):
4+
os.system("wget " + path) # NOT OK

‎python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction_fixed.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
import subprocess
2+
3+
def download (path):
4+
subprocess.run(["wget", path]) # OK

0 commit comments

Comments
 (0)