Merge branch 'main' into patch-1

ruby/ql/lib/change-notes/2022-06-21-barrierguard-deprecation.md

@@ -0,0 +1,4 @@
+---
+category: deprecated
+---
+* The `BarrierGuard` class has been deprecated. Such barriers and sanitizers can now instead be created using the new `BarrierGuard` parameterized module.

ruby/ql/lib/change-notes/2022-06-22-sensitive-common-words.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Improved modeling of sensitive data sources, so common words like `certain` and `secretary` are no longer considered a certificate and a secret (respectively).

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,6 +80,18 @@ 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:
@@ -56,8 +106,29 @@ 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,
+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:
+ *
+ * ```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.
+ */
+deprecated class StringConstArrayInclusionCall extends DataFlow::BarrierGuard,
   CfgNodes::ExprNodes::MethodCallCfgNode {
   private CfgNode checkedNode;
 

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

@@ -90,14 +90,20 @@ abstract class Configuration extends string {
   /** Holds if data flow out of `node` is prohibited. */
   predicate isBarrierOut(Node node) { none() }
 
-  /** Holds if data flow through nodes guarded by `guard` is prohibited. */
-  predicate isBarrierGuard(BarrierGuard guard) { none() }
+  /**
+   * DEPRECATED: Use `isBarrier` and `BarrierGuard` module instead.
+   *
+   * Holds if data flow through nodes guarded by `guard` is prohibited.
+   */
+  deprecated predicate isBarrierGuard(BarrierGuard guard) { none() }
 
   /**
+   * DEPRECATED: Use `isBarrier` and `BarrierGuard` module instead.
+   *
    * Holds if data flow through nodes guarded by `guard` is prohibited when
    * the flow state is `state`
    */
-  predicate isBarrierGuard(BarrierGuard guard, FlowState state) { none() }
+  deprecated predicate isBarrierGuard(BarrierGuard guard, FlowState state) { none() }
 
   /**
    * Holds if data may flow from `node1` to `node2` in addition to the normal data-flow steps.
@@ -335,6 +341,29 @@ private predicate outBarrier(NodeEx node, Configuration config) {
   )
 }
 
+/** A bridge class to access the deprecated `isBarrierGuard`. */
+private class BarrierGuardGuardedNodeBridge extends Unit {
+  abstract predicate guardedNode(Node n, Configuration config);
+
+  abstract predicate guardedNode(Node n, FlowState state, Configuration config);
+}
+
+private class BarrierGuardGuardedNode extends BarrierGuardGuardedNodeBridge {
+  deprecated override predicate guardedNode(Node n, Configuration config) {
+    exists(BarrierGuard g |
+      config.isBarrierGuard(g) and
+      n = g.getAGuardedNode()
+    )
+  }
+
+  deprecated override predicate guardedNode(Node n, FlowState state, Configuration config) {
+    exists(BarrierGuard g |
+      config.isBarrierGuard(g, state) and
+      n = g.getAGuardedNode()
+    )
+  }
+}
+
 pragma[nomagic]
 private predicate fullBarrier(NodeEx node, Configuration config) {
   exists(Node n | node.asNode() = n |
@@ -348,10 +377,7 @@ private predicate fullBarrier(NodeEx node, Configuration config) {
     not config.isSink(n) and
     not config.isSink(n, _)
     or
-    exists(BarrierGuard g |
-      config.isBarrierGuard(g) and
-      n = g.getAGuardedNode()
-    )
+    any(BarrierGuardGuardedNodeBridge b).guardedNode(n, config)
   )
 }
 
@@ -360,10 +386,7 @@ private predicate stateBarrier(NodeEx node, FlowState state, Configuration confi
   exists(Node n | node.asNode() = n |
     config.isBarrier(n, state)
     or
-    exists(BarrierGuard g |
-      config.isBarrierGuard(g, state) and
-      n = g.getAGuardedNode()
-    )
+    any(BarrierGuardGuardedNodeBridge b).guardedNode(n, state, config)
   )
 }
 

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

@@ -90,14 +90,20 @@ abstract class Configuration extends string {
   /** Holds if data flow out of `node` is prohibited. */
   predicate isBarrierOut(Node node) { none() }
 
-  /** Holds if data flow through nodes guarded by `guard` is prohibited. */
-  predicate isBarrierGuard(BarrierGuard guard) { none() }
+  /**
+   * DEPRECATED: Use `isBarrier` and `BarrierGuard` module instead.
+   *
+   * Holds if data flow through nodes guarded by `guard` is prohibited.
+   */
+  deprecated predicate isBarrierGuard(BarrierGuard guard) { none() }
 
   /**
+   * DEPRECATED: Use `isBarrier` and `BarrierGuard` module instead.
+   *
    * Holds if data flow through nodes guarded by `guard` is prohibited when
    * the flow state is `state`
    */
-  predicate isBarrierGuard(BarrierGuard guard, FlowState state) { none() }
+  deprecated predicate isBarrierGuard(BarrierGuard guard, FlowState state) { none() }
 
   /**
    * Holds if data may flow from `node1` to `node2` in addition to the normal data-flow steps.
@@ -335,6 +341,29 @@ private predicate outBarrier(NodeEx node, Configuration config) {
   )
 }
 
+/** A bridge class to access the deprecated `isBarrierGuard`. */
+private class BarrierGuardGuardedNodeBridge extends Unit {
+  abstract predicate guardedNode(Node n, Configuration config);
+
+  abstract predicate guardedNode(Node n, FlowState state, Configuration config);
+}
+
+private class BarrierGuardGuardedNode extends BarrierGuardGuardedNodeBridge {
+  deprecated override predicate guardedNode(Node n, Configuration config) {
+    exists(BarrierGuard g |
+      config.isBarrierGuard(g) and
+      n = g.getAGuardedNode()
+    )
+  }
+
+  deprecated override predicate guardedNode(Node n, FlowState state, Configuration config) {
+    exists(BarrierGuard g |
+      config.isBarrierGuard(g, state) and
+      n = g.getAGuardedNode()
+    )
+  }
+}
+
 pragma[nomagic]
 private predicate fullBarrier(NodeEx node, Configuration config) {
   exists(Node n | node.asNode() = n |
@@ -348,10 +377,7 @@ private predicate fullBarrier(NodeEx node, Configuration config) {
     not config.isSink(n) and
     not config.isSink(n, _)
     or
-    exists(BarrierGuard g |
-      config.isBarrierGuard(g) and
-      n = g.getAGuardedNode()
-    )
+    any(BarrierGuardGuardedNodeBridge b).guardedNode(n, config)
   )
 }
 
@@ -360,10 +386,7 @@ private predicate stateBarrier(NodeEx node, FlowState state, Configuration confi
   exists(Node n | node.asNode() = n |
     config.isBarrier(n, state)
     or
-    exists(BarrierGuard g |
-      config.isBarrierGuard(g, state) and
-      n = g.getAGuardedNode()
-    )
+    any(BarrierGuardGuardedNodeBridge b).guardedNode(n, state, config)
   )
 }
 

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

@@ -90,14 +90,20 @@ abstract class Configuration extends string {
   /** Holds if data flow out of `node` is prohibited. */
   predicate isBarrierOut(Node node) { none() }
 
-  /** Holds if data flow through nodes guarded by `guard` is prohibited. */
-  predicate isBarrierGuard(BarrierGuard guard) { none() }
+  /**
+   * DEPRECATED: Use `isBarrier` and `BarrierGuard` module instead.
+   *
+   * Holds if data flow through nodes guarded by `guard` is prohibited.
+   */
+  deprecated predicate isBarrierGuard(BarrierGuard guard) { none() }
 
   /**
+   * DEPRECATED: Use `isBarrier` and `BarrierGuard` module instead.
+   *
    * Holds if data flow through nodes guarded by `guard` is prohibited when
    * the flow state is `state`
    */
-  predicate isBarrierGuard(BarrierGuard guard, FlowState state) { none() }
+  deprecated predicate isBarrierGuard(BarrierGuard guard, FlowState state) { none() }
 
   /**
    * Holds if data may flow from `node1` to `node2` in addition to the normal data-flow steps.
@@ -335,6 +341,29 @@ private predicate outBarrier(NodeEx node, Configuration config) {
   )
 }
 
+/** A bridge class to access the deprecated `isBarrierGuard`. */
+private class BarrierGuardGuardedNodeBridge extends Unit {
+  abstract predicate guardedNode(Node n, Configuration config);
+
+  abstract predicate guardedNode(Node n, FlowState state, Configuration config);
+}
+
+private class BarrierGuardGuardedNode extends BarrierGuardGuardedNodeBridge {
+  deprecated override predicate guardedNode(Node n, Configuration config) {
+    exists(BarrierGuard g |
+      config.isBarrierGuard(g) and
+      n = g.getAGuardedNode()
+    )
+  }
+
+  deprecated override predicate guardedNode(Node n, FlowState state, Configuration config) {
+    exists(BarrierGuard g |
+      config.isBarrierGuard(g, state) and
+      n = g.getAGuardedNode()
+    )
+  }
+}
+
 pragma[nomagic]
 private predicate fullBarrier(NodeEx node, Configuration config) {
   exists(Node n | node.asNode() = n |
@@ -348,10 +377,7 @@ private predicate fullBarrier(NodeEx node, Configuration config) {
     not config.isSink(n) and
     not config.isSink(n, _)
     or
-    exists(BarrierGuard g |
-      config.isBarrierGuard(g) and
-      n = g.getAGuardedNode()
-    )
+    any(BarrierGuardGuardedNodeBridge b).guardedNode(n, config)
   )
 }
 
@@ -360,10 +386,7 @@ private predicate stateBarrier(NodeEx node, FlowState state, Configuration confi
   exists(Node n | node.asNode() = n |
     config.isBarrier(n, state)
     or
-    exists(BarrierGuard g |
-      config.isBarrierGuard(g, state) and
-      n = g.getAGuardedNode()
-    )
+    any(BarrierGuardGuardedNodeBridge b).guardedNode(n, state, config)
   )
 }
 

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/FlowSummaryImpl.qll

@@ -1116,13 +1116,13 @@ module Private {
       preservesValue = false and result = "taint"
     }
 
-    private string renderGenerated(RelevantSummarizedCallable c) {
-      if c.(SummarizedCallable).isAutoGenerated() then result = "generated:" else result = ""
+    private string renderProvenance(RelevantSummarizedCallable c) {
+      if c.(SummarizedCallable).isAutoGenerated() then result = "generated" else result = "manual"
     }
 
     /**
      * A query predicate for outputting flow summaries in semi-colon separated format in QL tests.
-     * The syntax is: "namespace;type;overrides;name;signature;ext;inputspec;outputspec;(generated:)?kind",
+     * The syntax is: "namespace;type;overrides;name;signature;ext;inputspec;outputspec;kind;provenance"",
      * ext is hardcoded to empty.
      */
     query predicate summary(string csv) {
@@ -1133,7 +1133,7 @@ module Private {
         c.relevantSummary(input, output, preservesValue) and
         csv =
           c.getCallableCsv() + getComponentStackCsv(input) + ";" + getComponentStackCsv(output) +
-            ";" + renderGenerated(c) + renderKind(preservesValue)
+            ";" + renderKind(preservesValue) + ";" + renderProvenance(c)
       )
     }
   }

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/dataflow/internal/tainttracking1/TaintTrackingImpl.qll

@@ -116,20 +116,30 @@ abstract class Configuration extends DataFlow::Configuration {
 
   final override predicate isBarrierOut(DataFlow::Node node) { this.isSanitizerOut(node) }
 
-  /** Holds if taint propagation through nodes guarded by `guard` is prohibited. */
-  predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
+  /**
+   * DEPRECATED: Use `isSanitizer` and `BarrierGuard` module instead.
+   *
+   * Holds if taint propagation through nodes guarded by `guard` is prohibited.
+   */
+  deprecated predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
 
-  final override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
-    this.isSanitizerGuard(guard) or defaultTaintSanitizerGuard(guard)
+  deprecated final override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
+    this.isSanitizerGuard(guard)
   }
 
   /**
+   * DEPRECATED: Use `isSanitizer` and `BarrierGuard` module instead.
+   *
    * Holds if taint propagation through nodes guarded by `guard` is prohibited
    * when the flow state is `state`.
    */
-  predicate isSanitizerGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) { none() }
+  deprecated predicate isSanitizerGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) {
+    none()
+  }
 
-  final override predicate isBarrierGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) {
+  deprecated final override predicate isBarrierGuard(
+    DataFlow::BarrierGuard guard, DataFlow::FlowState state
+  ) {
     this.isSanitizerGuard(guard, state)
   }
 

ruby/ql/lib/codeql/ruby/dataflow/internal/tainttrackingforlibraries/TaintTrackingImpl.qll

@@ -116,20 +116,30 @@ abstract class Configuration extends DataFlow::Configuration {
 
   final override predicate isBarrierOut(DataFlow::Node node) { this.isSanitizerOut(node) }
 
-  /** Holds if taint propagation through nodes guarded by `guard` is prohibited. */
-  predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
+  /**
+   * DEPRECATED: Use `isSanitizer` and `BarrierGuard` module instead.
+   *
+   * Holds if taint propagation through nodes guarded by `guard` is prohibited.
+   */
+  deprecated predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { none() }
 
-  final override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
-    this.isSanitizerGuard(guard) or defaultTaintSanitizerGuard(guard)
+  deprecated final override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
+    this.isSanitizerGuard(guard)
   }
 
   /**
+   * DEPRECATED: Use `isSanitizer` and `BarrierGuard` module instead.
+   *
    * Holds if taint propagation through nodes guarded by `guard` is prohibited
    * when the flow state is `state`.
    */
-  predicate isSanitizerGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) { none() }
+  deprecated predicate isSanitizerGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) {
+    none()
+  }
 
-  final override predicate isBarrierGuard(DataFlow::BarrierGuard guard, DataFlow::FlowState state) {
+  deprecated final override predicate isBarrierGuard(
+    DataFlow::BarrierGuard guard, DataFlow::FlowState state
+  ) {
     this.isSanitizerGuard(guard, state)
   }
 

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 {

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

@@ -22,11 +22,13 @@ 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 isSanitizer(DataFlow::Node node) {
+    node instanceof Sanitizer or
+    node instanceof StringConstCompareBarrier or
+    node instanceof StringConstArrayInclusionCallBarrier
+  }
 
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof SanitizerGuard or
-    guard instanceof StringConstCompare or
-    guard instanceof StringConstArrayInclusionCall
+  deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof SanitizerGuard
   }
 }

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

@@ -30,7 +30,7 @@ module StoredXss {
       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/UrlRedirectCustomizations.qll

@@ -34,9 +34,11 @@ module UrlRedirect {
   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 { }
 
   /**
    * Additional taint steps for "URL redirection" vulnerabilities.
@@ -77,7 +79,7 @@ module UrlRedirect {
   /**
    * A comparison with a constant string, considered as a sanitizer-guard.
    */
-  class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }
+  class StringConstCompareAsSanitizer extends Sanitizer, StringConstCompareBarrier { }
 
   /**
    * Some methods will propagate taint to their return values.

ruby/ql/lib/codeql/ruby/security/UrlRedirectQuery.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
   }
 

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

@@ -36,9 +36,11 @@ private module Shared {
   abstract class Sanitizer extends DataFlow::Node { }
 
   /**
+   * DEPRECATED: Use `Sanitizer` instead.
+   *
    * A sanitizer guard for "server-side cross-site scripting" vulnerabilities.
    */
-  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
+  abstract deprecated class SanitizerGuard extends DataFlow::BarrierGuard { }
 
   private class ErbOutputMethodCallArgumentNode extends DataFlow::Node {
     private MethodCall call;
@@ -93,13 +95,13 @@ private module Shared {
   /**
    * 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 { }
 
   /**
    * A `VariableWriteAccessCfgNode` that is not succeeded (locally) by another
@@ -274,8 +276,12 @@ module ReflectedXss {
   /** A sanitizer for stored XSS vulnerabilities. */
   class Sanitizer = Shared::Sanitizer;
 
-  /** A sanitizer guard for stored XSS vulnerabilities. */
-  class SanitizerGuard = Shared::SanitizerGuard;
+  /**
+   * DEPRECATED: Use `Sanitizer` instead.
+   *
+   * A sanitizer guard for stored XSS vulnerabilities.
+   */
+  deprecated class SanitizerGuard = Shared::SanitizerGuard;
 
   /**
    * An additional step that is preserves dataflow in the context of reflected XSS.
@@ -329,8 +335,12 @@ module StoredXss {
   /** A sanitizer for stored XSS vulnerabilities. */
   class Sanitizer = Shared::Sanitizer;
 
-  /** A sanitizer guard for stored XSS vulnerabilities. */
-  class SanitizerGuard = Shared::SanitizerGuard;
+  /**
+   * DEPRECATED: Use `Sanitizer` instead.
+   *
+   * A sanitizer guard for stored XSS vulnerabilities.
+   */
+  deprecated class SanitizerGuard = Shared::SanitizerGuard;
 
   /**
    * An additional step that preserves dataflow in the context of stored XSS.

ruby/ql/lib/codeql/ruby/security/internal/SensitiveDataHeuristics.qll

@@ -50,7 +50,7 @@ module HeuristicNames {
    * Gets a regular expression that identifies strings that may indicate the presence of secret
    * or trusted data.
    */
-  string maybeSecret() { result = "(?is).*((?<!is)secret|(?<!un|is)trusted).*" }
+  string maybeSecret() { result = "(?is).*((?<!is|is_)secret|(?<!un|un_|is|is_)trusted).*" }
 
   /**
    * Gets a regular expression that identifies strings that may indicate the presence of
@@ -96,10 +96,14 @@ module HeuristicNames {
    * Gets a regular expression that identifies strings that may indicate the presence of data
    * that is hashed or encrypted, and hence rendered non-sensitive, or contains special characters
    * suggesting nouns within the string do not represent the meaning of the whole string (e.g. a URL or a SQL query).
+   *
+   * We also filter out common words like `certain` and `concert`, since otherwise these could
+   * be matched by the certificate regular expressions. Same for `accountable` (account), or
+   * `secretarial` (secret).
    */
   string notSensitiveRegexp() {
     result =
-      "(?is).*([^\\w$.-]|redact|censor|obfuscate|hash|md5|sha|random|((?<!un)(en))?(crypt|code)).*"
+      "(?is).*([^\\w$.-]|redact|censor|obfuscate|hash|md5|sha|random|((?<!un)(en))?(crypt|code)|certain|concert|secretar|accountant|accountab).*"
   }
 
   /**

ruby/ql/lib/codeql/ruby/security/performance/PolynomialReDoSCustomizations.qll

@@ -34,10 +34,12 @@ module PolynomialReDoS {
   abstract class Sanitizer extends DataFlow::Node { }
 
   /**
+   * DEPRECATED: Use `Sanitizer` instead.
+   *
    * A sanitizer guard for polynomial regular expression denial of service
    * 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.
@@ -108,23 +110,21 @@ module PolynomialReDoS {
     override DataFlow::Node getHighlight() { result = matchNode }
   }
 
+  private predicate lengthGuard(CfgNodes::ExprCfgNode g, CfgNode node, boolean branch) {
+    exists(DataFlow::Node input, DataFlow::CallNode length, DataFlow::ExprNode operand |
+      length.asExpr().getExpr().(AST::MethodCall).getMethodName() = "length" and
+      length.getReceiver() = input and
+      length.flowsTo(operand) and
+      operand.getExprNode() = g.(CfgNodes::ExprNodes::RelationalOperationCfgNode).getAnOperand() and
+      node = input.asExpr() and
+      branch = true
+    )
+  }
+
   /**
    * A check on the length of a string, seen as a sanitizer guard.
    */
-  class LengthGuard extends SanitizerGuard, CfgNodes::ExprNodes::RelationalOperationCfgNode {
-    private DataFlow::Node input;
-
-    LengthGuard() {
-      exists(DataFlow::CallNode length, DataFlow::ExprNode operand |
-        length.asExpr().getExpr().(AST::MethodCall).getMethodName() = "length" and
-        length.getReceiver() = input and
-        length.flowsTo(operand) and
-        operand.getExprNode() = this.getAnOperand()
-      )
-    }
-
-    override predicate checks(CfgNode node, boolean branch) {
-      node = input.asExpr() and branch = true
-    }
+  class LengthGuard extends Sanitizer {
+    LengthGuard() { this = DataFlow::BarrierGuard<lengthGuard/3>::getABarrierNode() }
   }
 }

ruby/ql/lib/codeql/ruby/security/performance/PolynomialReDoSQuery.qll

@@ -30,7 +30,7 @@ module PolynomialReDoS {
 
     override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
 
-    override predicate isSanitizerGuard(DataFlow::BarrierGuard node) {
+    deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard node) {
       node instanceof SanitizerGuard
     }
   }

ruby/ql/lib/codeql/ruby/security/performance/RegExpInjectionCustomizations.qll

@@ -28,9 +28,11 @@ module RegExpInjection {
   abstract class Sink extends DataFlow::Node { }
 
   /**
+   * DEPRECATED: Use `Sanitizer` instead.
+   *
    * A sanitizer guard for regexp injection vulnerabilities.
    */
-  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
+  abstract deprecated class SanitizerGuard extends DataFlow::BarrierGuard { }
 
   /**
    * A data flow sanitized for regexp injection vulnerabilities.
@@ -64,14 +66,14 @@ module RegExpInjection {
   /**
    * 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 { }
 
   /**
    * A call to `Regexp.escape` (or its alias, `Regexp.quote`), considered as a

ruby/ql/lib/codeql/ruby/security/performance/RegExpInjectionQuery.qll

@@ -20,7 +20,7 @@ class Configuration extends TaintTracking::Configuration {
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof RegExpInjection::Sink }
 
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+  deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
     guard instanceof RegExpInjection::SanitizerGuard
   }
 

ruby/ql/src/queries/security/cwe-078/KernelOpen.ql

@@ -59,9 +59,9 @@ class Configuration extends TaintTracking::Configuration {
     exists(IOReadCall c | c.getArgument(0) = sink)
   }
 
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof StringConstCompare or
-    guard instanceof StringConstArrayInclusionCall
+  override predicate isSanitizer(DataFlow::Node node) {
+    node instanceof StringConstCompareBarrier or
+    node instanceof StringConstArrayInclusionCallBarrier
   }
 }
 

ruby/ql/src/queries/security/cwe-089/SqlInjection.ql

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

ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected

@@ -1,7 +0,0 @@
-| barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | barrier-guards.rb:3:4:3:6 | foo | true |
-| barrier-guards.rb:9:4:9:24 | call to include? | barrier-guards.rb:10:5:10:7 | foo | barrier-guards.rb:9:21:9:23 | foo | true |
-| barrier-guards.rb:15:4:15:15 | ... != ... | barrier-guards.rb:18:5:18:7 | foo | barrier-guards.rb:15:4:15:6 | foo | false |
-| barrier-guards.rb:21:8:21:19 | ... == ... | barrier-guards.rb:24:5:24:7 | foo | barrier-guards.rb:21:8:21:10 | foo | true |
-| barrier-guards.rb:27:8:27:19 | ... != ... | barrier-guards.rb:28:5:28:7 | foo | barrier-guards.rb:27:8:27:10 | foo | false |
-| barrier-guards.rb:37:4:37:20 | call to include? | barrier-guards.rb:38:5:38:7 | foo | barrier-guards.rb:37:17:37:19 | foo | true |
-| barrier-guards.rb:43:4:43:15 | ... == ... | barrier-guards.rb:45:9:45:11 | foo | barrier-guards.rb:43:4:43:6 | foo | true |

ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql

@@ -1,7 +0,0 @@
-import codeql.ruby.dataflow.internal.DataFlowPublic
-import codeql.ruby.dataflow.BarrierGuards
-import codeql.ruby.controlflow.CfgNodes
-
-from BarrierGuard g, boolean branch, ExprCfgNode expr
-where g.checks(expr, branch)
-select g, g.getAGuardedNode(), expr, branch

ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb

@@ -1,64 +0,0 @@
-foo = "foo"
-
-if foo == "foo"
-    foo
-else
-    foo
-end
-
-if ["foo"].include?(foo)
-    foo
-else
-    foo
-end
-
-if foo != "foo"
-    foo
-else
-    foo
-end
-
-unless foo == "foo"
-    foo
-else
-    foo
-end
-
-unless foo != "foo"
-    foo
-else
-    foo
-end
-
-foo
-
-FOO = ["foo"]
-
-if FOO.include?(foo)
-    foo
-else
-    foo
-end
-
-if foo == "foo"
-    capture {
-        foo # guarded
-    }
-end
-
-if foo == "foo"
-    capture {
-        foo = "bar"
-        foo # not guarded
-    }
-end
-
-if foo == "foo"
-    my_lambda = -> () {
-        foo # not guarded
-    }
-
-    foo = "bar"
-
-    my_lambda()
-end