Python: Deprecate and replace BarrierGuard class.

Author: aschackmull

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

@@ -119,16 +119,21 @@ module Path {
   }
 
   /** A data-flow node that checks that a path is safe to access. */
-  class SafeAccessCheck extends DataFlow::BarrierGuard instanceof SafeAccessCheck::Range {
-    override predicate checks(ControlFlowNode node, boolean branch) {
-      SafeAccessCheck::Range.super.checks(node, branch)
-    }
+  class SafeAccessCheck extends DataFlow::ExprNode {
+    SafeAccessCheck() { this = DataFlow::BarrierGuard<safeAccessCheck/3>::getABarrierNode() }
+  }
+
+  private predicate safeAccessCheck(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) {
+    g.(SafeAccessCheck::Range).checks(node, branch)
   }
 
   /** Provides a class for modeling new path safety checks. */
   module SafeAccessCheck {
     /** A data-flow node that checks that a path is safe to access. */
-    abstract class Range extends DataFlow::BarrierGuard { }
+    abstract class Range extends DataFlow::GuardNode {
+      /** Holds if this guard validates `node` upon evaluating to `branch`. */
+      abstract predicate checks(ControlFlowNode node, boolean branch);
+    }
   }
 }
 

python/ql/lib/semmle/python/dataflow/new/BarrierGuards.qll

@@ -3,8 +3,44 @@
 private import python
 private import semmle.python.dataflow.new.DataFlow
 
+private predicate stringConstCompare(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) {
+  exists(CompareNode cn | cn = g |
+    exists(StrConst str_const, Cmpop op |
+      op = any(Eq eq) and branch = true
+      or
+      op = any(NotEq ne) and branch = false
+    |
+      cn.operands(str_const.getAFlowNode(), op, node)
+      or
+      cn.operands(node, op, str_const.getAFlowNode())
+    )
+    or
+    exists(IterableNode str_const_iterable, Cmpop op |
+      op = any(In in_) and branch = true
+      or
+      op = any(NotIn ni) and branch = false
+    |
+      forall(ControlFlowNode elem | elem = str_const_iterable.getAnElement() |
+        elem.getNode() instanceof StrConst
+      ) and
+      cn.operands(node, op, str_const_iterable)
+    )
+  )
+}
+
 /** A validation of unknown node by comparing with a constant string value. */
-class StringConstCompare extends DataFlow::BarrierGuard, CompareNode {
+class StringConstCompareBarrier extends DataFlow::Node {
+  StringConstCompareBarrier() {
+    this = DataFlow::BarrierGuard<stringConstCompare/3>::getABarrierNode()
+  }
+}
+
+/**
+ * DEPRECATED: Use `StringConstCompareBarrier` instead.
+ *
+ * A validation of unknown node by comparing with a constant string value.
+ */
+deprecated class StringConstCompare extends DataFlow::BarrierGuard, CompareNode {
   ControlFlowNode checked_node;
   boolean safe_branch;
 

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll

@@ -540,6 +540,35 @@ class GuardNode extends ControlFlowNode {
 }
 
 /**
+ * Holds if the guard `g` validates `node` upon evaluating to `branch`.
+ *
+ * The expression `e` is expected to be a syntactic part of the guard `g`.
+ * For example, the guard `g` might be a call `isSafe(x)` and the expression `e`
+ * the argument `x`.
+ */
+signature predicate guardChecksSig(GuardNode g, ControlFlowNode node, boolean branch);
+
+/**
+ * Provides a set of barrier nodes for a guard that validates a node.
+ *
+ * This is expected to be used in `isBarrier`/`isSanitizer` definitions
+ * in data flow and taint tracking.
+ */
+module BarrierGuard<guardChecksSig/3 guardChecks> {
+  /** Gets a node that is safely guarded by the given guard check. */
+  ExprNode getABarrierNode() {
+    exists(GuardNode g, EssaDefinition def, ControlFlowNode node, boolean branch |
+      AdjacentUses::useOfDef(def, node) and
+      guardChecks(g, node, branch) and
+      AdjacentUses::useOfDef(def, result.asCfgNode()) and
+      g.controlsBlock(result.asCfgNode().getBasicBlock(), branch)
+    )
+  }
+}
+
+/**
+ * DEPRECATED: Use `BarrierGuard` module instead.
+ *
  * A guard that validates some expression.
  *
  * To use this in a configuration, extend the class and provide a
@@ -548,7 +577,7 @@ class GuardNode extends ControlFlowNode {
  *
  * It is important that all extending classes in scope are disjoint.
  */
-class BarrierGuard extends GuardNode {
+deprecated class BarrierGuard extends GuardNode {
   /** Holds if this guard validates `node` upon evaluating to `branch`. */
   abstract predicate checks(ControlFlowNode node, boolean branch);
 

python/ql/lib/semmle/python/dataflow/new/internal/TaintTrackingPrivate.qll

@@ -10,12 +10,6 @@ private import semmle.python.ApiGraphs
  */
 predicate defaultTaintSanitizer(DataFlow::Node node) { none() }
 
-/**
- * Holds if `guard` should be a sanitizer guard in all global taint flow configurations
- * but not in local taint.
- */
-predicate defaultTaintSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
-
 /**
  * Holds if default `TaintTracking::Configuration`s should allow implicit reads
  * of `c` at sinks and inputs to additional taint steps.

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

@@ -32,9 +32,11 @@ module CodeInjection {
   abstract class Sanitizer extends DataFlow::Node { }
 
   /**
+   * DEPRECATED: Use `Sanitizer` instead.
+   *
    * A sanitizer guard for "code injection" vulnerabilities.
    */
-  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
+  abstract deprecated class SanitizerGuard extends DataFlow::BarrierGuard { }
 
   /**
    * A source of remote user input, considered as a flow source.

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

@@ -23,7 +23,7 @@ class Configuration extends TaintTracking::Configuration {
 
   override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
 
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+  deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
     guard instanceof SanitizerGuard
   }
 }

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

@@ -32,9 +32,11 @@ module CommandInjection {
   abstract class Sanitizer extends DataFlow::Node { }
 
   /**
+   * DEPRECATED: Use `Sanitizer` instead.
+   *
    * A sanitizer guard for "command injection" vulnerabilities.
    */
-  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
+  abstract deprecated class SanitizerGuard extends DataFlow::BarrierGuard { }
 
   /**
    * A source of remote user input, considered as a flow source.
@@ -83,5 +85,5 @@ module CommandInjection {
   /**
    * A comparison with a constant string, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }
+  class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
 }

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

@@ -23,7 +23,7 @@ class Configuration extends TaintTracking::Configuration {
 
   override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
 
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+  deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
     guard instanceof SanitizerGuard
   }
 }

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

@@ -42,14 +42,18 @@ module LdapInjection {
   abstract class FilterSanitizer extends DataFlow::Node { }
 
   /**
+   * DEPRECATED: Use `DnSanitizer` instead.
+   *
    * A sanitizer guard for "ldap injection" vulnerabilities.
    */
-  abstract class DnSanitizerGuard extends DataFlow::BarrierGuard { }
+  abstract deprecated class DnSanitizerGuard extends DataFlow::BarrierGuard { }
 
   /**
+   * DEPRECATED: Use `FilterSanitizer` instead.
+   *
    * A sanitizer guard for "ldap injection" vulnerabilities.
    */
-  abstract class FilterSanitizerGuard extends DataFlow::BarrierGuard { }
+  abstract deprecated class FilterSanitizerGuard extends DataFlow::BarrierGuard { }
 
   /**
    * A source of remote user input, considered as a flow source.
@@ -73,12 +77,12 @@ module LdapInjection {
   /**
    * A comparison with a constant string, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsDnSanitizerGuard extends DnSanitizerGuard, StringConstCompare { }
+  class StringConstCompareAsDnSanitizerGuard extends DnSanitizer, StringConstCompareBarrier { }
 
   /**
    * A comparison with a constant string, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsFilterSanitizerGuard extends FilterSanitizerGuard, StringConstCompare {
+  class StringConstCompareAsFilterSanitizerGuard extends FilterSanitizer, StringConstCompareBarrier {
   }
 
   /**

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

@@ -26,7 +26,7 @@ class DnConfiguration extends TaintTracking::Configuration {
 
   override predicate isSanitizer(DataFlow::Node node) { node instanceof DnSanitizer }
 
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+  deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
     guard instanceof DnSanitizerGuard
   }
 }
@@ -44,7 +44,7 @@ class FilterConfiguration extends TaintTracking::Configuration {
 
   override predicate isSanitizer(DataFlow::Node node) { node instanceof FilterSanitizer }
 
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+  deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
     guard instanceof FilterSanitizerGuard
   }
 }