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

python: Rewrite path injection query to use flow state

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,8 +68,12 @@ 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
@@ -73,8 +81,12 @@ query predicate edges(CustomPathNode a, CustomPathNode b) {
   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)