Python: Add extra sinks for command-injection query.

Author: markshannon

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

@@ -15,8 +15,9 @@ private ModuleObject subprocessModule() {
     result.getName() = "subprocess"
 }
 
-private ModuleObject osModule() {
-    result.getName() = "os"
+private ModuleObject osOrPopenModule() {
+    result.getName() = "os" or
+    result.getName() = "popen2"
 }
 
 private Object makeOsCall() {
@@ -25,7 +26,8 @@ private Object makeOsCall() {
         name = "Popen" or
         name = "call" or 
         name = "check_call" or
-        name = "check_output"
+        name = "check_output" or
+        name = "run"
     )
 }
 
@@ -75,9 +77,17 @@ class ShellCommand extends TaintSink {
             istrue.booleanValue() = true
         )
         or
+        exists(CallNode call, string name |
+            call.getAnArg() = this and
+            call.getFunction().refersTo(osOrPopenModule().getAttribute(name)) |
+            name = "system" or
+            name = "popen" or
+            name.matches("popen_")
+        )
+        or
         exists(CallNode call |
-            call.getFunction().refersTo(osModule().getAttribute("system")) and
-            call.getAnArg() = this
+            call.getAnArg() = this and
+            call.getFunction().refersTo(any(ModuleObject commands | commands.getName() = "commands"))
         )
     }
 

python/ql/test/query-tests/Security/CWE-078/CommandInjection.expected

@@ -10,9 +10,15 @@ edges
 | command_injection.py:24:11:24:37 | externally controlled string | command_injection.py:25:23:25:25 | externally controlled string |
 | command_injection.py:25:23:25:25 | externally controlled string | command_injection.py:25:22:25:36 | first item in sequence of externally controlled string |
 | command_injection.py:25:23:25:25 | externally controlled string | command_injection.py:25:22:25:36 | sequence of externally controlled string |
+| command_injection.py:30:13:30:24 | dict of externally controlled string | command_injection.py:30:13:30:41 | externally controlled string |
+| command_injection.py:30:13:30:41 | externally controlled string | command_injection.py:32:22:32:26 | externally controlled string |
+| command_injection.py:32:14:32:26 | externally controlled string | ../lib/os/__init__.py:4:11:4:13 | externally controlled string |
+| command_injection.py:32:22:32:26 | externally controlled string | command_injection.py:32:14:32:26 | externally controlled string |
 parents
 | ../lib/os/__init__.py:1:12:1:14 | externally controlled string | command_injection.py:12:15:12:27 | externally controlled string |
+| ../lib/os/__init__.py:4:11:4:13 | externally controlled string | command_injection.py:32:14:32:26 | externally controlled string |
 #select
 | command_injection.py:12:15:12:27 | shell command | command_injection.py:10:13:10:24 | dict of externally controlled string | command_injection.py:12:15:12:27 | externally controlled string | This command depends on $@. | command_injection.py:10:13:10:24 | flask.request.args | a user-provided value |
 | command_injection.py:19:22:19:34 | shell command | command_injection.py:17:13:17:24 | dict of externally controlled string | command_injection.py:19:22:19:34 | sequence of externally controlled string | This command depends on $@. | command_injection.py:17:13:17:24 | flask.request.args | a user-provided value |
 | command_injection.py:25:22:25:36 | OS command first argument | command_injection.py:24:11:24:22 | dict of externally controlled string | command_injection.py:25:22:25:36 | first item in sequence of externally controlled string | This command depends on $@. | command_injection.py:24:11:24:22 | flask.request.args | a user-provided value |
+| command_injection.py:32:14:32:26 | shell command | command_injection.py:30:13:30:24 | dict of externally controlled string | command_injection.py:32:14:32:26 | externally controlled string | This command depends on $@. | command_injection.py:30:13:30:24 | flask.request.args | a user-provided value |

python/ql/test/query-tests/Security/CWE-078/command_injection.py

@@ -23,3 +23,10 @@ def command_injection2():
 def first_arg_injection():
     cmd = request.args.get('cmd', '')
     subprocess.Popen([cmd, "param1"])
+
+
+@app.route("/other_cases")
+def others():
+    files = request.args.get('files', '')
+    # Don't let files be `; rm -rf /`
+    os.popen("ls " + files)

python/ql/test/query-tests/Security/lib/os/__init__.py

@@ -1,2 +1,5 @@
 def system(cmd, *args, **kwargs):
-    return None
+    return None
+
+def popen(cmd, *args, **kwargs):
+    return None