Merge pull request github#7687 from yoff/python/PathInjection-FlowState

python: Rewrite path injection query to use flow state

Author: RasmusWL

python/ql/lib/semmle/python/Concepts.qll

@@ -115,6 +115,9 @@ module Path {
     PathNormalization::Range range;
 
     PathNormalization() { this = range }
+
+    /** Gets an argument to this path normalization that is interpreted as a path. */
+    DataFlow::Node getPathArg() { result = range.getPathArg() }
   }
 
   /** Provides a class for modeling new path normalization APIs. */
@@ -123,7 +126,10 @@ module Path {
      * A data-flow node that performs path normalization. This is often needed in order
      * to safely access paths.
      */
-    abstract class Range extends DataFlow::Node { }
+    abstract class Range extends DataFlow::Node {
+      /** Gets an argument to this path normalization that is interpreted as a path. */
+      abstract DataFlow::Node getPathArg();
+    }
   }
 
   /** A data-flow node that checks that a path is safe to access. */

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

@@ -970,7 +970,7 @@ private module StdlibPrivate {
   private class OsPathNormpathCall extends Path::PathNormalization::Range, DataFlow::CallCfgNode {
     OsPathNormpathCall() { this = os::path().getMember("normpath").getACall() }
 
-    DataFlow::Node getPathArg() { result in [this.getArg(0), this.getArgByName("path")] }
+    override DataFlow::Node getPathArg() { result in [this.getArg(0), this.getArgByName("path")] }
   }
 
   /**
@@ -980,7 +980,7 @@ private module StdlibPrivate {
   private class OsPathAbspathCall extends Path::PathNormalization::Range, DataFlow::CallCfgNode {
     OsPathAbspathCall() { this = os::path().getMember("abspath").getACall() }
 
-    DataFlow::Node getPathArg() { result in [this.getArg(0), this.getArgByName("path")] }
+    override DataFlow::Node getPathArg() { result in [this.getArg(0), this.getArgByName("path")] }
   }
 
   /**
@@ -990,7 +990,7 @@ private module StdlibPrivate {
   private class OsPathRealpathCall extends Path::PathNormalization::Range, DataFlow::CallCfgNode {
     OsPathRealpathCall() { this = os::path().getMember("realpath").getACall() }
 
-    DataFlow::Node getPathArg() { result in [this.getArg(0), this.getArgByName("path")] }
+    override DataFlow::Node getPathArg() { result in [this.getArg(0), this.getArgByName("path")] }
   }
 
   /**

python/ql/lib/semmle/python/security/dataflow/ChainedConfigs12.qll

@@ -1,4 +1,6 @@
 /**
+ * DEPRECATED -- use flow state instead
+ *
  * This defines a `PathGraph` where sinks from `TaintTracking::Configuration`s are identified with
  * sources from `TaintTracking2::Configuration`s if they represent the same `ControlFlowNode`.
  *
@@ -28,9 +30,11 @@ private newtype TCustomPathNode =
   CrossoverNode(DataFlow::Node node) { crossoverNode(node) }
 
 /**
+ * DEPRECATED: Use flow state instead
+ *
  * A class representing the set of all the path nodes in either config.
  */
-class CustomPathNode extends TCustomPathNode {
+deprecated class CustomPathNode extends TCustomPathNode {
   /** Gets the PathNode if it is in Config1. */
   DataFlow::PathNode asNode1() {
     this = Config1Node(result) or this = CrossoverNode(result.getNode())
@@ -64,17 +68,25 @@ class CustomPathNode extends TCustomPathNode {
   }
 }
 
-/** Holds if `(a,b)` is an edge in the graph of data flow path explanations. */
-query predicate edges(CustomPathNode a, CustomPathNode b) {
+/**
+ * DEPRECATED: Use flow state instead
+ *
+ * Holds if `(a,b)` is an edge in the graph of data flow path explanations.
+ */
+deprecated query predicate edges(CustomPathNode a, CustomPathNode b) {
   // Edge is in Config1 graph
   DataFlow::PathGraph::edges(a.asNode1(), b.asNode1())
   or
   // Edge is in Config2 graph
   DataFlow2::PathGraph::edges(a.asNode2(), b.asNode2())
 }
 
-/** Holds if `n` is a node in the graph of data flow path explanations. */
-query predicate nodes(CustomPathNode n, string key, string val) {
+/**
+ * DEPRECATED: Use flow state instead
+ *
+ * Holds if `n` is a node in the graph of data flow path explanations.
+ */
+deprecated query predicate nodes(CustomPathNode n, string key, string val) {
   // Node is in Config1 graph
   DataFlow::PathGraph::nodes(n.asNode1(), key, val)
   or

python/ql/lib/semmle/python/security/dataflow/PathInjection.qll

@@ -2,24 +2,102 @@
  * Provides taint-tracking configurations for detecting "path injection" vulnerabilities.
  *
  * Note, for performance reasons: only import this file if
- * the Configurations or the `pathInjection` predicate are needed, otherwise
+ * `PathInjection::Configuration` is needed, otherwise
  * `PathInjectionCustomizations` should be imported instead.
  */
 
 private import python
 private import semmle.python.Concepts
-private import semmle.python.dataflow.new.DataFlow
+import semmle.python.dataflow.new.DataFlow
+import semmle.python.dataflow.new.TaintTracking
+
+/**
+ * Provides a taint-tracking configuration for detecting "path injection" vulnerabilities.
+ */
+module PathInjection {
+  import PathInjectionCustomizations::PathInjection
+
+  /**
+   * A taint-tracking configuration for detecting "path injection" vulnerabilities.
+   *
+   * This configuration uses two flow states, `NotNormalized` and `NormalizedUnchecked`,
+   * to track the requirement that a file path must be first normalized and then checked
+   * before it is safe to use.
+   *
+   * At sources, paths are assumed not normalized. At normalization points, they change
+   * state to `NormalizedUnchecked` after which they can be made safe by an appropriate
+   * check of the prefix.
+   *
+   * Such checks are ineffective in the `NotNormalized` state.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "PathInjection" }
+
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
+      source instanceof Source and state instanceof NotNormalized
+    }
+
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
+      sink instanceof Sink and
+      (
+        state instanceof NotNormalized or
+        state instanceof NormalizedUnchecked
+      )
+    }
+
+    override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+
+    override predicate isBarrier(DataFlow::Node node, DataFlow::FlowState state) {
+      // Block `NotNormalized` paths here, since they change state to `NormalizedUnchecked`
+      node instanceof Path::PathNormalization and
+      state instanceof NotNormalized
+      or
+      node = any(Path::SafeAccessCheck c).getAGuardedNode() and
+      state instanceof NormalizedUnchecked
+    }
+
+    override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+      guard instanceof SanitizerGuard
+    }
+
+    override predicate isAdditionalFlowStep(
+      DataFlow::Node nodeFrom, DataFlow::FlowState stateFrom, DataFlow::Node nodeTo,
+      DataFlow::FlowState stateTo
+    ) {
+      nodeFrom = nodeTo.(Path::PathNormalization).getPathArg() and
+      stateFrom instanceof NotNormalized and
+      stateTo instanceof NormalizedUnchecked
+    }
+  }
+
+  /** A state signifying that the file path has not been normalized. */
+  class NotNormalized extends DataFlow::FlowState {
+    NotNormalized() { this = "NotNormalized" }
+  }
+
+  /** A state signifying that the file path has been normalized, but not checked. */
+  class NormalizedUnchecked extends DataFlow::FlowState {
+    NormalizedUnchecked() { this = "NormalizedUnchecked" }
+  }
+}
+
+// ---------------------------------------------------------------------------
+// Old, deprecated code
+// ---------------------------------------------------------------------------
 private import semmle.python.dataflow.new.DataFlow2
-private import semmle.python.dataflow.new.TaintTracking
 private import semmle.python.dataflow.new.TaintTracking2
-import ChainedConfigs12
+private import ChainedConfigs12
 import PathInjectionCustomizations::PathInjection
 
 // ---------------------------------------------------------------------------
 // Case 1. The path is never normalized.
 // ---------------------------------------------------------------------------
-/** Configuration to find paths from sources to sinks that contain no normalization. */
-class PathNotNormalizedConfiguration extends TaintTracking::Configuration {
+/**
+ * DEPRECATED: Use `PathInjection::Configuration` instead
+ *
+ * Configuration to find paths from sources to sinks that contain no normalization.
+ */
+deprecated class PathNotNormalizedConfiguration extends TaintTracking::Configuration {
   PathNotNormalizedConfiguration() { this = "PathNotNormalizedConfiguration" }
 
   override predicate isSource(DataFlow::Node source) { source instanceof Source }
@@ -38,18 +116,24 @@ class PathNotNormalizedConfiguration extends TaintTracking::Configuration {
 }
 
 /**
+ * DEPRECATED: Use `PathInjection::Configuration` instead
+ *
  * Holds if there is a path injection from source to sink, where the (python) path is
  * not normalized.
  */
-predicate pathNotNormalized(CustomPathNode source, CustomPathNode sink) {
+deprecated predicate pathNotNormalized(CustomPathNode source, CustomPathNode sink) {
   any(PathNotNormalizedConfiguration config).hasFlowPath(source.asNode1(), sink.asNode1())
 }
 
 // ---------------------------------------------------------------------------
 // Case 2. The path is normalized at least once, but never checked afterwards.
 // ---------------------------------------------------------------------------
-/** Configuration to find paths from sources to normalizations that contain no prior normalizations. */
-class FirstNormalizationConfiguration extends TaintTracking::Configuration {
+/**
+ * DEPRECATED: Use `PathInjection::Configuration` instead
+ *
+ * Configuration to find paths from sources to normalizations that contain no prior normalizations.
+ */
+deprecated class FirstNormalizationConfiguration extends TaintTracking::Configuration {
   FirstNormalizationConfiguration() { this = "FirstNormalizationConfiguration" }
 
   override predicate isSource(DataFlow::Node source) { source instanceof Source }
@@ -65,8 +149,12 @@ class FirstNormalizationConfiguration extends TaintTracking::Configuration {
   }
 }
 
-/** Configuration to find paths from normalizations to sinks that do not go through a check. */
-class NormalizedPathNotCheckedConfiguration extends TaintTracking2::Configuration {
+/**
+ * DEPRECATED: Use `PathInjection::Configuration` instead
+ *
+ * Configuration to find paths from normalizations to sinks that do not go through a check.
+ */
+deprecated class NormalizedPathNotCheckedConfiguration extends TaintTracking2::Configuration {
   NormalizedPathNotCheckedConfiguration() { this = "NormalizedPathNotCheckedConfiguration" }
 
   override predicate isSource(DataFlow::Node source) { source instanceof Path::PathNormalization }
@@ -83,10 +171,12 @@ class NormalizedPathNotCheckedConfiguration extends TaintTracking2::Configuratio
 }
 
 /**
+ * DEPRECATED: Use `PathInjection::Configuration` instead
+ *
  * Holds if there is a path injection from source to sink, where the (python) path is
  * normalized at least once, but never checked afterwards.
  */
-predicate pathNotCheckedAfterNormalization(CustomPathNode source, CustomPathNode sink) {
+deprecated predicate pathNotCheckedAfterNormalization(CustomPathNode source, CustomPathNode sink) {
   exists(
     FirstNormalizationConfiguration config, DataFlow::PathNode mid1, DataFlow2::PathNode mid2,
     NormalizedPathNotCheckedConfiguration config2
@@ -100,8 +190,12 @@ predicate pathNotCheckedAfterNormalization(CustomPathNode source, CustomPathNode
 // ---------------------------------------------------------------------------
 // Query: Either case 1 or case 2.
 // ---------------------------------------------------------------------------
-/** Holds if there is a path injection from source to sink */
-predicate pathInjection(CustomPathNode source, CustomPathNode sink) {
+/**
+ * DEPRECATED: Use `PathInjection::Configuration` instead
+ *
+ * Holds if there is a path injection from source to sink
+ */
+deprecated predicate pathInjection(CustomPathNode source, CustomPathNode sink) {
   pathNotNormalized(source, sink)
   or
   pathNotCheckedAfterNormalization(source, sink)

python/ql/src/Security/CWE-022/PathInjection.ql

@@ -18,7 +18,9 @@
 
 import python
 import semmle.python.security.dataflow.PathInjection
+import DataFlow::PathGraph
 
-from CustomPathNode source, CustomPathNode sink
-where pathInjection(source, sink)
-select sink, source, sink, "This path depends on $@.", source, "a user-provided value"
+from PathInjection::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "This path depends on $@.", source.getNode(),
+  "a user-provided value"

python/ql/src/experimental/Security/CWE-943/NoSQLInjection.ql

@@ -11,8 +11,9 @@
 
 import python
 import experimental.semmle.python.security.injection.NoSQLInjection
+import DataFlow::PathGraph
 
-from CustomPathNode source, CustomPathNode sink
-where noSQLInjectionFlow(source, sink)
+from NoSQLInjection::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
 select sink, source, sink, "$@ NoSQL query contains an unsanitized $@", sink, "This", source,
   "user-provided value"