Merge pull request #536 from markshannon/python-more-shell-injection

Python: Some additional sinks for command injection.

change-notes/1.19/analysis-python.md

@@ -67,6 +67,7 @@ Most security alerts are now visible on LGTM by default.
 |----------------------------|------------------------|------------------------------------------------------------------|
 | Assert statement tests the truth value of a literal constant (`py/assert-literal-constant`) | reliability, correctness     | Checks whether an assert statement is testing the truth of a literal constant value. Not shown by default. |
 | Code injection (`py/code-injection`) | Supports path visualization and is now visible on LGTM by default | No change to expected results |
+| Command injection (`py/command-line-injection`) | Additional sinks in the `os`, and `popen` modules | Possibility of new results |
 | Deserializing untrusted input (`py/unsafe-deserialization`) | Supports path visualization | No change to expected results |
 | Encoding error (`py/encoding-error`) | Better alert location | Alert is now shown at the position of the first offending character, rather than at the top of the file. |
 | Missing call to \_\_init\_\_ during object initialization (`py/missing-call-to-init`) | Fewer false positive results | Results where it is likely that the full call chain has not been analyzed are no longer reported. |

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