Skip to content

Commit ac7c74d

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#3111 from RasmusWL/python-fabric-command-injection
Approved by BekaValentine
2 parents ae076da + 49fa7c8 commit ac7c74d

File tree

18 files changed

+336
-35
lines changed

18 files changed

+336
-35
lines changed

‎change-notes/1.24/analysis-python.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ Support for Django version 2.x and 3.x
1515

1616
| **Query** | **Expected impact** | **Change** |
1717
|----------------------------|------------------------|------------------------------------------------------------------|
18+
| Uncontrolled command line (`py/command-line-injection`) | More results | We now model the `fabric` and `invoke` pacakges for command execution. |
1819

1920
### Web framework support
2021

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,7 @@ class CommandInjectionConfiguration extends TaintTracking::Configuration {
2929
}
3030

3131
override predicate isSink(TaintTracking::Sink sink) {
32-
sink instanceof OsCommandFirstArgument or
33-
sink instanceof ShellCommand
32+
sink instanceof CommandSink
3433
}
3534

3635
override predicate isExtension(TaintTracking::Extension extension) {

‎python/ql/src/semmle/python/security/injection/Command.qll

Lines changed: 143 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,27 @@
1-
/** Provides class and predicates to track external data that
1+
/**
2+
* Provides class and predicates to track external data that
23
* may represent malicious OS commands.
34
*
45
* This module is intended to be imported into a taint-tracking query
56
* to extend `TaintKind` and `TaintSink`.
6-
*
77
*/
8-
import python
98

9+
import python
1010
import semmle.python.security.TaintTracking
1111
import semmle.python.security.strings.Untrusted
1212

13+
/** Abstract taint sink that is potentially vulnerable to malicious shell commands. */
14+
abstract class CommandSink extends TaintSink { }
1315

1416
private ModuleObject osOrPopenModule() {
1517
result.getName() = "os" or
1618
result.getName() = "popen2"
1719
}
1820

1921
private Object makeOsCall() {
20-
exists(string name |
21-
result = ModuleObject::named("subprocess").attr(name) |
22+
exists(string name | result = ModuleObject::named("subprocess").attr(name) |
2223
name = "Popen" or
23-
name = "call" or
24+
name = "call" or
2425
name = "check_call" or
2526
name = "check_output" or
2627
name = "run"
@@ -29,40 +30,27 @@ private Object makeOsCall() {
2930

3031
/**Special case for first element in sequence. */
3132
class FirstElementKind extends TaintKind {
33+
FirstElementKind() { this = "sequence[" + any(ExternalStringKind key) + "][0]" }
3234

33-
FirstElementKind() {
34-
this = "sequence[" + any(ExternalStringKind key) + "][0]"
35-
}
36-
37-
override string repr() {
38-
result = "first item in sequence of " + this.getItem().repr()
39-
}
35+
override string repr() { result = "first item in sequence of " + this.getItem().repr() }
4036

4137
/** Gets the taint kind for item in this sequence. */
42-
ExternalStringKind getItem() {
43-
this = "sequence[" + result + "][0]"
44-
}
45-
38+
ExternalStringKind getItem() { this = "sequence[" + result + "][0]" }
4639
}
4740

4841
class FirstElementFlow extends DataFlowExtension::DataFlowNode {
42+
FirstElementFlow() { this = any(SequenceNode s).getElement(0) }
4943

50-
FirstElementFlow() {
51-
this = any(SequenceNode s).getElement(0)
52-
}
53-
54-
override
55-
ControlFlowNode getASuccessorNode(TaintKind fromkind, TaintKind tokind) {
44+
override ControlFlowNode getASuccessorNode(TaintKind fromkind, TaintKind tokind) {
5645
result.(SequenceNode).getElement(0) = this and tokind.(FirstElementKind).getItem() = fromkind
5746
}
58-
5947
}
6048

61-
/** A taint sink that is potentially vulnerable to malicious shell commands.
49+
/**
50+
* A taint sink that is potentially vulnerable to malicious shell commands.
6251
* The `vuln` in `subprocess.call(shell=vuln)` and similar calls.
6352
*/
64-
class ShellCommand extends TaintSink {
65-
53+
class ShellCommand extends CommandSink {
6654
override string toString() { result = "shell command" }
6755

6856
ShellCommand() {
@@ -75,7 +63,8 @@ class ShellCommand extends TaintSink {
7563
or
7664
exists(CallNode call, string name |
7765
call.getAnArg() = this and
78-
call.getFunction().refersTo(osOrPopenModule().attr(name)) |
66+
call.getFunction().refersTo(osOrPopenModule().attr(name))
67+
|
7968
name = "system" or
8069
name = "popen" or
8170
name.matches("popen_")
@@ -94,19 +83,18 @@ class ShellCommand extends TaintSink {
9483
/* List (or tuple) containing a tainted string command */
9584
kind instanceof ExternalStringSequenceKind
9685
}
97-
9886
}
9987

100-
/** A taint sink that is potentially vulnerable to malicious shell commands.
88+
/**
89+
* A taint sink that is potentially vulnerable to malicious shell commands.
10190
* The `vuln` in `subprocess.call(vuln, ...)` and similar calls.
10291
*/
103-
class OsCommandFirstArgument extends TaintSink {
104-
92+
class OsCommandFirstArgument extends CommandSink {
10593
override string toString() { result = "OS command first argument" }
10694

10795
OsCommandFirstArgument() {
10896
not this instanceof ShellCommand and
109-
exists(CallNode call|
97+
exists(CallNode call |
11098
call.getFunction().refersTo(makeOsCall()) and
11199
call.getArg(0) = this
112100
)
@@ -119,5 +107,127 @@ class OsCommandFirstArgument extends TaintSink {
119107
/* List (or tuple) whose first element is tainted */
120108
kind instanceof FirstElementKind
121109
}
110+
}
111+
112+
// -------------------------------------------------------------------------- //
113+
// Modeling of the 'invoke' package and 'fabric' package (v 2.x)
114+
//
115+
// Since fabric build so closely upon invoke, we model them together to avoid
116+
// duplication
117+
// -------------------------------------------------------------------------- //
118+
/**
119+
* A taint sink that is potentially vulnerable to malicious shell commands.
120+
* The `vuln` in `invoke.run(vuln, ...)` and similar calls.
121+
*/
122+
class InvokeRun extends CommandSink {
123+
InvokeRun() {
124+
this = Value::named("invoke.run").(FunctionValue).getArgumentForCall(_, 0)
125+
or
126+
this = Value::named("invoke.sudo").(FunctionValue).getArgumentForCall(_, 0)
127+
}
128+
129+
override string toString() { result = "InvokeRun" }
130+
131+
override predicate sinks(TaintKind kind) { kind instanceof ExternalStringKind }
132+
}
133+
134+
/**
135+
* Internal TaintKind to track the invoke.Context instance passed to functions
136+
* marked with @invoke.task
137+
*/
138+
private class InvokeContextArg extends TaintKind {
139+
InvokeContextArg() { this = "InvokeContextArg" }
140+
}
141+
142+
/** Internal TaintSource to track the context passed to functions marked with @invoke.task */
143+
private class InvokeContextArgSource extends TaintSource {
144+
InvokeContextArgSource() {
145+
exists(Function f, Expr decorator |
146+
count(f.getADecorator()) = 1 and
147+
(
148+
decorator = f.getADecorator() and not decorator instanceof Call
149+
or
150+
decorator = f.getADecorator().(Call).getFunc()
151+
) and
152+
(
153+
decorator.pointsTo(Value::named("invoke.task"))
154+
or
155+
decorator.pointsTo(Value::named("fabric.task"))
156+
)
157+
|
158+
this.(ControlFlowNode).getNode() = f.getArg(0)
159+
)
160+
}
161+
162+
override predicate isSourceOf(TaintKind kind) { kind instanceof InvokeContextArg }
163+
}
164+
165+
/**
166+
* A taint sink that is potentially vulnerable to malicious shell commands.
167+
* The `vuln` in `invoke.Context().run(vuln, ...)` and similar calls.
168+
*/
169+
class InvokeContextRun extends CommandSink {
170+
InvokeContextRun() {
171+
exists(CallNode call |
172+
any(InvokeContextArg k).taints(call.getFunction().(AttrNode).getObject("run"))
173+
or
174+
call = Value::named("invoke.Context").(ClassValue).lookup("run").getACall()
175+
or
176+
// fabric.connection.Connection is a subtype of invoke.context.Context
177+
// since fabric.Connection.run has a decorator, it doesn't work with FunctionValue :|
178+
// and `Value::named("fabric.Connection").(ClassValue).lookup("run").getACall()` returned no results,
179+
// so here is the hacky solution that works :\
180+
call.getFunction().(AttrNode).getObject("run").pointsTo().getClass() =
181+
Value::named("fabric.Connection")
182+
|
183+
this = call.getArg(0)
184+
or
185+
this = call.getArgByName("command")
186+
)
187+
}
188+
189+
override string toString() { result = "InvokeContextRun" }
190+
191+
override predicate sinks(TaintKind kind) { kind instanceof ExternalStringKind }
192+
}
193+
194+
/**
195+
* A taint sink that is potentially vulnerable to malicious shell commands.
196+
* The `vuln` in `fabric.Group().run(vuln, ...)` and similar calls.
197+
*/
198+
class FabricGroupRun extends CommandSink {
199+
FabricGroupRun() {
200+
exists(ClassValue cls |
201+
cls.getASuperType() = Value::named("fabric.Group") and
202+
this = cls.lookup("run").(FunctionValue).getArgumentForCall(_, 1)
203+
)
204+
}
205+
206+
override string toString() { result = "FabricGroupRun" }
207+
208+
override predicate sinks(TaintKind kind) { kind instanceof ExternalStringKind }
209+
}
210+
211+
// -------------------------------------------------------------------------- //
212+
// Modeling of the 'invoke' package and 'fabric' package (v 1.x)
213+
// -------------------------------------------------------------------------- //
214+
class FabricV1Commands extends CommandSink {
215+
FabricV1Commands() {
216+
// since `run` and `sudo` are decorated, we can't use FunctionValue's :(
217+
exists(CallNode call |
218+
call = Value::named("fabric.api.local").getACall()
219+
or
220+
call = Value::named("fabric.api.run").getACall()
221+
or
222+
call = Value::named("fabric.api.sudo").getACall()
223+
|
224+
this = call.getArg(0)
225+
or
226+
this = call.getArgByName("command")
227+
)
228+
}
229+
230+
override string toString() { result = "FabricV1Commands" }
122231

232+
override predicate sinks(TaintKind kind) { kind instanceof ExternalStringKind }
123233
}

‎python/ql/test/library-tests/security/command-execution/CommandSinks.expected

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
| fabric_v1_test.py:8:7:8:28 | FabricV1Commands | externally controlled string |
2+
| fabric_v1_test.py:9:5:9:27 | FabricV1Commands | externally controlled string |
3+
| fabric_v1_test.py:10:6:10:38 | FabricV1Commands | externally controlled string |
4+
| fabric_v2_test.py:10:16:10:25 | InvokeContextRun | externally controlled string |
5+
| fabric_v2_test.py:12:15:12:36 | InvokeContextRun | externally controlled string |
6+
| fabric_v2_test.py:16:45:16:54 | FabricGroupRun | externally controlled string |
7+
| fabric_v2_test.py:21:10:21:13 | FabricGroupRun | externally controlled string |
8+
| fabric_v2_test.py:31:14:31:41 | InvokeContextRun | externally controlled string |
9+
| fabric_v2_test.py:33:15:33:64 | InvokeContextRun | externally controlled string |
10+
| invoke_test.py:8:12:8:21 | InvokeRun | externally controlled string |
11+
| invoke_test.py:9:20:9:40 | InvokeRun | externally controlled string |
12+
| invoke_test.py:12:17:12:24 | InvokeRun | externally controlled string |
13+
| invoke_test.py:13:25:13:32 | InvokeRun | externally controlled string |
14+
| invoke_test.py:17:11:17:40 | InvokeContextRun | externally controlled string |
15+
| invoke_test.py:21:11:21:32 | InvokeContextRun | externally controlled string |
16+
| invoke_test.py:27:11:27:25 | InvokeContextRun | externally controlled string |
17+
| invoke_test.py:32:11:32:25 | InvokeContextRun | externally controlled string |

‎python/ql/test/library-tests/security/command-execution/CommandSinks.ql

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
import python
2+
import semmle.python.security.injection.Command
3+
import semmle.python.security.strings.Untrusted
4+
5+
from CommandSink sink, TaintKind kind
6+
where sink.sinks(kind)
7+
select sink, kind

‎python/ql/test/library-tests/security/command-execution/fabric-LICENSE

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
Copyright (c) 2020 Jeff Forcier.
2+
All rights reserved.
3+
4+
Redistribution and use in source and binary forms, with or without
5+
modification, are permitted provided that the following conditions are met:
6+
7+
* Redistributions of source code must retain the above copyright notice,
8+
this list of conditions and the following disclaimer.
9+
* Redistributions in binary form must reproduce the above copyright notice,
10+
this list of conditions and the following disclaimer in the documentation
11+
and/or other materials provided with the distribution.
12+
13+
THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
14+
ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
15+
WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
16+
DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
17+
FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
18+
DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
19+
SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
20+
CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
21+
OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
22+
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

‎python/ql/test/library-tests/security/command-execution/fabric_v1_test.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
"""tests for the 'fabric' package (v1.x)
2+
3+
See http://docs.fabfile.org/en/1.14/tutorial.html
4+
"""
5+
6+
from fabric.api import run, local, sudo
7+
8+
local('echo local execution')
9+
run('echo remote execution')
10+
sudo('echo remote execution with sudo')

‎python/ql/test/library-tests/security/command-execution/fabric_v2_test.py

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
"""tests for the 'fabric' package (v2.x)
2+
3+
Most of these examples are taken from the fabric documentation: http://docs.fabfile.org/en/2.5/getting-started.html
4+
See fabric-LICENSE for its' license.
5+
"""
6+
7+
from fabric import Connection
8+
9+
c = Connection('web1')
10+
result = c.run('uname -s')
11+
12+
c.run(command='echo run with kwargs')
13+
14+
15+
from fabric import SerialGroup as Group
16+
results = Group('web1', 'web2', 'mac1').run('uname -s')
17+
18+
19+
from fabric import SerialGroup as Group
20+
pool = Group('web1', 'web2', 'web3')
21+
pool.run('ls')
22+
23+
24+
25+
# using the 'fab' command-line tool
26+
27+
from fabric import task
28+
29+
@task
30+
def upload_and_unpack(c):
31+
if c.run('test -f /opt/mydata/myfile', warn=True).failed:
32+
c.put('myfiles.tgz', '/opt/mydata')
33+
c.run('tar -C /opt/mydata -xzvf /opt/mydata/myfiles.tgz')

‎python/ql/test/library-tests/security/command-execution/invoke_test.py

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
"""tests for the 'invoke' package
2+
3+
see https://www.pyinvoke.org/
4+
"""
5+
6+
import invoke
7+
8+
invoke.run('echo run')
9+
invoke.run(command='echo run with kwarg')
10+
11+
def with_sudo():
12+
invoke.sudo('whoami')
13+
invoke.sudo(command='whoami')
14+
15+
def manual_context():
16+
c = invoke.Context()
17+
c.run('echo run from manual context')
18+
manual_context()
19+
20+
def foo_helper(c):
21+
c.run('echo from foo_helper')
22+
23+
# for use with the 'invoke' command-line tool
24+
@invoke.task
25+
def foo(c):
26+
# 'c' is a invoke.context.Context
27+
c.run('echo task foo')
28+
foo_helper(c)
29+
30+
@invoke.task()
31+
def bar(c):
32+
c.run('echo task bar')

‎python/ql/test/library-tests/security/command-execution/options

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
semmle-extractor-options: --max-import-depth=2 -p ../../../query-tests/Security/lib/

0 commit comments

Comments
 (0)