use more API-nodes to model subprocess.run (and friends)

Author: erik-krogh

python/ql/lib/semmle/python/frameworks/Stdlib.qll

@@ -1185,7 +1185,7 @@ private module StdlibPrivate {
    * See https://docs.python.org/3.8/library/subprocess.html#subprocess.Popen
    * ref: https://docs.python.org/3/library/subprocess.html#legacy-shell-invocation-functions
    */
-  private class SubprocessPopenCall extends SystemCommandExecution::Range, DataFlow::CallCfgNode {
+  private class SubprocessPopenCall extends SystemCommandExecution::Range, API::CallNode {
     SubprocessPopenCall() {
       exists(string name |
         name in [
@@ -1195,44 +1195,33 @@ private module StdlibPrivate {
       )
     }
 
-    /** Gets the ControlFlowNode for the `args` argument, if any. */
-    private DataFlow::Node get_args_arg() { result in [this.getArg(0), this.getArgByName("args")] }
+    /** Gets the API-node for the `args` argument, if any. */
+    private API::Node get_args_arg() { result = this.getParameter(0, "args") }
 
-    /** Gets the ControlFlowNode for the `shell` argument, if any. */
-    private DataFlow::Node get_shell_arg() {
-      result in [this.getArg(8), this.getArgByName("shell")]
-    }
+    /** Gets the API-node for the `shell` argument, if any. */
+    private API::Node get_shell_arg() { result = this.getParameter(8, "shell") }
 
     private boolean get_shell_arg_value() {
-      // TODO: API-node this thing - with added tests for unsafe-shell-command-construction
       not exists(this.get_shell_arg()) and
       result = false
       or
-      exists(DataFlow::Node shell_arg | shell_arg = this.get_shell_arg() |
-        result = shell_arg.asCfgNode().getNode().(ImmutableLiteral).booleanValue()
-        or
-        // TODO: Track the "shell" argument to determine possible values
-        not shell_arg.asCfgNode().getNode() instanceof ImmutableLiteral and
-        (
-          result = true
-          or
-          result = false
-        )
-      )
+      result =
+        this.get_shell_arg().getAValueReachingSink().asExpr().(ImmutableLiteral).booleanValue()
+      or
+      not this.get_shell_arg().getAValueReachingSink().asExpr() instanceof ImmutableLiteral and
+      result = [true, false]
     }
 
-    /** Gets the ControlFlowNode for the `executable` argument, if any. */
-    private DataFlow::Node get_executable_arg() {
-      result in [this.getArg(2), this.getArgByName("executable")]
-    }
+    /** Gets the API-node for the `executable` argument, if any. */
+    private API::Node get_executable_arg() { result = this.getParameter(2, "executable") }
 
     override DataFlow::Node getCommand() {
       // TODO: Track arguments ("args" and "shell")
       // TODO: Handle using `args=["sh", "-c", <user-input>]`
-      result = this.get_executable_arg()
+      result = this.get_executable_arg().asSink()
       or
       exists(DataFlow::Node arg_args, boolean shell |
-        arg_args = this.get_args_arg() and
+        arg_args = this.get_args_arg().asSink() and
         shell = this.get_shell_arg_value()
       |
         // When "executable" argument is set, and "shell" argument is `False`, the
@@ -1260,7 +1249,7 @@ private module StdlibPrivate {
     }
 
     override predicate isShellInterpreted(DataFlow::Node arg) {
-      arg = [this.get_executable_arg(), this.get_args_arg()] and
+      arg = [this.get_executable_arg(), this.get_args_arg()].asSink() and
       this.get_shell_arg_value() = true
     }
   }

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

@@ -8,6 +8,9 @@ edges
 | src/unsafe_shell_test.py:14:34:14:39 | ControlFlowNode for List | src/unsafe_shell_test.py:14:25:14:40 | ControlFlowNode for Attribute() |
 | src/unsafe_shell_test.py:26:20:26:23 | ControlFlowNode for name | src/unsafe_shell_test.py:29:30:29:33 | ControlFlowNode for name |
 | src/unsafe_shell_test.py:36:22:36:25 | ControlFlowNode for name | src/unsafe_shell_test.py:39:30:39:33 | ControlFlowNode for name |
+| src/unsafe_shell_test.py:36:22:36:25 | ControlFlowNode for name | src/unsafe_shell_test.py:44:20:44:23 | ControlFlowNode for name |
+| src/unsafe_shell_test.py:41:24:41:24 | ControlFlowNode for x | src/unsafe_shell_test.py:42:34:42:34 | ControlFlowNode for x |
+| src/unsafe_shell_test.py:44:20:44:23 | ControlFlowNode for name | src/unsafe_shell_test.py:41:24:41:24 | ControlFlowNode for x |
 nodes
 | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
 | src/unsafe_shell_test.py:5:25:5:28 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
@@ -21,6 +24,9 @@ nodes
 | src/unsafe_shell_test.py:29:30:29:33 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
 | src/unsafe_shell_test.py:36:22:36:25 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
 | src/unsafe_shell_test.py:39:30:39:33 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
+| src/unsafe_shell_test.py:41:24:41:24 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| src/unsafe_shell_test.py:42:34:42:34 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| src/unsafe_shell_test.py:44:20:44:23 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
 subpaths
 #select
 | src/unsafe_shell_test.py:5:15:5:28 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:5:25:5:28 | ControlFlowNode for name | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:5:5:5:29 | ControlFlowNode for Attribute() | shell command |
@@ -31,3 +37,4 @@ subpaths
 | src/unsafe_shell_test.py:20:15:20:30 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:20:27:20:30 | ControlFlowNode for name | This formatted string which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:20:5:20:31 | ControlFlowNode for Attribute() | shell command |
 | src/unsafe_shell_test.py:29:20:29:33 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:26:20:26:23 | ControlFlowNode for name | src/unsafe_shell_test.py:29:30:29:33 | ControlFlowNode for name | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:26:20:26:23 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:29:5:29:46 | ControlFlowNode for Attribute() | shell command |
 | src/unsafe_shell_test.py:39:20:39:33 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:36:22:36:25 | ControlFlowNode for name | src/unsafe_shell_test.py:39:30:39:33 | ControlFlowNode for name | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:36:22:36:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:39:5:39:46 | ControlFlowNode for Attribute() | shell command |
+| src/unsafe_shell_test.py:42:24:42:34 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:36:22:36:25 | ControlFlowNode for name | src/unsafe_shell_test.py:42:34:42:34 | ControlFlowNode for x | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:36:22:36:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:42:9:42:47 | ControlFlowNode for Attribute() | shell command |

python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py

@@ -38,7 +38,7 @@ def subprocess_flag (name):
 
     subprocess.run("ping " + name, shell=True) # $result=BAD
 
-    def indirect(flag): 
-        subprocess.run("ping " + name, shell=flag) # $ MISSING: result=BAD
+    def indirect(flag, x): 
+        subprocess.run("ping " + x, shell=flag) # $result=BAD
 
-    indirect(True)
+    indirect(True, name)