Merge pull request github#11114 from hmac/case-barrier-guard-3

Ruby: Add case string comparison barrier guard

Author: hmac

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/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" }
 
@@ -432,8 +432,36 @@ module ExprNodes {
     final ExprCfgNode getBody() { e.hasCfgChild(e.getBody(), this, result) }
   }
 
-  private class WhenClauseChildMapping extends NonExprChildMapping, WhenClause {
-    override predicate relevantChild(AstNode e) { e = [this.getBody(), this.getAPattern()] }
+  // `when` clauses need special treatment, since they are neither pre-order
+  // nor post-order
+  private class WhenClauseChildMapping extends WhenClause {
+    predicate patternReachesBasicBlock(int i, CfgNode cfnPattern, BasicBlock bb) {
+      exists(Expr pattern |
+        pattern = this.getPattern(i) and
+        cfnPattern.getNode() = pattern and
+        bb.getANode() = cfnPattern
+      )
+      or
+      exists(BasicBlock mid |
+        this.patternReachesBasicBlock(i, cfnPattern, mid) and
+        bb = mid.getASuccessor() and
+        not mid.getANode().getNode() = this
+      )
+    }
+
+    predicate bodyReachesBasicBlock(CfgNode cfnBody, BasicBlock bb) {
+      exists(Stmt body |
+        body = this.getBody() and
+        cfnBody.getNode() = body and
+        bb.getANode() = cfnBody
+      )
+      or
+      exists(BasicBlock mid |
+        this.bodyReachesBasicBlock(cfnBody, mid) and
+        bb = mid.getAPredecessor() and
+        not mid.getANode().getNode() = this
+      )
+    }
   }
 
   /** A control-flow node that wraps a `WhenClause` AST expression. */
@@ -443,10 +471,16 @@ module ExprNodes {
     override WhenClauseChildMapping e;
 
     /** Gets the body of this `when`-clause. */
-    final ExprCfgNode getBody() { e.hasCfgChild(e.getBody(), this, result) }
+    final ExprCfgNode getBody() {
+      result.getNode() = desugar(e.getBody()) and
+      e.bodyReachesBasicBlock(result, this.getBasicBlock())
+    }
 
     /** Gets the `i`th pattern this `when`-clause. */
-    final ExprCfgNode getPattern(int i) { e.hasCfgChild(e.getPattern(i), this, result) }
+    final ExprCfgNode getPattern(int i) {
+      result.getNode() = desugar(e.getPattern(i)) and
+      e.patternReachesBasicBlock(i, result, this.getBasicBlock())
+    }
   }
 
   /** A control-flow node that wraps a `CasePattern`. */
@@ -866,6 +900,19 @@ module ExprNodes {
     final override RelationalOperation getExpr() { result = super.getExpr() }
   }
 
+  private class SplatExprChildMapping extends OperationExprChildMapping, SplatExpr {
+    override predicate relevantChild(AstNode n) { n = this.getOperand() }
+  }
+
+  /** A control-flow node that wraps a `SplatExpr` AST expression. */
+  class SplatExprCfgNode extends UnaryOperationCfgNode {
+    override string getAPrimaryQlClass() { result = "SplatExprCfgNode" }
+
+    override SplatExprChildMapping e;
+
+    final override SplatExpr getExpr() { result = super.getExpr() }
+  }
+
   /** A control-flow node that wraps an `ElementReference` AST expression. */
   class ElementReferenceCfgNode extends MethodCallCfgNode {
     override string getAPrimaryQlClass() { result = "ElementReferenceCfgNode" }

ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll

@@ -211,8 +211,11 @@ private predicate inBooleanContext(AstNode n) {
   or
   exists(CaseExpr c, WhenClause w |
     not exists(c.getValue()) and
-    c.getABranch() = w and
+    c.getABranch() = w
+  |
     w.getPattern(_) = n
+    or
+    w = n
   )
 }
 
@@ -233,8 +236,11 @@ private predicate inMatchingContext(AstNode n) {
   or
   exists(CaseExpr c, WhenClause w |
     exists(c.getValue()) and
-    c.getABranch() = w and
+    c.getABranch() = w
+  |
     w.getPattern(_) = n
+    or
+    w = n
   )
   or
   n instanceof CasePattern

ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll

@@ -400,8 +400,9 @@ module Trees {
       c instanceof SimpleCompletion
       or
       exists(int i, WhenTree branch | branch = this.getBranch(i) |
-        last(branch.getLastPattern(), pred, c) and
+        pred = branch and
         first(this.getBranch(i + 1), succ) and
+        c.isValidFor(branch) and
         c.(ConditionalCompletion).getValue() = false
       )
       or
@@ -1397,8 +1398,10 @@ module Trees {
     final override ControlFlowTree getChildElement(int i) { result = this.getMethodName(i) }
   }
 
-  private class WhenTree extends PreOrderTree, WhenClause {
-    final override predicate propagatesAbnormal(AstNode child) { child = this.getAPattern() }
+  private class WhenTree extends ControlFlowTree, WhenClause {
+    final override predicate propagatesAbnormal(AstNode child) {
+      child = [this.getAPattern(), this.getBody()]
+    }
 
     final Expr getLastPattern() {
       exists(int i |
@@ -1407,28 +1410,35 @@ module Trees {
       )
     }
 
+    final override predicate first(AstNode first) { first(this.getPattern(0), first) }
+
     final override predicate last(AstNode last, Completion c) {
-      last(this.getLastPattern(), last, c) and
+      last = this and
+      c.isValidFor(this) and
       c.(ConditionalCompletion).getValue() = false
       or
       last(this.getBody(), last, c)
     }
 
     final override predicate succ(AstNode pred, AstNode succ, Completion c) {
       pred = this and
-      first(this.getPattern(0), succ) and
-      c instanceof SimpleCompletion
+      c.isValidFor(this) and
+      c.(ConditionalCompletion).getValue() = true and
+      first(this.getBody(), succ)
       or
       exists(int i, Expr p, boolean b |
         p = this.getPattern(i) and
         last(p, pred, c) and
         b = c.(ConditionalCompletion).getValue()
       |
         b = true and
-        first(this.getBody(), succ)
+        succ = this
         or
         b = false and
         first(this.getPattern(i + 1), succ)
+        or
+        not exists(this.getPattern(i + 1)) and
+        succ = this
       )
     }
   }

ruby/ql/lib/codeql/ruby/controlflow/internal/Splitting.qll

@@ -86,6 +86,10 @@ private module ConditionalCompletionSplitting {
         last(succ.(ConditionalExpr).getBranch(_), pred, c) and
         completion = c
       )
+      or
+      succ(pred, succ, c) and
+      succ instanceof WhenClause and
+      completion = c
     }
 
     override predicate hasEntryScope(CfgScope scope, AstNode succ) { none() }

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

@@ -7,22 +7,53 @@ private import codeql.ruby.controlflow.CfgNodes
 private import codeql.ruby.dataflow.SSA
 private import codeql.ruby.ast.internal.Constant
 private import codeql.ruby.InclusionTests
+private import codeql.ruby.ast.internal.Literal
 
-private predicate stringConstCompare(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch) {
+cached
+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
+      // Only consider strings without any interpolations
+      not strLitNode.getExpr().getComponent(_) instanceof StringInterpolationComponent and
+      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)
+  or
+  exists(CfgNodes::ExprNodes::BinaryOperationCfgNode g |
+    g = guard and
+    stringConstCompareOr(guard, branch) and
+    stringConstCompare(g.getLeftOperand(), testedNode, _)
+  )
+}
+
+/**
+ * Holds if `guard` is an `or` expression whose operands are string comparison guards.
+ * For example:
+ *
+ * ```rb
+ * x == "foo" or x == "bar"
+ * ```
+ */
+private predicate stringConstCompareOr(
+  CfgNodes::ExprNodes::BinaryOperationCfgNode guard, boolean branch
+) {
+  guard.getExpr() instanceof LogicalOrExpr and
+  branch = true and
+  forall(CfgNode innerGuard | innerGuard = guard.getAnOperand() |
+    stringConstCompare(innerGuard, any(Ssa::Definition def).getARead(), branch)
+  )
 }
 
 /**
@@ -72,10 +103,13 @@ deprecated class StringConstCompare extends DataFlow::BarrierGuard,
   }
 }
 
-private predicate stringConstArrayInclusionCall(CfgNodes::ExprCfgNode g, CfgNode e, boolean branch) {
+cached
+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 +166,68 @@ 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}")
+ * when *["bob", "charlie"]
+ *   User.find_by("username = #{name}")
+ * when "dave", "eve" # this is not yet recognised as a barrier guard
+ *   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
+    (
+      guard =
+        any(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode |
+          branchNode = case.getBranch(_) 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
+            pattern =
+              any(CfgNodes::ExprNodes::SplatExprCfgNode splat |
+                // when *["foo", "bar"]
+                forex(ExprCfgNode elem |
+                  elem = splat.getOperand().(ExprNodes::ArrayLiteralCfgNode).getAnArgument()
+                |
+                  elem instanceof ExprNodes::StringLiteralCfgNode
+                )
+                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
+                  )
+                )
+              )
+          )
+        )
+      or
+      // in "foo"
+      exists(
+        CfgNodes::ExprNodes::InClauseCfgNode branchNode, ExprNodes::StringLiteralCfgNode pattern
+      |
+        branchNode = case.getBranch(_) and
+        pattern = branchNode.getPattern() and
+        guard = pattern
+      )
+    )
+  )
+}

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

@@ -615,7 +615,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.
@@ -625,21 +625,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)
     )
@@ -669,8 +669,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

@@ -129,7 +129,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