Ruby: Deprecate and replace BarrierGuard class.

Author: aschackmull

ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll

@@ -4,6 +4,23 @@ private import ruby
 private import codeql.ruby.DataFlow
 private import codeql.ruby.CFG
 
+private predicate stringConstCompare(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch) {
+  exists(CfgNodes::ExprNodes::ComparisonOperationCfgNode c |
+    c = g and
+    exists(CfgNodes::ExprNodes::StringLiteralCfgNode strLitNode |
+      c.getExpr() instanceof EqExpr and branch = true
+      or
+      c.getExpr() instanceof CaseEqExpr and branch = true
+      or
+      c.getExpr() instanceof NEExpr and branch = false
+    |
+      c.getLeftOperand() = strLitNode and c.getRightOperand() = e
+      or
+      c.getLeftOperand() = e and c.getRightOperand() = strLitNode
+    )
+  )
+}
+
 /**
  * A validation of value by comparing with a constant string value, for example
  * in:
@@ -17,7 +34,28 @@ private import codeql.ruby.CFG
  * the equality operation guards against `dir` taking arbitrary values when used
  * in the `order` call.
  */
-class StringConstCompare extends DataFlow::BarrierGuard,
+class StringConstCompareBarrier extends DataFlow::Node {
+  StringConstCompareBarrier() {
+    this = DataFlow::BarrierGuard<stringConstCompare/3>::getABarrierNode()
+  }
+}
+
+/**
+ * DEPRECATED: Use `StringConstCompareBarrier` instead.
+ *
+ * A validation of value by comparing with a constant string value, for example
+ * in:
+ *
+ * ```rb
+ * dir = params[:order]
+ * dir = "DESC" unless dir == "ASC"
+ * User.order("name #{dir}")
+ * ```
+ *
+ * the equality operation guards against `dir` taking arbitrary values when used
+ * in the `order` call.
+ */
+deprecated class StringConstCompare extends DataFlow::BarrierGuard,
   CfgNodes::ExprNodes::ComparisonOperationCfgNode {
   private CfgNode checkedNode;
   // The value of the condition that results in the node being validated.
@@ -42,7 +80,41 @@ class StringConstCompare extends DataFlow::BarrierGuard,
   }
 }
 
+private predicate stringConstArrayInclusionCall(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch) {
+  exists(CfgNodes::ExprNodes::MethodCallCfgNode mc, ArrayLiteral aLit |
+    mc = g and
+    mc.getExpr().getMethodName() = "include?" and
+    [mc.getExpr().getReceiver(), mc.getExpr().getReceiver().(ConstantReadAccess).getValue()] = aLit
+  |
+    forall(Expr elem | elem = aLit.getAnElement() | elem instanceof StringLiteral) and
+    mc.getArgument(0) = e
+  ) and
+  branch = true
+}
+
+/**
+ * A validation of a value by checking for inclusion in an array of string
+ * literal values, for example in:
+ *
+ * ```rb
+ * name = params[:user_name]
+ * if %w(alice bob charlie).include? name
+ *   User.find_by("username = #{name}")
+ * end
+ * ```
+ *
+ * the `include?` call guards against `name` taking arbitrary values when used
+ * in the `find_by` call.
+ */
+class StringConstArrayInclusionCallBarrier extends DataFlow::Node {
+  StringConstArrayInclusionCallBarrier() {
+    this = DataFlow::BarrierGuard<stringConstArrayInclusionCall/3>::getABarrierNode()
+  }
+}
+
 /**
+ * DEPRECATED: Use `StringConstArrayInclusionCallBarrier` instead.
+ *
  * A validation of a value by checking for inclusion in an array of string
  * literal values, for example in:
  *
@@ -56,8 +128,7 @@ class StringConstCompare extends DataFlow::BarrierGuard,
  * the `include?` call guards against `name` taking arbitrary values when used
  * in the `find_by` call.
  */
-//
-class StringConstArrayInclusionCall extends DataFlow::BarrierGuard,
+deprecated class StringConstArrayInclusionCall extends DataFlow::BarrierGuard,
   CfgNodes::ExprNodes::MethodCallCfgNode {
   private CfgNode checkedNode;
 

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll

@@ -334,6 +334,66 @@ class ContentSet extends TContentSet {
   }
 }
 
+/**
+ * Holds if the guard `g` validates the expression `e` 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(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch);
+
+/**
+ * Provides a set of barrier nodes for a guard that validates an expression.
+ *
+ * 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. */
+  Node getABarrierNode() {
+    exists(
+      CfgNodes::ExprCfgNode g, boolean branch, CfgNodes::ExprCfgNode testedNode, Ssa::Definition def
+    |
+      def.getARead() = testedNode and
+      def.getARead() = result.asExpr() and
+      guardChecks(g, testedNode, branch) and
+      guardControlsBlock(g, result.asExpr().getBasicBlock(), branch)
+    )
+    or
+    result.asExpr() = getAMaybeGuardedCapturedDef().getARead()
+  }
+
+  /**
+   * Gets an implicit entry definition for a captured variable that
+   * may be guarded, because a call to the capturing callable is guarded.
+   *
+   * This is restricted to calls where the variable is captured inside a
+   * block.
+   */
+  private Ssa::Definition getAMaybeGuardedCapturedDef() {
+    exists(
+      CfgNodes::ExprCfgNode g, boolean branch, CfgNodes::ExprCfgNode testedNode,
+      Ssa::Definition def, CfgNodes::ExprNodes::CallCfgNode call
+    |
+      def.getARead() = testedNode and
+      guardChecks(g, testedNode, branch) and
+      SsaImpl::captureFlowIn(call, def, result) and
+      guardControlsBlock(g, call.getBasicBlock(), branch) and
+      result.getBasicBlock().getScope() = call.getExpr().(MethodCall).getBlock()
+    )
+  }
+}
+
+/** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
+private predicate guardControlsBlock(CfgNodes::ExprCfgNode guard, BasicBlock bb, boolean branch) {
+  exists(ConditionBlock conditionBlock, SuccessorTypes::BooleanSuccessor s |
+    guard = conditionBlock.getLastNode() and
+    s.getValue() = branch and
+    conditionBlock.controls(bb, s)
+  )
+}
+
 /**
  * A guard that validates some expression.
  *
@@ -343,7 +403,7 @@ class ContentSet extends TContentSet {
  *
  * It is important that all extending classes in scope are disjoint.
  */
-abstract class BarrierGuard extends CfgNodes::ExprCfgNode {
+abstract deprecated class BarrierGuard extends CfgNodes::ExprCfgNode {
   private ConditionBlock conditionBlock;
 
   BarrierGuard() { this = conditionBlock.getLastNode() }

ruby/ql/lib/codeql/ruby/dataflow/internal/TaintTrackingPrivate.qll

@@ -11,12 +11,6 @@ private import FlowSummaryImpl as FlowSummaryImpl
  */
 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.

ruby/ql/lib/codeql/ruby/security/CodeInjectionCustomizations.qll

@@ -22,9 +22,16 @@ module CodeInjection {
   abstract class Sink extends DataFlow::Node { }
 
   /**
+   * A sanitizer for "Code injection" vulnerabilities.
+   */
+  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.

ruby/ql/lib/codeql/ruby/security/CodeInjectionQuery.qll

@@ -20,9 +20,13 @@ class Configuration extends TaintTracking::Configuration {
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof SanitizerGuard or
-    guard instanceof StringConstCompare or
-    guard instanceof StringConstArrayInclusionCall
+  override predicate isSanitizer(DataFlow::Node node) {
+    node instanceof Sanitizer or
+    node instanceof StringConstCompareBarrier or
+    node instanceof StringConstArrayInclusionCallBarrier
+  }
+
+  deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof SanitizerGuard
   }
 }

ruby/ql/lib/codeql/ruby/security/CommandInjectionQuery.qll

@@ -23,10 +23,9 @@ class Configuration extends TaintTracking::Configuration {
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
-  override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
-
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof StringConstCompare or
-    guard instanceof StringConstArrayInclusionCall
+  override predicate isSanitizer(DataFlow::Node node) {
+    node instanceof Sanitizer or
+    node instanceof StringConstCompareBarrier or
+    node instanceof StringConstArrayInclusionCallBarrier
   }
 }

ruby/ql/lib/codeql/ruby/security/PathInjectionCustomizations.qll

@@ -24,9 +24,16 @@ module PathInjection {
   abstract class Sink extends DataFlow::Node { }
 
   /**
+   * A sanitizer for path injection vulnerabilities.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * DEPRECATED: Use `Sanitizer` instead.
+   *
    * A sanitizer guard for path 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.
@@ -43,12 +50,12 @@ module PathInjection {
   /**
    * A comparison with a constant string, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }
+  class StringConstCompareAsSanitizer extends Sanitizer, StringConstCompareBarrier { }
 
   /**
    * An inclusion check against an array of constant strings, considered as a
    * sanitizer-guard.
    */
-  class StringConstArrayInclusionCallAsSanitizerGuard extends SanitizerGuard,
-    StringConstArrayInclusionCall { }
+  class StringConstArrayInclusionCallAsSanitizer extends Sanitizer,
+    StringConstArrayInclusionCallBarrier { }
 }

ruby/ql/lib/codeql/ruby/security/PathInjectionQuery.qll

@@ -23,9 +23,11 @@ class Configuration extends TaintTracking::Configuration {
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof PathInjection::Sink }
 
-  override predicate isSanitizer(DataFlow::Node node) { node instanceof Path::PathSanitization }
+  override predicate isSanitizer(DataFlow::Node node) {
+    node instanceof Path::PathSanitization or node instanceof PathInjection::Sanitizer
+  }
 
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+  deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
     guard instanceof PathInjection::SanitizerGuard
   }
 }

ruby/ql/lib/codeql/ruby/security/ReflectedXSSQuery.qll

@@ -28,7 +28,7 @@ module ReflectedXss {
 
     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
     }
 

ruby/ql/lib/codeql/ruby/security/ServerSideRequestForgeryCustomizations.qll

@@ -32,9 +32,11 @@ module ServerSideRequestForgery {
   abstract class Sanitizer extends DataFlow::Node { }
 
   /**
+   * DEPRECATED: Use `Sanitizer` instead.
+   *
    * A sanitizer guard for "URL redirection" 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 for server side request forgery. */
   class RemoteFlowSourceAsSource extends Source {