From 187cfd7be7285beeaa31f0e1607acebd0781c789 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Tue, 31 Jan 2023 14:23:57 +0100
Subject: [PATCH 01/17] add `isShellInterpreted` to the
 `SystemCommandExecution` concept

---
 python/ql/lib/semmle/python/Concepts.qll      |  6 ++++
 .../lib/semmle/python/frameworks/Fabric.qll   | 14 ++++++--
 .../lib/semmle/python/frameworks/Invoke.qll   |  2 ++
 .../lib/semmle/python/frameworks/Stdlib.qll   | 32 +++++++++++++++++--
 4 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/python/ql/lib/semmle/python/Concepts.qll b/python/ql/lib/semmle/python/Concepts.qll
index b4d2a64cb45..8ea64a1373c 100644
--- a/python/ql/lib/semmle/python/Concepts.qll
+++ b/python/ql/lib/semmle/python/Concepts.qll
@@ -19,6 +19,9 @@ private import semmle.python.security.internal.EncryptionKeySizes
  * extend `SystemCommandExecution::Range` instead.
  */
 class SystemCommandExecution extends DataFlow::Node instanceof SystemCommandExecution::Range {
+  /** Holds if a shell interprets `arg`. */
+  predicate isShellInterpreted(DataFlow::Node arg) { super.isShellInterpreted(arg) }
+
   /** Gets the argument that specifies the command to be executed. */
   DataFlow::Node getCommand() { result = super.getCommand() }
 }
@@ -35,6 +38,9 @@ module SystemCommandExecution {
   abstract class Range extends DataFlow::Node {
     /** Gets the argument that specifies the command to be executed. */
     abstract DataFlow::Node getCommand();
+
+    /** Holds if a shell interprets `arg`. */
+    predicate isShellInterpreted(DataFlow::Node arg) { none() }
   }
 }
 
diff --git a/python/ql/lib/semmle/python/frameworks/Fabric.qll b/python/ql/lib/semmle/python/frameworks/Fabric.qll
index 5fd9d2afe18..31511dbeaf4 100644
--- a/python/ql/lib/semmle/python/frameworks/Fabric.qll
+++ b/python/ql/lib/semmle/python/frameworks/Fabric.qll
@@ -43,13 +43,19 @@ private module FabricV1 {
        * - https://docs.fabfile.org/en/1.14/api/core/operations.html#fabric.operations.run
        * - https://docs.fabfile.org/en/1.14/api/core/operations.html#fabric.operations.sudo
        */
-      private class FabricApiLocalRunSudoCall extends SystemCommandExecution::Range,
-        DataFlow::CallCfgNode {
+      private class FabricApiLocalRunSudoCall extends SystemCommandExecution::Range, API::CallNode {
         FabricApiLocalRunSudoCall() { this = api().getMember(["local", "run", "sudo"]).getACall() }
 
         override DataFlow::Node getCommand() {
           result = [this.getArg(0), this.getArgByName("command")]
         }
+
+        override predicate isShellInterpreted(DataFlow::Node arg) {
+          arg = this.getCommand() and
+          // defaults to running in a shell
+          not this.getParameter(1, "shell").asSink().asExpr().(ImmutableLiteral).booleanValue() =
+            false // TODO: Test this in unsafe-shell-command-construction - and add tracking as a separate step.
+        }
       }
     }
   }
@@ -161,6 +167,8 @@ private module FabricV2 {
       override DataFlow::Node getCommand() {
         result = [this.getArg(0), this.getArgByName("command")]
       }
+
+      override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getCommand() }
     }
 
     // -------------------------------------------------------------------------
@@ -243,6 +251,8 @@ private module FabricV2 {
         override DataFlow::Node getCommand() {
           result = [this.getArg(0), this.getArgByName("command")]
         }
+
+        override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getCommand() }
       }
 
       /**
diff --git a/python/ql/lib/semmle/python/frameworks/Invoke.qll b/python/ql/lib/semmle/python/frameworks/Invoke.qll
index b1ceb078907..30637610ac9 100644
--- a/python/ql/lib/semmle/python/frameworks/Invoke.qll
+++ b/python/ql/lib/semmle/python/frameworks/Invoke.qll
@@ -81,5 +81,7 @@ private module Invoke {
     override DataFlow::Node getCommand() {
       result in [this.getArg(0), this.getArgByName("command")]
     }
+
+    override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getCommand() }
   }
 }
diff --git a/python/ql/lib/semmle/python/frameworks/Stdlib.qll b/python/ql/lib/semmle/python/frameworks/Stdlib.qll
index 3fce979c147..d3512aca668 100644
--- a/python/ql/lib/semmle/python/frameworks/Stdlib.qll
+++ b/python/ql/lib/semmle/python/frameworks/Stdlib.qll
@@ -1060,7 +1060,11 @@ private module StdlibPrivate {
   private class OsSystemCall extends SystemCommandExecution::Range, DataFlow::CallCfgNode {
     OsSystemCall() { this = os().getMember("system").getACall() }
 
-    override DataFlow::Node getCommand() { result = this.getArg(0) }
+    override DataFlow::Node getCommand() {
+      result in [this.getArg(0), this.getArgByName("command")]
+    }
+
+    override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getCommand() }
   }
 
   /**
@@ -1071,7 +1075,7 @@ private module StdlibPrivate {
    * Although deprecated since version 2.6, they still work in 2.7.
    * See https://docs.python.org/2.7/library/os.html#os.popen2
    */
-  private class OsPopenCall extends SystemCommandExecution::Range, DataFlow::CallCfgNode {
+  private class OsPopenCall extends SystemCommandExecution::Range, API::CallNode {
     string name;
 
     OsPopenCall() {
@@ -1085,6 +1089,8 @@ private module StdlibPrivate {
       not name = "popen" and
       result = this.getArgByName("cmd")
     }
+
+    override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getCommand() }
   }
 
   /**
@@ -1103,6 +1109,10 @@ private module StdlibPrivate {
     override DataFlow::Node getCommand() { result = this.getArg(0) }
 
     override DataFlow::Node getAPathArgument() { result = this.getCommand() }
+
+    override predicate isShellInterpreted(DataFlow::Node arg) {
+      none() // this is a safe API.
+    }
   }
 
   /**
@@ -1129,6 +1139,10 @@ private module StdlibPrivate {
     }
 
     override DataFlow::Node getAPathArgument() { result = this.getCommand() }
+
+    override predicate isShellInterpreted(DataFlow::Node arg) {
+      none() // this is a safe API.
+    }
   }
 
   /**
@@ -1142,6 +1156,10 @@ private module StdlibPrivate {
     override DataFlow::Node getCommand() { result in [this.getArg(0), this.getArgByName("path")] }
 
     override DataFlow::Node getAPathArgument() { result = this.getCommand() }
+
+    override predicate isShellInterpreted(DataFlow::Node arg) {
+      none() // this is a safe API.
+    }
   }
 
   /** An additional taint step for calls to `os.path.join` */
@@ -1186,6 +1204,7 @@ private module StdlibPrivate {
     }
 
     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
@@ -1239,6 +1258,11 @@ private module StdlibPrivate {
         )
       )
     }
+
+    override predicate isShellInterpreted(DataFlow::Node arg) {
+      arg = this.get_executable_arg() and
+      this.get_shell_arg_value() = true
+    }
   }
 
   // ---------------------------------------------------------------------------
@@ -1385,6 +1409,8 @@ private module StdlibPrivate {
     }
 
     override DataFlow::Node getCommand() { result in [this.getArg(0), this.getArgByName("cmd")] }
+
+    override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getCommand() }
   }
 
   // ---------------------------------------------------------------------------
@@ -1401,6 +1427,8 @@ private module StdlibPrivate {
     PlatformPopenCall() { this = platform().getMember("popen").getACall() }
 
     override DataFlow::Node getCommand() { result in [this.getArg(0), this.getArgByName("cmd")] }
+
+    override predicate isShellInterpreted(DataFlow::Node arg) { arg = this.getCommand() }
   }
 
   // ---------------------------------------------------------------------------

From 7fcc548665ba307f465767d31e168e54c5244010 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Tue, 31 Jan 2023 14:51:38 +0100
Subject: [PATCH 02/17] add `py/shell-command-constructed-from-input`, but
 without a source. It's a very direct port from Ruby, with only minor
 adjustments to fit the Python APIs

---
 ...ShellCommandConstructionCustomizations.qll | 158 ++++++++++++++++++
 .../UnsafeShellCommandConstructionQuery.qll   |  35 ++++
 .../UnsafeShellCommandConstruction.qhelp      |  73 ++++++++
 .../CWE-078/UnsafeShellCommandConstruction.ql |  27 +++
 .../unsafe-shell-command-construction.py      |   4 +
 ...unsafe-shell-command-construction_fixed.py |   4 +
 6 files changed, 301 insertions(+)
 create mode 100644 python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
 create mode 100644 python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionQuery.qll
 create mode 100644 python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp
 create mode 100644 python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.ql
 create mode 100644 python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction.py
 create mode 100644 python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction_fixed.py

diff --git a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
new file mode 100644
index 00000000000..6cf1a765c1f
--- /dev/null
+++ b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
@@ -0,0 +1,158 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * shell command constructed from library input vulnerabilities, as
+ * well as extension points for adding your own.
+ */
+
+private import python
+import semmle.python.dataflow.new.DataFlow
+import semmle.python.dataflow.new.TaintTracking
+import CommandInjectionCustomizations::CommandInjection as CommandInjection
+private import semmle.python.Concepts as Concepts
+
+/**
+ * Module containing sources, sinks, and sanitizers for shell command constructed from library input.
+ */
+module UnsafeShellCommandConstruction {
+  /** A source for shell command constructed from library input vulnerabilities. */
+  abstract class Source extends DataFlow::Node { }
+
+  /** An input parameter to a gem seen as a source. */
+  private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode {
+    LibraryInputAsSource() {
+      none() // TODO: Do something here, put it in a shared library.
+    }
+  }
+
+  /** A sink for shell command constructed from library input vulnerabilities. */
+  abstract class Sink extends DataFlow::Node {
+    /** Gets a description of how the string in this sink was constructed. */
+    abstract string describe();
+
+    /** Gets the dataflow node where the string is constructed. */
+    DataFlow::Node getStringConstruction() { result = this }
+
+    /** Gets the dataflow node that executed the string as a shell command. */
+    abstract DataFlow::Node getCommandExecution();
+  }
+
+  /** Holds if the string constructed at `source` is executed at `shellExec` */
+  predicate isUsedAsShellCommand(DataFlow::Node source, Concepts::SystemCommandExecution shellExec) {
+    source = backtrackShellExec(TypeTracker::TypeBackTracker::end(), shellExec)
+  }
+
+  import semmle.python.dataflow.new.TypeTracker as TypeTracker
+
+  private DataFlow::LocalSourceNode backtrackShellExec(
+    TypeTracker::TypeBackTracker t, Concepts::SystemCommandExecution shellExec
+  ) {
+    t.start() and
+    result = any(DataFlow::Node n | shellExec.isShellInterpreted(n)).getALocalSource()
+    or
+    exists(TypeTracker::TypeBackTracker t2 |
+      result = backtrackShellExec(t2, shellExec).backtrack(t2, t)
+    )
+  }
+
+  /**
+   * A string constructed from a string-literal (e.g. `f'foo {sink}'`),
+   * where the resulting string ends up being executed as a shell command.
+   */
+  class StringInterpolationAsSink extends Sink {
+    // TODO: Add test.
+    Concepts::SystemCommandExecution s;
+    Fstring fstring;
+
+    StringInterpolationAsSink() {
+      isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr() = fstring), s) and
+      this.asExpr() = fstring.getASubExpression()
+    }
+
+    override string describe() { result = "string construction" }
+
+    override DataFlow::Node getCommandExecution() { result = s }
+
+    override DataFlow::Node getStringConstruction() { result.asExpr() = fstring }
+  }
+
+  /**
+   * A component of a string-concatenation (e.g. `"foo " + sink`),
+   * where the resulting string ends up being executed as a shell command.
+   */
+  class StringConcatAsSink extends Sink {
+    // TODO: Add test.
+    Concepts::SystemCommandExecution s;
+    BinaryExpr add;
+
+    StringConcatAsSink() {
+      add.getOp() instanceof Add and
+      isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr() = add), s) and
+      this.asExpr() = add.getASubExpression()
+    }
+
+    override DataFlow::Node getCommandExecution() { result = s }
+
+    override string describe() { result = "string concatenation" }
+
+    override DataFlow::Node getStringConstruction() { result.asExpr() = add }
+  }
+
+  /**
+   * A string constructed using a `.join(" ")` call, where the resulting string ends up being executed as a shell command.
+   */
+  class ArrayJoin extends Sink {
+    // TODO: Add test.
+    Concepts::SystemCommandExecution s;
+    DataFlow::MethodCallNode call;
+
+    ArrayJoin() {
+      call.getMethodName() = "join" and
+      unique( | | call.getArg(_)).asExpr().(Str).getText() = " " and
+      isUsedAsShellCommand(call, s) and
+      (
+        this = call.getObject() and
+        not call.getObject().asExpr() instanceof List
+        or
+        this.asExpr() = call.getObject().asExpr().(List).getASubExpression()
+      )
+    }
+
+    override string describe() { result = "array" }
+
+    override DataFlow::Node getCommandExecution() { result = s }
+
+    override DataFlow::Node getStringConstruction() { result = call }
+  }
+
+  /**
+   * A string constructed from a format call,
+   * where the resulting string ends up being executed as a shell command.
+   * Either a call to `.format(..)` or a string-interpolation with a `%` operator.
+   */
+  class TaintedFormatStringAsSink extends Sink {
+    // TODO: Test
+    Concepts::SystemCommandExecution s;
+    DataFlow::Node formatCall;
+
+    TaintedFormatStringAsSink() {
+      (
+        formatCall.asExpr().(BinaryExpr).getOp() instanceof Mod and
+        this.asExpr() = formatCall.asExpr().(BinaryExpr).getASubExpression()
+        or
+        formatCall.(DataFlow::MethodCallNode).getMethodName() = "format" and
+        this =
+          [
+            formatCall.(DataFlow::MethodCallNode).getArg(_),
+            formatCall.(DataFlow::MethodCallNode).getObject()
+          ]
+      ) and
+      isUsedAsShellCommand(formatCall, s)
+    }
+
+    override string describe() { result = "formatted string" }
+
+    override DataFlow::Node getCommandExecution() { result = s }
+
+    override DataFlow::Node getStringConstruction() { result = formatCall }
+  }
+}
diff --git a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionQuery.qll b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionQuery.qll
new file mode 100644
index 00000000000..6fc567289e7
--- /dev/null
+++ b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionQuery.qll
@@ -0,0 +1,35 @@
+/**
+ * Provides a taint tracking configuration for reasoning about shell command
+ * constructed from library input vulnerabilities
+ *
+ * Note, for performance reasons: only import this file if `Configuration` is needed,
+ * otherwise `UnsafeShellCommandConstructionCustomizations` should be imported instead.
+ */
+
+import python
+import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction
+import semmle.python.dataflow.new.DataFlow
+import semmle.python.dataflow.new.TaintTracking
+import CommandInjectionCustomizations::CommandInjection as CommandInjection
+import semmle.python.dataflow.new.BarrierGuards
+
+/**
+ * A taint-tracking configuration for detecting shell command constructed from library input vulnerabilities.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "UnsafeShellCommandConstruction" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    node instanceof CommandInjection::Sanitizer or // using all sanitizers from `rb/command-injection`
+    node instanceof StringConstCompareBarrier
+  }
+
+  // override to require the path doesn't have unmatched return steps
+  override DataFlow::FlowFeature getAFeature() {
+    result instanceof DataFlow::FeatureHasSourceCallContext
+  }
+}
diff --git a/python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp b/python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp
new file mode 100644
index 00000000000..3176d5c035d
--- /dev/null
+++ b/python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp
@@ -0,0 +1,73 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+<overview>
+	<p>
+		Dynamically constructing a shell command with inputs from exported 
+		functions may inadvertently change the meaning of the shell command.
+		
+		Clients using the exported function may use inputs containing
+		characters that the shell interprets in a special way, for instance
+		quotes and spaces.
+
+		This can result in the shell command misbehaving, or even
+		allowing a malicious user to execute arbitrary commands on the system.
+	</p>
+
+
+</overview>
+<recommendation>
+
+	<p>
+		If possible, provide the dynamic arguments to the shell as an array 
+		to APIs such as <code>system(..)</code> to avoid interpretation by the shell.
+	</p>
+
+	<p>
+		Alternatively, if the shell command must be constructed
+		dynamically, then add code to ensure that special characters 
+		do not alter the shell command unexpectedly.
+	</p>
+
+</recommendation>
+<example>
+
+	<p>
+		The following example shows a dynamically constructed shell
+		command that downloads a file from a remote URL.
+	</p>
+
+	<sample src="examples/unsafe-shell-command-construction.py" />
+
+	<p>
+		The shell command will, however, fail to work as intended if the
+		input contains spaces or other special characters interpreted in a 
+		special way by the shell. 
+	</p>
+
+	<p>
+		Even worse, a client might pass in user-controlled
+		data, not knowing that the input is interpreted as a shell command. 
+		This could allow a malicious user to provide the input <code>http://example.org; cat /etc/passwd</code>
+		in order to execute the command <code>cat /etc/passwd</code>.
+	</p>
+
+	<p>
+		To avoid such potentially catastrophic behaviors, provide the
+		input from exported functions as an argument that does not
+		get interpreted by a shell:
+	</p>
+
+	<sample src="examples/unsafe-shell-command-construction_fixed.py" />
+
+</example>
+<references>
+
+	<li>
+		OWASP:
+		<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
+	</li>
+
+</references>
+</qhelp>
diff --git a/python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.ql b/python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.ql
new file mode 100644
index 00000000000..d22ee170f3a
--- /dev/null
+++ b/python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.ql
@@ -0,0 +1,27 @@
+/**
+ * @name Unsafe shell command constructed from library input
+ * @description Using externally controlled strings in a command line may allow a malicious
+ *              user to change the meaning of the command.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 6.3
+ * @precision high
+ * @id py/shell-command-constructed-from-input
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-088
+ *       external/cwe/cwe-073
+ */
+
+import python
+import semmle.python.security.dataflow.UnsafeShellCommandConstructionQuery
+import DataFlow::PathGraph
+
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
+where
+  config.hasFlowPath(source, sink) and
+  sinkNode = sink.getNode()
+select sinkNode.getStringConstruction(), source, sink,
+  "This " + sinkNode.describe() + " which depends on $@ is later used in a $@.", source.getNode(),
+  "library input", sinkNode.getCommandExecution(), "shell command"
diff --git a/python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction.py b/python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction.py
new file mode 100644
index 00000000000..644af24938d
--- /dev/null
+++ b/python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction.py
@@ -0,0 +1,4 @@
+import os
+
+def download (path): 
+    os.system("wget " + path) # NOT OK
diff --git a/python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction_fixed.py b/python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction_fixed.py
new file mode 100644
index 00000000000..d62b3478bf1
--- /dev/null
+++ b/python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction_fixed.py
@@ -0,0 +1,4 @@
+import subprocess
+
+def download (path): 
+    subprocess.run(["wget", path]) # OK

From 47a06d282433fc16c4fb88a42e2893cacca770ca Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Wed, 1 Feb 2023 10:32:02 +0100
Subject: [PATCH 03/17] add library inputs as a source, and get minimal test to
 work

---
 python/ql/lib/semmle/python/Frameworks.qll    |  1 +
 .../semmle/python/frameworks/Setuptools.qll   | 71 +++++++++++++++++++
 ...ShellCommandConstructionCustomizations.qll |  9 +--
 .../DataflowQueryTest.expected                |  2 +
 .../DataflowQueryTest.ql                      |  3 +
 .../UnsafeShellCommandConstruction.expected   |  8 +++
 .../UnsafeShellCommandConstruction.qlref      |  1 +
 .../options                                   |  1 +
 .../setup.py                                  |  0
 .../src/__init__.py                           |  0
 .../src/unsafe_shell_test.py                  |  5 ++
 11 files changed, 97 insertions(+), 4 deletions(-)
 create mode 100644 python/ql/lib/semmle/python/frameworks/Setuptools.qll
 create mode 100644 python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/DataflowQueryTest.expected
 create mode 100644 python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/DataflowQueryTest.ql
 create mode 100644 python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
 create mode 100644 python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref
 create mode 100644 python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/options
 create mode 100644 python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/setup.py
 create mode 100644 python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/__init__.py
 create mode 100644 python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py

diff --git a/python/ql/lib/semmle/python/Frameworks.qll b/python/ql/lib/semmle/python/Frameworks.qll
index eab33a03ed7..d9592fcafb3 100644
--- a/python/ql/lib/semmle/python/Frameworks.qll
+++ b/python/ql/lib/semmle/python/Frameworks.qll
@@ -51,6 +51,7 @@ private import semmle.python.frameworks.Simplejson
 private import semmle.python.frameworks.SqlAlchemy
 private import semmle.python.frameworks.Starlette
 private import semmle.python.frameworks.Stdlib
+private import semmle.python.frameworks.Setuptools
 private import semmle.python.frameworks.Toml
 private import semmle.python.frameworks.Tornado
 private import semmle.python.frameworks.Twisted
diff --git a/python/ql/lib/semmle/python/frameworks/Setuptools.qll b/python/ql/lib/semmle/python/frameworks/Setuptools.qll
new file mode 100644
index 00000000000..32509cfccec
--- /dev/null
+++ b/python/ql/lib/semmle/python/frameworks/Setuptools.qll
@@ -0,0 +1,71 @@
+/**
+ * Provides classes modeling package setup as defined by `setuptools`.
+ */
+
+private import python
+private import semmle.python.dataflow.new.DataFlow
+
+/** Provides models for the use of `setuptools` in setup scripts, and the APIs exported by the library defined using `setuptools`. */
+module Setuptools {
+  /**
+   * Gets a file that sets up a package using `setuptools` (or the deprecated `distutils`).
+   */
+  private File setupFile() {
+    // all of these might not be extracted, but the support is ready for when they are
+    result.getBaseName() = ["setup.py", "setup.cfg", "pyproject.toml"]
+  }
+
+  /**
+   * Gets a file or folder that is exported by a library.
+   */
+  private Container getALibraryExportedContainer() {
+    result = setupFile().getParent()
+    or
+    // child of a library exported container
+    result = getALibraryExportedContainer().getAChildContainer() and
+    (
+      // either any file
+      not result instanceof Folder
+      or
+      // or a folder with an __init__.py file
+      exists(result.(Folder).getFile("__init__.py"))
+    ) and
+    // that is not a test folder
+    not result.(Folder).getBaseName() = ["test", "tests", "testing"]
+  }
+
+  /**
+   * Gets an AST node that is exported by a library.
+   */
+  private AstNode getAnExportedLibraryFeature() {
+    result.(Module).getFile() = getALibraryExportedContainer()
+    or
+    result = getAnExportedLibraryFeature().(Module).getAStmt()
+    or
+    result = getAnExportedLibraryFeature().(ClassDef).getDefinedClass().getAMethod()
+    or
+    result = getAnExportedLibraryFeature().(ClassDef).getDefinedClass().getInitMethod()
+    or
+    result = getAnExportedLibraryFeature().(FunctionDef).getDefinedFunction()
+  }
+
+  /**
+   * Gets a public function (or __init__) that is exported by a library.
+   */
+  private Function getAnExportedFunction() {
+    result = getAnExportedLibraryFeature() and
+    (
+      result.isPublic()
+      or
+      result.isInitMethod()
+    )
+  }
+
+  /**
+   * Gets a parameter to a public function that is exported by a library.
+   */
+  DataFlow::ParameterNode getALibraryInput() {
+    result.getParameter() = getAnExportedFunction().getAnArg() and
+    not result.getParameter().isSelf()
+  }
+}
diff --git a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
index 6cf1a765c1f..ac06d12edcd 100644
--- a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
+++ b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
@@ -17,15 +17,17 @@ module UnsafeShellCommandConstruction {
   /** A source for shell command constructed from library input vulnerabilities. */
   abstract class Source extends DataFlow::Node { }
 
+  private import semmle.python.frameworks.Setuptools
+
   /** An input parameter to a gem seen as a source. */
   private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode {
-    LibraryInputAsSource() {
-      none() // TODO: Do something here, put it in a shared library.
-    }
+    LibraryInputAsSource() { this = Setuptools::getALibraryInput() }
   }
 
   /** A sink for shell command constructed from library input vulnerabilities. */
   abstract class Sink extends DataFlow::Node {
+    Sink() { not this.asExpr() instanceof StrConst } // filter out string constants, makes testing easier
+
     /** Gets a description of how the string in this sink was constructed. */
     abstract string describe();
 
@@ -80,7 +82,6 @@ module UnsafeShellCommandConstruction {
    * where the resulting string ends up being executed as a shell command.
    */
   class StringConcatAsSink extends Sink {
-    // TODO: Add test.
     Concepts::SystemCommandExecution s;
     BinaryExpr add;
 
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/DataflowQueryTest.expected b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/DataflowQueryTest.expected
new file mode 100644
index 00000000000..3875da4e143
--- /dev/null
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/DataflowQueryTest.expected
@@ -0,0 +1,2 @@
+missingAnnotationOnSink
+failures
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/DataflowQueryTest.ql b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/DataflowQueryTest.ql
new file mode 100644
index 00000000000..f9a934e1794
--- /dev/null
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/DataflowQueryTest.ql
@@ -0,0 +1,3 @@
+import python
+import experimental.dataflow.TestUtil.DataflowQueryTest
+import semmle.python.security.dataflow.UnsafeShellCommandConstructionQuery
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
new file mode 100644
index 00000000000..fbb4dc0af1b
--- /dev/null
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
@@ -0,0 +1,8 @@
+edges
+| src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:5:25:5:28 | ControlFlowNode for name |
+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 |
+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 |
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref
new file mode 100644
index 00000000000..fdc01b9ecbf
--- /dev/null
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref
@@ -0,0 +1 @@
+Security/CWE-078/UnsafeShellCommandConstruction.ql
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/options b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/options
new file mode 100644
index 00000000000..9ec98787409
--- /dev/null
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/options
@@ -0,0 +1 @@
+semmle-extractor-options: --lang=3 --max-import-depth=0 -r src
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/setup.py b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/setup.py
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/__init__.py b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/__init__.py
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
new file mode 100644
index 00000000000..eae8ca34a05
--- /dev/null
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
@@ -0,0 +1,5 @@
+import os
+import subprocess
+
+def unsafe_shell_one(name):
+    os.system("ping " + name) # $result=BAD

From 5bddfc0d79a2c98b7071d50a41f6ed475e0fddc3 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Wed, 1 Feb 2023 23:05:44 +0100
Subject: [PATCH 04/17] add test for f-strings as sink

---
 .../dataflow/UnsafeShellCommandConstructionCustomizations.qll  | 1 -
 .../UnsafeShellCommandConstruction.expected                    | 3 +++
 .../src/unsafe_shell_test.py                                   | 3 +++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
index ac06d12edcd..fbea50dc536 100644
--- a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
+++ b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
@@ -61,7 +61,6 @@ module UnsafeShellCommandConstruction {
    * where the resulting string ends up being executed as a shell command.
    */
   class StringInterpolationAsSink extends Sink {
-    // TODO: Add test.
     Concepts::SystemCommandExecution s;
     Fstring fstring;
 
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
index fbb4dc0af1b..6b89266b7ea 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
@@ -1,8 +1,11 @@
 edges
 | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:5:25:5:28 | ControlFlowNode for name |
+| src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name |
 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 |
+| src/unsafe_shell_test.py:8:23:8:26 | 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 |
+| src/unsafe_shell_test.py:8:15:8:28 | ControlFlowNode for Fstring | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name | This string construction 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:8:5:8:29 | ControlFlowNode for Attribute() | shell command |
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
index eae8ca34a05..f541b0da99c 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
@@ -3,3 +3,6 @@ import subprocess
 
 def unsafe_shell_one(name):
     os.system("ping " + name) # $result=BAD
+
+    # f-strings
+    os.system(f"ping {name}") # $result=BAD

From 33c506d7feccfd81b2c4938e4c92db7122c3dc39 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Wed, 1 Feb 2023 23:08:57 +0100
Subject: [PATCH 05/17] add minimal test for Array join as a sink, and learn
 that the order is flipped compared to JS. Thanks Copilot!

---
 .../UnsafeShellCommandConstructionCustomizations.qll       | 7 +++----
 .../UnsafeShellCommandConstruction.expected                | 3 +++
 .../src/unsafe_shell_test.py                               | 3 +++
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
index fbea50dc536..883c2db408b 100644
--- a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
+++ b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
@@ -101,7 +101,6 @@ module UnsafeShellCommandConstruction {
    * A string constructed using a `.join(" ")` call, where the resulting string ends up being executed as a shell command.
    */
   class ArrayJoin extends Sink {
-    // TODO: Add test.
     Concepts::SystemCommandExecution s;
     DataFlow::MethodCallNode call;
 
@@ -110,10 +109,10 @@ module UnsafeShellCommandConstruction {
       unique( | | call.getArg(_)).asExpr().(Str).getText() = " " and
       isUsedAsShellCommand(call, s) and
       (
-        this = call.getObject() and
-        not call.getObject().asExpr() instanceof List
+        this = call.getArg(0) and
+        not call.getArg(0).asExpr() instanceof List
         or
-        this.asExpr() = call.getObject().asExpr().(List).getASubExpression()
+        this.asExpr() = call.getArg(0).asExpr().(List).getASubExpression()
       )
     }
 
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
index 6b89266b7ea..c81a17adf28 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
@@ -1,11 +1,14 @@
 edges
 | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:5:25:5:28 | ControlFlowNode for name |
 | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name |
+| src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:11:25:11:38 | ControlFlowNode for Attribute() |
 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 |
 | src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
+| src/unsafe_shell_test.py:11:25:11:38 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
 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 |
 | src/unsafe_shell_test.py:8:15:8:28 | ControlFlowNode for Fstring | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name | This string construction 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:8:5:8:29 | ControlFlowNode for Attribute() | shell command |
+| src/unsafe_shell_test.py:11:15:11:38 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:11:25:11:38 | ControlFlowNode for Attribute() | 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:11:5:11:39 | ControlFlowNode for Attribute() | shell command |
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
index f541b0da99c..73ba9a55939 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
@@ -6,3 +6,6 @@ def unsafe_shell_one(name):
 
     # f-strings
     os.system(f"ping {name}") # $result=BAD
+
+    # array.join
+    os.system("ping " + " ".join(name)) # $result=BAD

From 6bbc4f4a482dac5ea34bfcfc9413b1cb3b9c5f93 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Wed, 1 Feb 2023 23:09:53 +0100
Subject: [PATCH 06/17] add more tests

---
 .../UnsafeShellCommandConstructionCustomizations.qll  |  1 -
 .../UnsafeShellCommandConstruction.expected           | 11 +++++++++++
 .../src/unsafe_shell_test.py                          | 11 +++++++++++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
index 883c2db408b..2012ec0c552 100644
--- a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
+++ b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
@@ -129,7 +129,6 @@ module UnsafeShellCommandConstruction {
    * Either a call to `.format(..)` or a string-interpolation with a `%` operator.
    */
   class TaintedFormatStringAsSink extends Sink {
-    // TODO: Test
     Concepts::SystemCommandExecution s;
     DataFlow::Node formatCall;
 
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
index c81a17adf28..d2efe2825ba 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
@@ -2,13 +2,24 @@ edges
 | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:5:25:5:28 | ControlFlowNode for name |
 | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name |
 | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:11:25:11:38 | ControlFlowNode for Attribute() |
+| src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:14:34:14:39 | ControlFlowNode for List |
+| src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:17:32:17:35 | ControlFlowNode for name |
+| src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:20:27:20:30 | ControlFlowNode for name |
+| src/unsafe_shell_test.py:14:34:14:39 | ControlFlowNode for List | src/unsafe_shell_test.py:14:25:14:40 | ControlFlowNode for Attribute() |
 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 |
 | src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
 | src/unsafe_shell_test.py:11:25:11:38 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
+| src/unsafe_shell_test.py:14:25:14:40 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
+| src/unsafe_shell_test.py:14:34:14:39 | ControlFlowNode for List | semmle.label | ControlFlowNode for List |
+| src/unsafe_shell_test.py:17:32:17:35 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
+| src/unsafe_shell_test.py:20:27:20:30 | 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 |
 | src/unsafe_shell_test.py:8:15:8:28 | ControlFlowNode for Fstring | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name | This string construction 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:8:5:8:29 | ControlFlowNode for Attribute() | shell command |
 | src/unsafe_shell_test.py:11:15:11:38 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:11:25:11:38 | ControlFlowNode for Attribute() | 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:11:5:11:39 | ControlFlowNode for Attribute() | shell command |
+| src/unsafe_shell_test.py:14:15:14:40 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:14:25:14:40 | ControlFlowNode for Attribute() | 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:14:5:14:41 | ControlFlowNode for Attribute() | shell command |
+| src/unsafe_shell_test.py:17:15:17:36 | ControlFlowNode for Attribute() | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:17:32:17:35 | 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:17:5:17:37 | ControlFlowNode for Attribute() | shell command |
+| 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 |
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
index 73ba9a55939..b45e81c6513 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
@@ -9,3 +9,14 @@ def unsafe_shell_one(name):
 
     # array.join
     os.system("ping " + " ".join(name)) # $result=BAD
+
+    # array.join, with a list
+    os.system("ping " + " ".join([name])) # $result=BAD
+
+    # format, using .format
+    os.system("ping {}".format(name)) # $result=BAD
+
+    # format, using %
+    os.system("ping %s" % name) # $result=BAD
+
+    os.system(name) # OK - seems intentional.

From 0a2c7d062cf18ed5c08e5acffb4013119826831f Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Thu, 2 Feb 2023 10:34:32 +0100
Subject: [PATCH 07/17] add Fabric test, and add tracking of the shell flag in
 Fabric

---
 python/ql/lib/semmle/python/frameworks/Fabric.qll    |  7 +++++--
 .../UnsafeShellCommandConstruction.expected          |  4 ++++
 .../src/unsafe_shell_test.py                         | 12 ++++++++++++
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/python/ql/lib/semmle/python/frameworks/Fabric.qll b/python/ql/lib/semmle/python/frameworks/Fabric.qll
index 31511dbeaf4..21302af2e0a 100644
--- a/python/ql/lib/semmle/python/frameworks/Fabric.qll
+++ b/python/ql/lib/semmle/python/frameworks/Fabric.qll
@@ -53,8 +53,11 @@ private module FabricV1 {
         override predicate isShellInterpreted(DataFlow::Node arg) {
           arg = this.getCommand() and
           // defaults to running in a shell
-          not this.getParameter(1, "shell").asSink().asExpr().(ImmutableLiteral).booleanValue() =
-            false // TODO: Test this in unsafe-shell-command-construction - and add tracking as a separate step.
+          not this.getParameter(1, "shell")
+              .getAValueReachingSink()
+              .asExpr()
+              .(ImmutableLiteral)
+              .booleanValue() = false
         }
       }
     }
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
index d2efe2825ba..208f72925dd 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
@@ -6,6 +6,7 @@ edges
 | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:17:32:17:35 | ControlFlowNode for name |
 | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:20:27:20:30 | ControlFlowNode for name |
 | 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 |
 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 |
@@ -15,6 +16,8 @@ nodes
 | src/unsafe_shell_test.py:14:34:14:39 | ControlFlowNode for List | semmle.label | ControlFlowNode for List |
 | src/unsafe_shell_test.py:17:32:17:35 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
 | src/unsafe_shell_test.py:20:27:20:30 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
+| src/unsafe_shell_test.py:26:20:26:23 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
+| src/unsafe_shell_test.py:29:30:29:33 | 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 |
@@ -23,3 +26,4 @@ subpaths
 | src/unsafe_shell_test.py:14:15:14:40 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:14:25:14:40 | ControlFlowNode for Attribute() | 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:14:5:14:41 | ControlFlowNode for Attribute() | shell command |
 | src/unsafe_shell_test.py:17:15:17:36 | ControlFlowNode for Attribute() | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:17:32:17:35 | 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:17:5:17:37 | ControlFlowNode for Attribute() | shell command |
 | 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 |
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
index b45e81c6513..8954758bce3 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
@@ -20,3 +20,15 @@ def unsafe_shell_one(name):
     os.system("ping %s" % name) # $result=BAD
 
     os.system(name) # OK - seems intentional.
+
+import fabric
+
+def facbric_stuff (name): 
+    fabric.api.run("ping " + name, shell=False) # OK
+
+    fabric.api.run("ping " + name, shell=True) # $result=BAD
+
+    def indirect(flag): 
+        fabric.api.run("ping " + name, shell=flag) # OK
+
+    indirect(False)

From bce83bfc4ecc960a0a46a20a7050cd90d270b96a Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Thu, 2 Feb 2023 11:05:29 +0100
Subject: [PATCH 08/17] add failing test for indirectly setting the shell=true
 flag for subprocess.run

---
 python/ql/lib/semmle/python/frameworks/Stdlib.qll      |  2 +-
 .../UnsafeShellCommandConstruction.expected            |  4 ++++
 .../src/unsafe_shell_test.py                           | 10 ++++++++++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/python/ql/lib/semmle/python/frameworks/Stdlib.qll b/python/ql/lib/semmle/python/frameworks/Stdlib.qll
index d3512aca668..8e18e0611b8 100644
--- a/python/ql/lib/semmle/python/frameworks/Stdlib.qll
+++ b/python/ql/lib/semmle/python/frameworks/Stdlib.qll
@@ -1260,7 +1260,7 @@ private module StdlibPrivate {
     }
 
     override predicate isShellInterpreted(DataFlow::Node arg) {
-      arg = this.get_executable_arg() and
+      arg = [this.get_executable_arg(), this.get_args_arg()] and
       this.get_shell_arg_value() = true
     }
   }
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
index 208f72925dd..951c08e5d62 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
@@ -7,6 +7,7 @@ edges
 | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:20:27:20:30 | ControlFlowNode for name |
 | 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 |
 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 |
@@ -18,6 +19,8 @@ nodes
 | src/unsafe_shell_test.py:20:27:20:30 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
 | src/unsafe_shell_test.py:26:20:26:23 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
 | 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 |
 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 |
@@ -27,3 +30,4 @@ subpaths
 | src/unsafe_shell_test.py:17:15:17:36 | ControlFlowNode for Attribute() | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:17:32:17:35 | 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:17:5:17:37 | ControlFlowNode for Attribute() | shell command |
 | 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 |
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
index 8954758bce3..65d87e5495e 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
@@ -32,3 +32,13 @@ def facbric_stuff (name):
         fabric.api.run("ping " + name, shell=flag) # OK
 
     indirect(False)
+
+def subprocess_flag (name): 
+    subprocess.run("ping " + name, shell=False) # OK - and nonsensical
+
+    subprocess.run("ping " + name, shell=True) # $result=BAD
+
+    def indirect(flag): 
+        subprocess.run("ping " + name, shell=flag) # $ MISSING: result=BAD
+
+    indirect(True)
\ No newline at end of file

From d228cf0e7b3d8d8c8223d71f7d16b7b1e5651569 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Thu, 2 Feb 2023 11:23:27 +0100
Subject: [PATCH 09/17] use more API-nodes to model subprocess.run (and
 friends)

---
 .../lib/semmle/python/frameworks/Stdlib.qll   | 41 +++++++------------
 .../UnsafeShellCommandConstruction.expected   |  7 ++++
 .../src/unsafe_shell_test.py                  |  6 +--
 3 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/python/ql/lib/semmle/python/frameworks/Stdlib.qll b/python/ql/lib/semmle/python/frameworks/Stdlib.qll
index 8e18e0611b8..c21987d9847 100644
--- a/python/ql/lib/semmle/python/frameworks/Stdlib.qll
+++ b/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
     }
   }
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
index 951c08e5d62..3378661f451 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
+++ b/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 |
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
index 65d87e5495e..e3a3f062b85 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
+++ b/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)
\ No newline at end of file
+    indirect(True, name)
\ No newline at end of file

From e9ebba3350561ebb52f6e5790bb2d36fe049baba Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Thu, 2 Feb 2023 14:43:14 +0100
Subject: [PATCH 10/17] assume shell=False for subprocess calls, fixes FPs in
 e.g. youtube-dl

---
 python/ql/lib/semmle/python/frameworks/Stdlib.qll             | 2 +-
 .../src/unsafe_shell_test.py                                  | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/python/ql/lib/semmle/python/frameworks/Stdlib.qll b/python/ql/lib/semmle/python/frameworks/Stdlib.qll
index c21987d9847..b08995ad99e 100644
--- a/python/ql/lib/semmle/python/frameworks/Stdlib.qll
+++ b/python/ql/lib/semmle/python/frameworks/Stdlib.qll
@@ -1209,7 +1209,7 @@ private module StdlibPrivate {
         this.get_shell_arg().getAValueReachingSink().asExpr().(ImmutableLiteral).booleanValue()
       or
       not this.get_shell_arg().getAValueReachingSink().asExpr() instanceof ImmutableLiteral and
-      result = [true, false]
+      result = false // defaults to `False`
     }
 
     /** Gets the API-node for the `executable` argument, if any. */
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
index e3a3f062b85..c1afd531fbe 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
@@ -41,4 +41,6 @@ def subprocess_flag (name):
     def indirect(flag, x): 
         subprocess.run("ping " + x, shell=flag) # $result=BAD
 
-    indirect(True, name)
\ No newline at end of file
+    indirect(True, name)
+
+    subprocess.Popen("ping " + name, shell=unknownValue) # OK - shell assumed to be False
\ No newline at end of file

From ef44cb86c2d7b82ab192e70a5bcc0475b8385ec8 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Thu, 2 Feb 2023 16:59:27 +0100
Subject: [PATCH 11/17] remove FPs related to parameters that are meant to be
 commands

---
 .../UnsafeShellCommandConstructionCustomizations.qll         | 5 ++++-
 .../src/unsafe_shell_test.py                                 | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
index 2012ec0c552..fcffb9f5145 100644
--- a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
+++ b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
@@ -21,7 +21,10 @@ module UnsafeShellCommandConstruction {
 
   /** An input parameter to a gem seen as a source. */
   private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode {
-    LibraryInputAsSource() { this = Setuptools::getALibraryInput() }
+    LibraryInputAsSource() {
+      this = Setuptools::getALibraryInput() and
+      not this.getParameter().getName().matches(["cmd%", "command%", "%_command", "%_cmd"])
+    }
   }
 
   /** A sink for shell command constructed from library input vulnerabilities. */
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
index c1afd531fbe..1b4bc708c45 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py
@@ -43,4 +43,7 @@ def subprocess_flag (name):
 
     indirect(True, name)
 
-    subprocess.Popen("ping " + name, shell=unknownValue) # OK - shell assumed to be False
\ No newline at end of file
+    subprocess.Popen("ping " + name, shell=unknownValue) # OK - shell assumed to be False
+
+def intentional(command): 
+    os.system("fish -ic " + command) # $result=OK - intentional
\ No newline at end of file

From 848b24cfe4527bc02e295d4af9ef4a3d7c67dbc4 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Thu, 2 Feb 2023 19:36:46 +0100
Subject: [PATCH 12/17] adjust concept tests after changing subprocess model

---
 .../library-tests/frameworks/stdlib/SystemCommandExecution.py   | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/python/ql/test/library-tests/frameworks/stdlib/SystemCommandExecution.py b/python/ql/test/library-tests/frameworks/stdlib/SystemCommandExecution.py
index ae165fe14bf..7a9d91d52b9 100644
--- a/python/ql/test/library-tests/frameworks/stdlib/SystemCommandExecution.py
+++ b/python/ql/test/library-tests/frameworks/stdlib/SystemCommandExecution.py
@@ -140,7 +140,7 @@ subprocess.Popen(args)  # $getCommand=args
 args = "<progname>"
 use_shell = False
 exe = "executable"
-subprocess.Popen(args, shell=use_shell, executable=exe)  # $getCommand=exe SPURIOUS: getCommand=args
+subprocess.Popen(args, shell=use_shell, executable=exe)  # $getCommand=exe
 
 
 ################################################################################

From cf094c2f4f189134882bc2065d728674b1f915bf Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Fri, 3 Feb 2023 12:06:43 +0100
Subject: [PATCH 13/17] adjust which folders are seen as exported to remove an
 FP

---
 python/ql/lib/semmle/python/frameworks/Setuptools.qll | 11 +++++++----
 .../setup_posix.py                                    |  4 ++++
 2 files changed, 11 insertions(+), 4 deletions(-)
 create mode 100644 python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/setup_posix.py

diff --git a/python/ql/lib/semmle/python/frameworks/Setuptools.qll b/python/ql/lib/semmle/python/frameworks/Setuptools.qll
index 32509cfccec..5edff4bf92c 100644
--- a/python/ql/lib/semmle/python/frameworks/Setuptools.qll
+++ b/python/ql/lib/semmle/python/frameworks/Setuptools.qll
@@ -19,7 +19,12 @@ module Setuptools {
    * Gets a file or folder that is exported by a library.
    */
   private Container getALibraryExportedContainer() {
-    result = setupFile().getParent()
+    // a child folder of the root that has a setup.py file
+    result = setupFile().getParent().(Folder).getAFolder() and
+    // where the folder has __init__.py file
+    exists(result.(Folder).getFile("__init__.py")) and
+    // and is not a test folder
+    not result.(Folder).getBaseName() = ["test", "tests", "testing"]
     or
     // child of a library exported container
     result = getALibraryExportedContainer().getAChildContainer() and
@@ -29,9 +34,7 @@ module Setuptools {
       or
       // or a folder with an __init__.py file
       exists(result.(Folder).getFile("__init__.py"))
-    ) and
-    // that is not a test folder
-    not result.(Folder).getBaseName() = ["test", "tests", "testing"]
+    )
   }
 
   /**
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/setup_posix.py b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/setup_posix.py
new file mode 100644
index 00000000000..d5f3fb63d96
--- /dev/null
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/setup_posix.py
@@ -0,0 +1,4 @@
+import os 
+
+def unsafe_setup(name):
+    os.system("ping " + name) # $result=OK - this is inside a setyp script, so it's fine.
\ No newline at end of file

From c5350ca6a01298f54228108eeda5b14cef43d2dd Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Fri, 3 Feb 2023 12:09:01 +0100
Subject: [PATCH 14/17] add change-note

---
 .../2023-02-03-unsafe-shell-command-construction.md           | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 python/ql/src/change-notes/2023-02-03-unsafe-shell-command-construction.md

diff --git a/python/ql/src/change-notes/2023-02-03-unsafe-shell-command-construction.md b/python/ql/src/change-notes/2023-02-03-unsafe-shell-command-construction.md
new file mode 100644
index 00000000000..0654a93582b
--- /dev/null
+++ b/python/ql/src/change-notes/2023-02-03-unsafe-shell-command-construction.md
@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `py/shell-command-constructed-from-input`, to detect libraries that unsafely construct shell commands from their inputs.

From 8e05fdb36942525ff36da50d276452a55948df46 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Fri, 3 Feb 2023 15:00:31 +0100
Subject: [PATCH 15/17] make more imports private

---
 .../UnsafeShellCommandConstructionCustomizations.qll      | 6 +++---
 .../dataflow/UnsafeShellCommandConstructionQuery.qll      | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
index fcffb9f5145..903b8f3eb72 100644
--- a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
+++ b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
@@ -5,9 +5,9 @@
  */
 
 private import python
-import semmle.python.dataflow.new.DataFlow
-import semmle.python.dataflow.new.TaintTracking
-import CommandInjectionCustomizations::CommandInjection as CommandInjection
+private import semmle.python.dataflow.new.DataFlow
+private import semmle.python.dataflow.new.TaintTracking
+private import CommandInjectionCustomizations::CommandInjection as CommandInjection
 private import semmle.python.Concepts as Concepts
 
 /**
diff --git a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionQuery.qll b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionQuery.qll
index 6fc567289e7..9003b19015c 100644
--- a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionQuery.qll
+++ b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionQuery.qll
@@ -7,11 +7,11 @@
  */
 
 import python
-import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction
 import semmle.python.dataflow.new.DataFlow
-import semmle.python.dataflow.new.TaintTracking
-import CommandInjectionCustomizations::CommandInjection as CommandInjection
-import semmle.python.dataflow.new.BarrierGuards
+import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction
+private import semmle.python.dataflow.new.TaintTracking
+private import CommandInjectionCustomizations::CommandInjection as CommandInjection
+private import semmle.python.dataflow.new.BarrierGuards
 
 /**
  * A taint-tracking configuration for detecting shell command constructed from library input vulnerabilities.

From 759854991a4ff006cae87703e008cdc331f40fb9 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Wed, 15 Feb 2023 11:10:43 +0100
Subject: [PATCH 16/17] fix various nits based on feedback

---
 .../UnsafeShellCommandConstructionCustomizations.qll        | 6 +++---
 .../dataflow/UnsafeShellCommandConstructionQuery.qll        | 3 +--
 .../Security/CWE-078/UnsafeShellCommandConstruction.qhelp   | 6 +++---
 .../CWE-078/examples/unsafe-shell-command-construction.py   | 2 +-
 .../examples/unsafe-shell-command-construction_fixed.py     | 2 +-
 .../UnsafeShellCommandConstruction.expected                 | 2 +-
 6 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
index 903b8f3eb72..390c9738cc3 100644
--- a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
+++ b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll
@@ -68,11 +68,11 @@ module UnsafeShellCommandConstruction {
     Fstring fstring;
 
     StringInterpolationAsSink() {
-      isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr() = fstring), s) and
+      isUsedAsShellCommand(DataFlow::exprNode(fstring), s) and
       this.asExpr() = fstring.getASubExpression()
     }
 
-    override string describe() { result = "string construction" }
+    override string describe() { result = "f-string" }
 
     override DataFlow::Node getCommandExecution() { result = s }
 
@@ -101,7 +101,7 @@ module UnsafeShellCommandConstruction {
   }
 
   /**
-   * A string constructed using a `.join(" ")` call, where the resulting string ends up being executed as a shell command.
+   * A string constructed using a `" ".join(...)` call, where the resulting string ends up being executed as a shell command.
    */
   class ArrayJoin extends Sink {
     Concepts::SystemCommandExecution s;
diff --git a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionQuery.qll b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionQuery.qll
index 9003b19015c..0d9ebb8a472 100644
--- a/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionQuery.qll
+++ b/python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionQuery.qll
@@ -24,8 +24,7 @@ class Configuration extends TaintTracking::Configuration {
   override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
   override predicate isSanitizer(DataFlow::Node node) {
-    node instanceof CommandInjection::Sanitizer or // using all sanitizers from `rb/command-injection`
-    node instanceof StringConstCompareBarrier
+    node instanceof CommandInjection::Sanitizer // using all sanitizers from `rb/command-injection`
   }
 
   // override to require the path doesn't have unmatched return steps
diff --git a/python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp b/python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp
index 3176d5c035d..007e691c243 100644
--- a/python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp
+++ b/python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp
@@ -4,7 +4,7 @@
 <qhelp>
 <overview>
 	<p>
-		Dynamically constructing a shell command with inputs from exported 
+		Dynamically constructing a shell command with inputs from library 
 		functions may inadvertently change the meaning of the shell command.
 		
 		Clients using the exported function may use inputs containing
@@ -21,7 +21,7 @@
 
 	<p>
 		If possible, provide the dynamic arguments to the shell as an array 
-		to APIs such as <code>system(..)</code> to avoid interpretation by the shell.
+		to APIs such as <code>subprocess.run</code> to avoid interpretation by the shell.
 	</p>
 
 	<p>
@@ -55,7 +55,7 @@
 
 	<p>
 		To avoid such potentially catastrophic behaviors, provide the
-		input from exported functions as an argument that does not
+		input from library functions as an argument that does not
 		get interpreted by a shell:
 	</p>
 
diff --git a/python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction.py b/python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction.py
index 644af24938d..3803f59059b 100644
--- a/python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction.py
+++ b/python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction.py
@@ -1,4 +1,4 @@
 import os
 
-def download (path): 
+def download(path): 
     os.system("wget " + path) # NOT OK
diff --git a/python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction_fixed.py b/python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction_fixed.py
index d62b3478bf1..6009774b9a8 100644
--- a/python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction_fixed.py
+++ b/python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction_fixed.py
@@ -1,4 +1,4 @@
 import subprocess
 
-def download (path): 
+def download(path): 
     subprocess.run(["wget", path]) # OK
diff --git a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
index 3378661f451..fc9f49f1c23 100644
--- a/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
+++ b/python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
@@ -30,7 +30,7 @@ nodes
 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 |
-| src/unsafe_shell_test.py:8:15:8:28 | ControlFlowNode for Fstring | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name | This string construction 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:8:5:8:29 | ControlFlowNode for Attribute() | shell command |
+| src/unsafe_shell_test.py:8:15:8:28 | ControlFlowNode for Fstring | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name | This f-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:8:5:8:29 | ControlFlowNode for Attribute() | shell command |
 | src/unsafe_shell_test.py:11:15:11:38 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:11:25:11:38 | ControlFlowNode for Attribute() | 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:11:5:11:39 | ControlFlowNode for Attribute() | shell command |
 | src/unsafe_shell_test.py:14:15:14:40 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:14:25:14:40 | ControlFlowNode for Attribute() | 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:14:5:14:41 | ControlFlowNode for Attribute() | shell command |
 | src/unsafe_shell_test.py:17:15:17:36 | ControlFlowNode for Attribute() | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:17:32:17:35 | 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:17:5:17:37 | ControlFlowNode for Attribute() | shell command |

From 6a5d6eb5c2fff4d1beeaa7ecf514beff716d8668 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Mon, 13 Mar 2023 14:56:42 +0100
Subject: [PATCH 17/17] lower precision of
 py/shell-command-constructed-from-input to medium

---
 .../ql/src/Security/CWE-078/UnsafeShellCommandConstruction.ql   | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.ql b/python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.ql
index d22ee170f3a..10f4b771261 100644
--- a/python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.ql
+++ b/python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.ql
@@ -5,7 +5,7 @@
  * @kind path-problem
  * @problem.severity error
  * @security-severity 6.3
- * @precision high
+ * @precision medium
  * @id py/shell-command-constructed-from-input
  * @tags correctness
  *       security