Ruby: Case barrier guards

ruby/ql/lib/change-notes/2022-10-26-case-expression-barrier-guard.md

@@ -0,0 +1,4 @@
+---
+ category: minorAnalysis
+---
+ * String literals and arrays of string literals in case expression patterns are now recognised as barrier guards.

ruby/ql/lib/codeql/ruby/controlflow/BasicBlocks.qll

@@ -313,6 +313,58 @@ private module Cached {
   predicate immediatelyControls(ConditionBlock cb, BasicBlock succ, ConditionalSuccessor s) {
     succ = cb.getASuccessor(s) and
     forall(BasicBlock pred | pred = succ.getAPredecessor() and pred != cb | succ.dominates(pred))
+    or
+    // special case for `when` clauses in case expressions
+    whenClauseImmediatelyControls(cb, succ, s)
+  }
+
+  /**
+   * Holds if all control flow paths reaching `succ` first exit `cb` with
+   * successor type `s`, assuming that the last node of `cb` is a `when` clause.
+   *
+   * ```
+   *   case foo
+   *         |
+   *     +---+
+   *     |
+   *     v
+   *   when  "foo" ---> "bar" ----+
+   *         |  |        |        |
+   * no-match|  |match   |match   |no-match
+   *         |  v        |        |
+   *         |  foo <----+        |
+   *         |                    |
+   *   end <-+--------------------+
+   * ```
+   *
+   * The read of `foo` in the `then` block is not technically controlled by
+   * either `"foo"` or `"bar"` because it can be reached from either of them.
+   * However if we consider the `when` clause as a whole, then it is
+   * controlled because _at least one of the patterns_ must match.
+   *
+   * We determine this by finding a common successor `succ` of each pattern
+   * with the same successor type `s`. We check that all predecessors of
+   * `succ` are patterns in the clause, or are dominated by a pattern in the
+   * clause.
+   */
+  cached
+  private predicate whenClauseImmediatelyControls(
+    ConditionBlock cb, BasicBlock succ, ConditionalSuccessor s
+  ) {
+    exists(ExprNodes::WhenClauseCfgNode when |
+      cb = when.getBasicBlock() and
+      forall(ExprCfgNode pattern | pattern = when.getPattern(_) |
+        succ = pattern.getBasicBlock().getASuccessor(s) and
+        forall(BasicBlock pred |
+          pred = succ.getAPredecessor() and
+          pred != cb
+        |
+          pred = when.getPattern(_).getBasicBlock()
+          or
+          when.getPattern(_).getBasicBlock().dominates(pred)
+        )
+      )
+    )
   }
 
   cached

ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll

@@ -234,7 +234,7 @@ module ExprNodes {
     override predicate relevantChild(AstNode n) { none() }
   }
 
-  /** A control-flow node that wraps an `ArrayLiteral` AST expression. */
+  /** A control-flow node that wraps a `Literal` AST expression. */
   class LiteralCfgNode extends ExprCfgNode {
     override string getAPrimaryQlClass() { result = "LiteralCfgNode" }
 
@@ -866,6 +866,17 @@ module ExprNodes {
     final override RelationalOperation getExpr() { result = super.getExpr() }
   }
 
+  /** A control-flow node that wraps a `SplatExpr` AST expression. */
+  class SplatExprCfgNode extends ExprCfgNode {
+    override string getAPrimaryQlClass() { result = "SplatExprCfgNode" }
+
+    SplatExprCfgNode() { e instanceof SplatExpr }
+
+    final override SplatExpr getExpr() { result = super.getExpr() }
+
+    final ExprCfgNode getOperand() { result.getExpr() = e.(SplatExpr).getOperand() }
+  }
+
   /** A control-flow node that wraps an `ElementReference` AST expression. */
   class ElementReferenceCfgNode extends MethodCallCfgNode {
     override string getAPrimaryQlClass() { result = "ElementReferenceCfgNode" }

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

@@ -8,21 +8,23 @@ private import codeql.ruby.dataflow.SSA
 private import codeql.ruby.ast.internal.Constant
 private import codeql.ruby.InclusionTests
 
-private predicate stringConstCompare(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch) {
+private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch) {
   exists(CfgNodes::ExprNodes::ComparisonOperationCfgNode c |
-    c = g and
+    c = guard 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
+      c.getLeftOperand() = strLitNode and c.getRightOperand() = testedNode
       or
-      c.getLeftOperand() = e and c.getRightOperand() = strLitNode
+      c.getLeftOperand() = testedNode and c.getRightOperand() = strLitNode
     )
   )
+  or
+  stringConstCaseCompare(guard, testedNode, branch)
 }
 
 /**
@@ -72,10 +74,12 @@ deprecated class StringConstCompare extends DataFlow::BarrierGuard,
   }
 }
 
-private predicate stringConstArrayInclusionCall(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch) {
+private predicate stringConstArrayInclusionCall(
+  CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch
+) {
   exists(InclusionTest t |
-    t.asExpr() = g and
-    e = t.getContainedNode().asExpr() and
+    t.asExpr() = guard and
+    testedNode = t.getContainedNode().asExpr() and
     branch = t.getPolarity()
   |
     exists(ExprNodes::ArrayLiteralCfgNode arr |
@@ -132,3 +136,53 @@ deprecated class StringConstArrayInclusionCall extends DataFlow::BarrierGuard,
 
   override predicate checks(CfgNode expr, boolean branch) { expr = checkedNode and branch = true }
 }
+
+/**
+ * A validation of a value by comparing with a constant string via a `case`
+ * expression. For example:
+ *
+ * ```rb
+ * name = params[:user_name]
+ * case name
+ * when "alice"
+ *   User.find_by("username = #{name}")
+ * end
+ * ```
+ */
+private predicate stringConstCaseCompare(
+  CfgNodes::AstCfgNode guard, CfgNode testedNode, boolean branch
+) {
+  branch = true and
+  exists(CfgNodes::ExprNodes::CaseExprCfgNode case |
+    case.getValue() = testedNode and
+    exists(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode |
+      branchNode = case.getBranch(_) and
+      guard = branchNode.getPattern(_) and
+      // For simplicity, consider patterns that contain only string literals or arrays of string literals
+      forall(ExprCfgNode pattern | pattern = branchNode.getPattern(_) |
+        // when "foo"
+        // when "foo", "bar"
+        pattern instanceof ExprNodes::StringLiteralCfgNode
+        or
+        // array literals behave weirdly in the CFG so we need to drop down to the AST level for this bit
+        // specifically: `SplatExprCfgNode.getOperand()` does not return results for array literals
+        exists(CfgNodes::ExprNodes::SplatExprCfgNode splat | splat = pattern |
+          // when *["foo", "bar"]
+          exists(ArrayLiteral arr |
+            splat.getExpr().getOperand() = arr and
+            forall(Expr elem | elem = arr.getAnElement() | elem instanceof StringLiteral)
+          )
+          or
+          // when *some_var
+          // when *SOME_CONST
+          exists(ExprNodes::ArrayLiteralCfgNode arr |
+            isArrayConstant(splat.getOperand(), arr) and
+            forall(ExprCfgNode elem | elem = arr.getAnArgument() |
+              elem instanceof ExprNodes::StringLiteralCfgNode
+            )
+          )
+        )
+      )
+    )
+  )
+}

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

@@ -431,7 +431,7 @@ class ContentSet extends TContentSet {
  * 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);
+signature predicate guardChecksSig(CfgNodes::AstCfgNode g, CfgNode e, boolean branch);
 
 /**
  * Provides a set of barrier nodes for a guard that validates an expression.
@@ -441,21 +441,21 @@ signature predicate guardChecksSig(CfgNodes::ExprCfgNode g, CfgNode e, boolean b
  */
 module BarrierGuard<guardChecksSig/3 guardChecks> {
   pragma[nomagic]
-  private predicate guardChecksSsaDef(CfgNodes::ExprCfgNode g, boolean branch, Ssa::Definition def) {
+  private predicate guardChecksSsaDef(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def) {
     guardChecks(g, def.getARead(), branch)
   }
 
   pragma[nomagic]
   private predicate guardControlsSsaDef(
-    CfgNodes::ExprCfgNode g, boolean branch, Ssa::Definition def, Node n
+    CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, Node n
   ) {
     def.getARead() = n.asExpr() and
     guardControlsBlock(g, n.asExpr().getBasicBlock(), branch)
   }
 
   /** Gets a node that is safely guarded by the given guard check. */
   Node getABarrierNode() {
-    exists(CfgNodes::ExprCfgNode g, boolean branch, Ssa::Definition def |
+    exists(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def |
       guardChecksSsaDef(g, branch, def) and
       guardControlsSsaDef(g, branch, def, result)
     )
@@ -485,8 +485,8 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
 }
 
 /** 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 |
+private predicate guardControlsBlock(CfgNodes::AstCfgNode guard, BasicBlock bb, boolean branch) {
+  exists(ConditionBlock conditionBlock, SuccessorTypes::ConditionalSuccessor s |
     guard = conditionBlock.getLastNode() and
     s.getValue() = branch and
     conditionBlock.controls(bb, s)

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

@@ -127,7 +127,7 @@ module PolynomialReDoS {
     override DataFlow::Node getHighlight() { result = matchNode }
   }
 
-  private predicate lengthGuard(CfgNodes::ExprCfgNode g, CfgNode node, boolean branch) {
+  private predicate lengthGuard(CfgNodes::AstCfgNode 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

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

@@ -20,3 +20,12 @@ newStyleBarrierGuards
 | barrier-guards.rb:71:5:71:7 | foo |
 | barrier-guards.rb:83:5:83:7 | foo |
 | barrier-guards.rb:91:5:91:7 | foo |
+| barrier-guards.rb:108:5:108:7 | foo |
+| barrier-guards.rb:115:5:115:7 | foo |
+| barrier-guards.rb:117:5:117:7 | foo |
+| barrier-guards.rb:122:5:122:7 | foo |
+| barrier-guards.rb:124:5:124:7 | foo |
+| barrier-guards.rb:131:5:131:7 | foo |
+| barrier-guards.rb:136:5:136:7 | foo |
+| barrier-guards.rb:141:5:141:7 | foo |
+| barrier-guards.rb:146:5:146:7 | foo |

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

@@ -102,3 +102,72 @@
 else
     foo
 end
+
+case foo
+when "foo"
+    foo
+else
+    foo
+end
+
+case foo
+when "foo"
+    foo
+when "bar"
+    foo
+end
+
+case foo
+when "foo", "bar"
+    foo
+when "baz", "quux"
+    foo
+else
+    foo
+end
+
+case foo
+when *["foo", "bar"]
+    foo
+end
+
+case foo
+when *%w[foo bar]
+    foo
+end
+
+case foo
+when *FOO
+    foo
+end
+
+case foo
+when *foos
+    foo
+end
+
+x = nil
+
+case foo
+when *["foo", x]
+    foo
+end
+
+case foo
+when "foo", x
+    foo
+end
+
+foo_and_x = ["foo", x]
+
+case foo
+when *foo_and_x
+    foo
+end
+
+FOO_AND_X = ["foo", x]
+
+case foo
+when *FOO_AND_X
+    foo
+end

ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected

@@ -10,11 +10,13 @@ edges
 | CommandInjection.rb:6:15:6:26 | ...[...] :  | CommandInjection.rb:34:39:34:51 | "grep #{...}" |
 | CommandInjection.rb:46:15:46:20 | call to params :  | CommandInjection.rb:46:15:46:26 | ...[...] :  |
 | CommandInjection.rb:46:15:46:26 | ...[...] :  | CommandInjection.rb:50:24:50:36 | "echo #{...}" |
-| CommandInjection.rb:64:18:64:23 | number :  | CommandInjection.rb:65:14:65:29 | "echo #{...}" |
-| CommandInjection.rb:72:23:72:33 | blah_number :  | CommandInjection.rb:73:14:73:34 | "echo #{...}" |
-| CommandInjection.rb:81:20:81:25 | **args :  | CommandInjection.rb:82:22:82:25 | args :  |
-| CommandInjection.rb:82:22:82:25 | args :  | CommandInjection.rb:82:22:82:37 | ...[...] :  |
-| CommandInjection.rb:82:22:82:37 | ...[...] :  | CommandInjection.rb:82:14:82:39 | "echo #{...}" |
+| CommandInjection.rb:54:13:54:18 | call to params :  | CommandInjection.rb:54:13:54:24 | ...[...] :  |
+| CommandInjection.rb:54:13:54:24 | ...[...] :  | CommandInjection.rb:59:14:59:16 | cmd |
+| CommandInjection.rb:73:18:73:23 | number :  | CommandInjection.rb:74:14:74:29 | "echo #{...}" |
+| CommandInjection.rb:81:23:81:33 | blah_number :  | CommandInjection.rb:82:14:82:34 | "echo #{...}" |
+| CommandInjection.rb:90:20:90:25 | **args :  | CommandInjection.rb:91:22:91:25 | args :  |
+| CommandInjection.rb:91:22:91:25 | args :  | CommandInjection.rb:91:22:91:37 | ...[...] :  |
+| CommandInjection.rb:91:22:91:37 | ...[...] :  | CommandInjection.rb:91:14:91:39 | "echo #{...}" |
 nodes
 | CommandInjection.rb:6:15:6:20 | call to params :  | semmle.label | call to params :  |
 | CommandInjection.rb:6:15:6:26 | ...[...] :  | semmle.label | ...[...] :  |
@@ -29,14 +31,17 @@ nodes
 | CommandInjection.rb:46:15:46:20 | call to params :  | semmle.label | call to params :  |
 | CommandInjection.rb:46:15:46:26 | ...[...] :  | semmle.label | ...[...] :  |
 | CommandInjection.rb:50:24:50:36 | "echo #{...}" | semmle.label | "echo #{...}" |
-| CommandInjection.rb:64:18:64:23 | number :  | semmle.label | number :  |
-| CommandInjection.rb:65:14:65:29 | "echo #{...}" | semmle.label | "echo #{...}" |
-| CommandInjection.rb:72:23:72:33 | blah_number :  | semmle.label | blah_number :  |
-| CommandInjection.rb:73:14:73:34 | "echo #{...}" | semmle.label | "echo #{...}" |
-| CommandInjection.rb:81:20:81:25 | **args :  | semmle.label | **args :  |
-| CommandInjection.rb:82:14:82:39 | "echo #{...}" | semmle.label | "echo #{...}" |
-| CommandInjection.rb:82:22:82:25 | args :  | semmle.label | args :  |
-| CommandInjection.rb:82:22:82:37 | ...[...] :  | semmle.label | ...[...] :  |
+| CommandInjection.rb:54:13:54:18 | call to params :  | semmle.label | call to params :  |
+| CommandInjection.rb:54:13:54:24 | ...[...] :  | semmle.label | ...[...] :  |
+| CommandInjection.rb:59:14:59:16 | cmd | semmle.label | cmd |
+| CommandInjection.rb:73:18:73:23 | number :  | semmle.label | number :  |
+| CommandInjection.rb:74:14:74:29 | "echo #{...}" | semmle.label | "echo #{...}" |
+| CommandInjection.rb:81:23:81:33 | blah_number :  | semmle.label | blah_number :  |
+| CommandInjection.rb:82:14:82:34 | "echo #{...}" | semmle.label | "echo #{...}" |
+| CommandInjection.rb:90:20:90:25 | **args :  | semmle.label | **args :  |
+| CommandInjection.rb:91:14:91:39 | "echo #{...}" | semmle.label | "echo #{...}" |
+| CommandInjection.rb:91:22:91:25 | args :  | semmle.label | args :  |
+| CommandInjection.rb:91:22:91:37 | ...[...] :  | semmle.label | ...[...] :  |
 subpaths
 #select
 | CommandInjection.rb:7:10:7:15 | #{...} | CommandInjection.rb:6:15:6:20 | call to params :  | CommandInjection.rb:7:10:7:15 | #{...} | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value |
@@ -48,6 +53,7 @@ subpaths
 | CommandInjection.rb:33:24:33:36 | "echo #{...}" | CommandInjection.rb:6:15:6:20 | call to params :  | CommandInjection.rb:33:24:33:36 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value |
 | CommandInjection.rb:34:39:34:51 | "grep #{...}" | CommandInjection.rb:6:15:6:20 | call to params :  | CommandInjection.rb:34:39:34:51 | "grep #{...}" | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value |
 | CommandInjection.rb:50:24:50:36 | "echo #{...}" | CommandInjection.rb:46:15:46:20 | call to params :  | CommandInjection.rb:50:24:50:36 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:46:15:46:20 | call to params | user-provided value |
-| CommandInjection.rb:65:14:65:29 | "echo #{...}" | CommandInjection.rb:64:18:64:23 | number :  | CommandInjection.rb:65:14:65:29 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:64:18:64:23 | number | user-provided value |
-| CommandInjection.rb:73:14:73:34 | "echo #{...}" | CommandInjection.rb:72:23:72:33 | blah_number :  | CommandInjection.rb:73:14:73:34 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:72:23:72:33 | blah_number | user-provided value |
-| CommandInjection.rb:82:14:82:39 | "echo #{...}" | CommandInjection.rb:81:20:81:25 | **args :  | CommandInjection.rb:82:14:82:39 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:81:20:81:25 | **args | user-provided value |
+| CommandInjection.rb:59:14:59:16 | cmd | CommandInjection.rb:54:13:54:18 | call to params :  | CommandInjection.rb:59:14:59:16 | cmd | This command depends on a $@. | CommandInjection.rb:54:13:54:18 | call to params | user-provided value |
+| CommandInjection.rb:74:14:74:29 | "echo #{...}" | CommandInjection.rb:73:18:73:23 | number :  | CommandInjection.rb:74:14:74:29 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:73:18:73:23 | number | user-provided value |
+| CommandInjection.rb:82:14:82:34 | "echo #{...}" | CommandInjection.rb:81:23:81:33 | blah_number :  | CommandInjection.rb:82:14:82:34 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:81:23:81:33 | blah_number | user-provided value |
+| CommandInjection.rb:91:14:91:39 | "echo #{...}" | CommandInjection.rb:90:20:90:25 | **args :  | CommandInjection.rb:91:14:91:39 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:90:20:90:25 | **args | user-provided value |