Merge pull request github#10931 from hvitved/ruby/fix-flow-into-phis

Ruby: Fix flow steps into phi nodes

Author: hvitved

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

@@ -76,19 +76,29 @@ module LocalFlow {
   private import codeql.ruby.dataflow.internal.SsaImpl
 
   /**
-   * Holds if `nodeFrom` is a last node referencing SSA definition `def`, which
-   * can reach `next`.
+   * Holds if `nodeFrom` is a node for SSA definition `def`, which can reach `next`.
    */
-  private predicate localFlowSsaInput(Node nodeFrom, Ssa::Definition def, Ssa::Definition next) {
-    exists(BasicBlock bb, int i | lastRefBeforeRedef(def, bb, i, next) |
-      def = nodeFrom.(SsaDefinitionNode).getDefinition() and
+  private predicate localFlowSsaInputFromDef(
+    SsaDefinitionNode nodeFrom, Ssa::Definition def, Ssa::Definition next
+  ) {
+    exists(BasicBlock bb, int i |
+      lastRefBeforeRedef(def, bb, i, next) and
+      def = nodeFrom.getDefinition() and
       def.definesAt(_, bb, i)
-      or
-      exists(CfgNodes::ExprCfgNode e |
-        e = nodeFrom.asExpr() and
-        e = bb.getNode(i) and
-        e.getExpr() instanceof VariableReadAccess
-      )
+    )
+  }
+
+  /**
+   * Holds if `exprFrom` is a last read of SSA definition `def`, which
+   * can reach `next`.
+   */
+  predicate localFlowSsaInputFromExpr(
+    CfgNodes::ExprCfgNode exprFrom, Ssa::Definition def, Ssa::Definition next
+  ) {
+    exists(BasicBlock bb, int i |
+      lastRefBeforeRedef(def, bb, i, next) and
+      exprFrom = bb.getNode(i) and
+      exprFrom.getExpr() instanceof VariableReadAccess
     )
   }
 
@@ -139,9 +149,9 @@ module LocalFlow {
       // Flow from read to next read
       localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo)
       or
-      // Flow into phi node
+      // Flow into phi node from definition
       exists(Ssa::PhiNode phi |
-        localFlowSsaInput(nodeFrom, def, phi) and
+        localFlowSsaInputFromDef(nodeFrom, def, phi) and
         phi = nodeTo.(SsaDefinitionNode).getDefinition() and
         def = phi.getAnInput()
       )
@@ -308,6 +318,18 @@ private module Cached {
     LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo) and
     not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _)
     or
+    // Flow into phi node from read
+    exists(Ssa::Definition def, Ssa::PhiNode phi, CfgNodes::ExprCfgNode exprFrom |
+      LocalFlow::localFlowSsaInputFromExpr(exprFrom, def, phi) and
+      phi = nodeTo.(SsaDefinitionNode).getDefinition() and
+      def = phi.getAnInput()
+    |
+      exprFrom = nodeFrom.asExpr() and
+      not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom, _)
+      or
+      exprFrom = nodeFrom.(PostUpdateNode).getPreUpdateNode().asExpr()
+    )
+    or
     FlowSummaryImpl::Private::Steps::summaryLocalStep(nodeFrom, nodeTo, true)
   }
 
@@ -338,6 +360,14 @@ private module Cached {
     )
     or
     LocalFlow::localSsaFlowStepUseUse(_, nodeFrom, nodeTo)
+    or
+    // Flow into phi node from read
+    exists(Ssa::Definition def, Ssa::PhiNode phi, CfgNodes::ExprCfgNode exprFrom |
+      LocalFlow::localFlowSsaInputFromExpr(exprFrom, def, phi) and
+      phi = nodeTo.(SsaDefinitionNode).getDefinition() and
+      def = phi.getAnInput() and
+      exprFrom = [nodeFrom.asExpr(), nodeFrom.(PostUpdateNode).getPreUpdateNode().asExpr()]
+    )
   }
 
   private predicate entrySsaDefinition(SsaDefinitionNode n) {

ruby/ql/test/library-tests/dataflow/ssa-flow/ssa-flow.expected

@@ -0,0 +1,20 @@
+failures
+edges
+| ssa_flow.rb:12:9:12:9 | [post] a [element 0] :  | ssa_flow.rb:16:10:16:10 | a [element 0] :  |
+| ssa_flow.rb:12:9:12:9 | [post] a [element 0] :  | ssa_flow.rb:16:10:16:10 | a [element 0] :  |
+| ssa_flow.rb:12:16:12:23 | call to taint :  | ssa_flow.rb:12:9:12:9 | [post] a [element 0] :  |
+| ssa_flow.rb:12:16:12:23 | call to taint :  | ssa_flow.rb:12:9:12:9 | [post] a [element 0] :  |
+| ssa_flow.rb:16:10:16:10 | a [element 0] :  | ssa_flow.rb:16:10:16:13 | ...[...] |
+| ssa_flow.rb:16:10:16:10 | a [element 0] :  | ssa_flow.rb:16:10:16:13 | ...[...] |
+nodes
+| ssa_flow.rb:12:9:12:9 | [post] a [element 0] :  | semmle.label | [post] a [element 0] :  |
+| ssa_flow.rb:12:9:12:9 | [post] a [element 0] :  | semmle.label | [post] a [element 0] :  |
+| ssa_flow.rb:12:16:12:23 | call to taint :  | semmle.label | call to taint :  |
+| ssa_flow.rb:12:16:12:23 | call to taint :  | semmle.label | call to taint :  |
+| ssa_flow.rb:16:10:16:10 | a [element 0] :  | semmle.label | a [element 0] :  |
+| ssa_flow.rb:16:10:16:10 | a [element 0] :  | semmle.label | a [element 0] :  |
+| ssa_flow.rb:16:10:16:13 | ...[...] | semmle.label | ...[...] |
+| ssa_flow.rb:16:10:16:13 | ...[...] | semmle.label | ...[...] |
+subpaths
+#select
+| ssa_flow.rb:16:10:16:13 | ...[...] | ssa_flow.rb:12:16:12:23 | call to taint :  | ssa_flow.rb:16:10:16:13 | ...[...] | $@ | ssa_flow.rb:12:16:12:23 | call to taint :  | call to taint :  |

ruby/ql/test/library-tests/dataflow/ssa-flow/ssa-flow.ql

@@ -0,0 +1,11 @@
+/**
+ * @kind path-problem
+ */
+
+import codeql.ruby.AST
+import TestUtilities.InlineFlowTest
+import PathGraph
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, DefaultValueFlowConf conf
+where conf.hasFlowPath(source, sink)
+select sink, source, sink, "$@", source, source.toString()

ruby/ql/test/library-tests/dataflow/ssa-flow/ssa_flow.rb

@@ -0,0 +1,32 @@
+def taint x
+    x
+end
+
+def sink x
+    puts "SINK: #{x}"
+end
+
+def m1
+    a = Array.new
+    if rand() > 0 then
+        a[0] = taint(1)
+    else
+        a = nil
+    end
+    sink(a[0]) # $ hasValueFlow=1
+end
+
+m1
+
+def m2
+    a = Array.new
+    if rand() > 0 then
+        a[0] = taint(2)
+        a.clear
+    else
+        a = nil
+    end
+    sink(a[0])
+end
+
+m2