Python: Port py/command-line-injection to use proper source/sink customization

Author: RasmusWL

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

@@ -19,7 +19,7 @@ import python
 import semmle.python.security.dataflow.CommandInjection
 import DataFlow::PathGraph
 
-from CommandInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
+from CommandInjection::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
 where config.hasFlowPath(source, sink)
 select sink.getNode(), source, sink, "This command depends on $@.", source.getNode(),
   "a user-provided value"

python/ql/src/semmle/python/security/dataflow/CommandInjection.qll

@@ -1,56 +1,42 @@
 /**
- * Provides a taint-tracking configuration for detecting command injection
- * vulnerabilities.
+ * Provides a taint-tracking configuration for detecting "command injection" vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if
+ * `CommandInjection::Configuration` is needed, otherwise
+ * `CommandInjectionCustomizations` should be imported instead.
  */
 
-import python
+private import python
 import semmle.python.dataflow.new.DataFlow
 import semmle.python.dataflow.new.TaintTracking
-import semmle.python.Concepts
-import semmle.python.dataflow.new.RemoteFlowSources
-import semmle.python.dataflow.new.BarrierGuards
 
 /**
- * A taint-tracking configuration for detecting command injection vulnerabilities.
+ * Provides a taint-tracking configuration for detecting "command injection" vulnerabilities.
  */
-class CommandInjectionConfiguration extends TaintTracking::Configuration {
-  CommandInjectionConfiguration() { this = "CommandInjectionConfiguration" }
-
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
-
-  override predicate isSink(DataFlow::Node sink) {
-    sink = any(SystemCommandExecution e).getCommand() and
-    // Since the implementation of standard library functions such `os.popen` looks like
-    // ```py
-    // def popen(cmd, mode="r", buffering=-1):
-    //     ...
-    //     proc = subprocess.Popen(cmd, ...)
-    // ```
-    // any time we would report flow to the `os.popen` sink, we can ALSO report the flow
-    // from the `cmd` parameter to the `subprocess.Popen` sink -- obviously we don't
-    // want that.
-    //
-    // However, simply removing taint edges out of a sink is not a good enough solution,
-    // since we would only flag one of the `os.system` calls in the following example
-    // due to use-use flow
-    // ```py
-    // os.system(cmd)
-    // os.system(cmd)
-    // ```
-    //
-    // Best solution I could come up with is to exclude all sinks inside the modules of
-    // known sinks. This does have a downside: If we have overlooked a function in any
-    // of these, that internally runs a command, we no longer give an alert :| -- and we
-    // need to keep them updated (which is hard to remember)
-    //
-    // This does not only affect `os.popen`, but also the helper functions in
-    // `subprocess`. See:
-    // https://github.com/python/cpython/blob/fa7ce080175f65d678a7d5756c94f82887fc9803/Lib/os.py#L974
-    // https://github.com/python/cpython/blob/fa7ce080175f65d678a7d5756c94f82887fc9803/Lib/subprocess.py#L341
-    not sink.getScope().getEnclosingModule().getName() in ["os", "subprocess", "platform", "popen2"]
-  }
+module CommandInjection {
+  import CommandInjectionCustomizations::CommandInjection
+
+  /**
+   * A taint-tracking configuration for detecting "command injection" vulnerabilities.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "CommandInjection" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof StringConstCompare
+    override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+
+    override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+      guard instanceof SanitizerGuard
+    }
   }
 }
+
+/**
+ * DEPRECATED: Don't extend this class for customization, since this will lead to bad
+ * performance, instead use the new `CommandInjectionCustomizations.qll` file, and extend
+ * its' classes.
+ */
+deprecated class CommandInjectionConfiguration = CommandInjection::Configuration;

python/ql/src/semmle/python/security/dataflow/CommandInjectionCustomizations.qll

@@ -0,0 +1,87 @@
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "command injection"
+ * vulnerabilities, as well as extension points for adding your own.
+ */
+
+private import python
+private import semmle.python.dataflow.new.DataFlow
+private import semmle.python.Concepts
+private import semmle.python.dataflow.new.RemoteFlowSources
+private import semmle.python.dataflow.new.BarrierGuards
+
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "command injection"
+ * vulnerabilities, as well as extension points for adding your own.
+ */
+module CommandInjection {
+  /**
+   * A data flow source for "command injection" vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for "command injection" vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A sanitizer for "command injection" vulnerabilities.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A sanitizer guard for "command injection" vulnerabilities.
+   */
+  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
+
+  /**
+   * A source of remote user input, considered as a flow source.
+   */
+  class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
+
+  /**
+   * A command execution, considered as a flow sink.
+   */
+  class CommandExecutionAsSink extends Sink {
+    CommandExecutionAsSink() {
+      this = any(SystemCommandExecution e).getCommand() and
+      // Since the implementation of standard library functions such `os.popen` looks like
+      // ```py
+      // def popen(cmd, mode="r", buffering=-1):
+      //     ...
+      //     proc = subprocess.Popen(cmd, ...)
+      // ```
+      // any time we would report flow to the `os.popen` sink, we can ALSO report the flow
+      // from the `cmd` parameter to the `subprocess.Popen` sink -- obviously we don't
+      // want that.
+      //
+      // However, simply removing taint edges out of a sink is not a good enough solution,
+      // since we would only flag one of the `os.system` calls in the following example
+      // due to use-use flow
+      // ```py
+      // os.system(cmd)
+      // os.system(cmd)
+      // ```
+      //
+      // Best solution I could come up with is to exclude all sinks inside the modules of
+      // known sinks. This does have a downside: If we have overlooked a function in any
+      // of these, that internally runs a command, we no longer give an alert :| -- and we
+      // need to keep them updated (which is hard to remember)
+      //
+      // This does not only affect `os.popen`, but also the helper functions in
+      // `subprocess`. See:
+      // https://github.com/python/cpython/blob/fa7ce080175f65d678a7d5756c94f82887fc9803/Lib/os.py#L974
+      // https://github.com/python/cpython/blob/fa7ce080175f65d678a7d5756c94f82887fc9803/Lib/subprocess.py#L341
+      not this.getScope().getEnclosingModule().getName() in [
+          "os", "subprocess", "platform", "popen2"
+        ]
+    }
+  }
+
+  /**
+   * A comparison with a constant string, considered as a sanitizer-guard.
+   */
+  class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }
+}