Merge pull request #3563 from RasmusWL/python-fabric-execute

Approved by tausbn

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

@@ -32,6 +32,8 @@ class CommandInjectionConfiguration extends TaintTracking::Configuration {
 
     override predicate isExtension(TaintTracking::Extension extension) {
         extension instanceof FirstElementFlow
+        or
+        extension instanceof FabricExecuteExtension
     }
 }
 

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

@@ -231,3 +231,41 @@ class FabricV1Commands extends CommandSink {
 
     override predicate sinks(TaintKind kind) { kind instanceof ExternalStringKind }
 }
+
+/**
+ * An extension that propagates taint from the arguments of `fabric.api.execute(func, arg0, arg1, ...)`
+ * to the parameters of `func`, since this will call `func(arg0, arg1, ...)`.
+ */
+class FabricExecuteExtension extends DataFlowExtension::DataFlowNode {
+    CallNode call;
+
+    FabricExecuteExtension() {
+        call = Value::named("fabric.api.execute").getACall() and
+        (
+            this = call.getArg(any(int i | i > 0))
+            or
+            this = call.getArgByName(any(string s | not s = "task"))
+        )
+    }
+
+    override ControlFlowNode getASuccessorNode(TaintKind fromkind, TaintKind tokind) {
+        tokind = fromkind and
+        exists(CallableValue func |
+            (
+                call.getArg(0).pointsTo(func)
+                or
+                call.getArgByName("task").pointsTo(func)
+            ) and
+            exists(int i |
+                // execute(func, arg0, arg1) => func(arg0, arg1)
+                this = call.getArg(i) and
+                result = func.getParameter(i - 1)
+            )
+            or
+            exists(string name |
+                this = call.getArgByName(name) and
+                result = func.getParameterByName(name)
+            )
+        )
+    }
+}

python/ql/test/library-tests/security/fabric-v1-execute/Taint.qll

@@ -0,0 +1,24 @@
+import python
+import semmle.python.dataflow.TaintTracking
+import semmle.python.security.strings.Untrusted
+import semmle.python.security.injection.Command
+
+class SimpleSource extends TaintSource {
+    SimpleSource() { this.(NameNode).getId() = "TAINTED_STRING" }
+
+    override predicate isSourceOf(TaintKind kind) { kind instanceof ExternalStringKind }
+
+    override string toString() { result = "taint source" }
+}
+
+class FabricExecuteTestConfiguration extends TaintTracking::Configuration {
+    FabricExecuteTestConfiguration() { this = "FabricExecuteTestConfiguration" }
+
+    override predicate isSource(TaintTracking::Source source) { source instanceof SimpleSource }
+
+    override predicate isSink(TaintTracking::Sink sink) { sink instanceof CommandSink }
+
+    override predicate isExtension(TaintTracking::Extension extension) {
+        extension instanceof FabricExecuteExtension
+    }
+}

python/ql/test/library-tests/security/fabric-v1-execute/TestTaint.expected

@@ -0,0 +1,10 @@
+| test.py:8 | ok   | unsafe | cmd | externally controlled string |
+| test.py:8 | ok   | unsafe | cmd2 | externally controlled string |
+| test.py:9 | ok   | unsafe | safe_arg | <NO TAINT> |
+| test.py:9 | ok   | unsafe | safe_optional | <NO TAINT> |
+| test.py:16 | ok   | unsafe | cmd | externally controlled string |
+| test.py:16 | ok   | unsafe | cmd2 | externally controlled string |
+| test.py:17 | ok   | unsafe | safe_arg | <NO TAINT> |
+| test.py:17 | ok   | unsafe | safe_optional | <NO TAINT> |
+| test.py:23 | ok   | some_http_handler | cmd | externally controlled string |
+| test.py:23 | ok   | some_http_handler | cmd2 | externally controlled string |

python/ql/test/library-tests/security/fabric-v1-execute/TestTaint.ql

@@ -0,0 +1,34 @@
+import python
+import semmle.python.security.TaintTracking
+import semmle.python.web.HttpRequest
+import semmle.python.security.strings.Untrusted
+
+import Taint
+
+from
+    Call call, Expr arg, boolean expected_taint, boolean has_taint, string test_res,
+    string taint_string
+where
+    call.getLocation().getFile().getShortName() = "test.py" and
+    (
+        call.getFunc().(Name).getId() = "ensure_tainted" and
+        expected_taint = true
+        or
+        call.getFunc().(Name).getId() = "ensure_not_tainted" and
+        expected_taint = false
+    ) and
+    arg = call.getAnArg() and
+    (
+        not exists(TaintedNode tainted | tainted.getAstNode() = arg) and
+        taint_string = "<NO TAINT>" and
+        has_taint = false
+        or
+        exists(TaintedNode tainted | tainted.getAstNode() = arg |
+            taint_string = tainted.getTaintKind().toString()
+        ) and
+        has_taint = true
+    ) and
+    if expected_taint = has_taint then test_res = "ok  " else test_res = "fail"
+// if expected_taint = has_taint then test_res = "✓" else test_res = "✕"
+select arg.getLocation().toString(), test_res, call.getScope().(Function).getName(), arg.toString(),
+    taint_string

python/ql/test/library-tests/security/fabric-v1-execute/options

@@ -0,0 +1 @@
+semmle-extractor-options: --max-import-depth=2 -p ../../../query-tests/Security/lib/

python/ql/test/library-tests/security/fabric-v1-execute/test.py

@@ -0,0 +1,28 @@
+"""Test that shows fabric.api.execute propagates taint"""
+
+from fabric.api import run, execute
+
+
+def unsafe(cmd, safe_arg, cmd2=None, safe_optional=5):
+    run('./venv/bin/activate && {}'.format(cmd))
+    ensure_tainted(cmd, cmd2)
+    ensure_not_tainted(safe_arg, safe_optional)
+
+
+class Foo(object):
+
+    def unsafe(self, cmd, safe_arg, cmd2=None, safe_optional=5):
+        run('./venv/bin/activate && {}'.format(cmd))
+        ensure_tainted(cmd, cmd2)
+        ensure_not_tainted(safe_arg, safe_optional)
+
+
+def some_http_handler():
+    cmd = TAINTED_STRING
+    cmd2 = TAINTED_STRING
+    ensure_tainted(cmd, cmd2)
+
+    execute(unsafe, cmd=cmd, safe_arg='safe_arg', cmd2=cmd2)
+
+    foo = Foo()
+    execute(foo.unsafe, cmd, 'safe_arg', cmd2)

python/ql/test/query-tests/Security/lib/fabric/api.py

@@ -23,3 +23,7 @@ def sudo(command, shell=True, pty=True, combine_stderr=None, user=None,
     quiet=False, warn_only=False, stdout=None, stderr=None, group=None,
     timeout=None, shell_escape=None, capture_buffer_size=None):
     pass
+
+# https://github.com/fabric/fabric/blob/1.14/fabric/tasks.py#L281
+def execute(task, *args, **kwargs):
+    pass