Merge pull request #15103 from hvitved/ruby/simple-pattern-flow

Ruby: Model simple pattern matching as value steps instead of taint steps

Author: hvitved

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

@@ -663,7 +663,8 @@ private module TrackInstanceInput implements CallGraphConstruction::InputSig {
     // We exclude steps into type checked variables. For those, we instead rely on the
     // type being checked against
     localFlowStep(nodeFrom, nodeTo, summary) and
-    not hasAdjacentTypeCheckedReads(nodeTo)
+    not hasAdjacentTypeCheckedReads(nodeTo) and
+    not asModulePattern(nodeTo, _)
   }
 
   predicate stepCall(DataFlow::Node nodeFrom, DataFlow::Node nodeTo, StepSummary summary) {

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

@@ -143,14 +143,40 @@ module LocalFlow {
     SsaImpl::adjacentReadPairExt(def, nodeFrom.asExpr(), nodeTo.asExpr())
   }
 
+  /**
+   * Holds if SSA definition `def` assigns `value` to the underlying variable.
+   *
+   * This is either a direct assignment, `x = value`, or an assignment via
+   * simple pattern matching
+   *
+   * ```rb
+   * case value
+   *  in Foo => x then ...
+   *  in y => then ...
+   * end
+   * ```
+   */
+  predicate ssaDefAssigns(Ssa::WriteDefinition def, CfgNodes::ExprCfgNode value) {
+    def.assigns(value)
+    or
+    exists(CfgNodes::ExprNodes::CaseExprCfgNode case, CfgNodes::AstCfgNode pattern |
+      case.getValue() = value and
+      pattern = case.getBranch(_).(CfgNodes::ExprNodes::InClauseCfgNode).getPattern()
+    |
+      def.getWriteAccess() = pattern
+      or
+      def.getWriteAccess() = pattern.(CfgNodes::ExprNodes::AsPatternCfgNode).getVariableAccess()
+    )
+  }
+
   /**
    * Holds if there is a local flow step from `nodeFrom` to `nodeTo` involving
    * SSA definition `def`.
    */
   pragma[nomagic]
   predicate localSsaFlowStep(SsaImpl::DefinitionExt def, Node nodeFrom, Node nodeTo) {
     // Flow from assignment into SSA definition
-    def.(Ssa::WriteDefinition).assigns(nodeFrom.asExpr()) and
+    ssaDefAssigns(def, nodeFrom.asExpr()) and
     nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def
     or
     // Flow from SSA definition to first read
@@ -273,7 +299,7 @@ module VariableCapture {
     or
     exists(Ssa::Definition def |
       def.getARead() = e2 and
-      def.getAnUltimateDefinition().(Ssa::WriteDefinition).assigns(e1)
+      LocalFlow::ssaDefAssigns(def.getAnUltimateDefinition(), e1)
     )
   }
 
@@ -574,8 +600,7 @@ private module Cached {
 
   /** Holds if `n` wraps an SSA definition without ingoing flow. */
   private predicate entrySsaDefinition(SsaDefinitionExtNode n) {
-    n.getDefinitionExt() =
-      any(SsaImpl::WriteDefinition def | not def.(Ssa::WriteDefinition).assigns(_))
+    n.getDefinitionExt() = any(SsaImpl::WriteDefinition def | not LocalFlow::ssaDefAssigns(def, _))
   }
 
   pragma[nomagic]

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

@@ -79,11 +79,16 @@ private module Cached {
   cached
   predicate defaultAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
     // value of `case` expression into variables in patterns
-    exists(CfgNodes::ExprNodes::CaseExprCfgNode case, CfgNodes::ExprNodes::InClauseCfgNode clause |
-      nodeFrom.asExpr() = case.getValue() and
+    exists(
+      CfgNodes::ExprNodes::CaseExprCfgNode case, CfgNodes::ExprCfgNode value,
+      CfgNodes::ExprNodes::InClauseCfgNode clause, Ssa::Definition def
+    |
+      nodeFrom.asExpr() = value and
+      value = case.getValue() and
       clause = case.getBranch(_) and
-      nodeTo.(SsaDefinitionExtNode).getDefinitionExt().(Ssa::Definition).getControlFlowNode() =
-        variablesInPattern(clause.getPattern())
+      def = nodeTo.(SsaDefinitionExtNode).getDefinitionExt() and
+      def.getControlFlowNode() = variablesInPattern(clause.getPattern()) and
+      not LocalFlow::ssaDefAssigns(def, value)
     )
     or
     // operation involving `nodeFrom`

ruby/ql/test/library-tests/dataflow/local/DataflowStep.expected

@@ -2528,6 +2528,8 @@
 | local_dataflow.rb:78:12:78:20 | [post] self | local_dataflow.rb:85:22:85:28 | self |
 | local_dataflow.rb:78:12:78:20 | [post] self | local_dataflow.rb:86:28:86:34 | self |
 | local_dataflow.rb:78:12:78:20 | [post] self | local_dataflow.rb:87:20:87:26 | self |
+| local_dataflow.rb:78:12:78:20 | call to source | local_dataflow.rb:79:13:79:13 | b |
+| local_dataflow.rb:78:12:78:20 | call to source | local_dataflow.rb:80:8:80:8 | a |
 | local_dataflow.rb:78:12:78:20 | self | local_dataflow.rb:79:20:79:26 | self |
 | local_dataflow.rb:78:12:78:20 | self | local_dataflow.rb:80:24:80:30 | self |
 | local_dataflow.rb:78:12:78:20 | self | local_dataflow.rb:82:7:82:13 | self |

ruby/ql/test/library-tests/dataflow/local/local_dataflow.rb

@@ -76,8 +76,8 @@ def test_case x
   end
 
   z = case source(1)
-    in 5 => b then sink(b) # $ hasTaintFlow=1
-    in a if a > 0 then sink(a) # $ hasTaintFlow=1
+    in 5 => b then sink(b) # $ hasValueFlow=1
+    in a if a > 0 then sink(a) # $ hasValueFlow=1
     in [c, *d, e ] then [
       sink(c), # $ hasTaintFlow=1
       sink(d), # $ hasTaintFlow=1