Ruby: Include content flow through captured variables

Author: hvitved

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

@@ -693,7 +693,21 @@ module ExprNodes {
 
     override VariableReadAccess e;
 
-    final override VariableReadAccess getExpr() { result = ExprCfgNode.super.getExpr() }
+    override VariableReadAccess getExpr() { result = ExprCfgNode.super.getExpr() }
+
+    /** Gets the variable that is being accessed. */
+    Variable getVariable() { result = this.getExpr().getVariable() }
+  }
+
+  /** A control-flow node that wraps a `LocalVariableReadAccess` AST expression. */
+  class LocalVariableReadAccessCfgNode extends VariableReadAccessCfgNode {
+    override string getAPrimaryQlClass() { result = "LocalVariableReadAccessCfgNode" }
+
+    override LocalVariableReadAccess e;
+
+    final override LocalVariableReadAccess getExpr() { result = super.getExpr() }
+
+    final override LocalVariable getVariable() { result = super.getVariable() }
   }
 
   private class InstanceVariableAccessMapping extends ExprChildMapping, InstanceVariableAccess {
@@ -733,7 +747,21 @@ module ExprNodes {
 
     override VariableWriteAccess e;
 
-    final override VariableWriteAccess getExpr() { result = ExprCfgNode.super.getExpr() }
+    override VariableWriteAccess getExpr() { result = ExprCfgNode.super.getExpr() }
+
+    /** Gets the variable that is being accessed. */
+    Variable getVariable() { result = this.getExpr().getVariable() }
+  }
+
+  /** A control-flow node that wraps a `LocalVariableWriteAccess` AST expression. */
+  class LocalVariableWriteAccessCfgNode extends VariableWriteAccessCfgNode {
+    override string getAPrimaryQlClass() { result = "LocalVariableReadAccessCfgNode" }
+
+    override LocalVariableWriteAccess e;
+
+    final override LocalVariableWriteAccess getExpr() { result = super.getExpr() }
+
+    final override LocalVariable getVariable() { result = super.getVariable() }
   }
 
   /** A control-flow node that wraps a `ConstantReadAccess` AST expression. */

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

@@ -71,6 +71,22 @@ CfgNodes::ExprCfgNode getAPostUpdateNodeForArg(Argument arg) {
   not exists(getALastEvalNode(result))
 }
 
+/**
+ * Gets the data-flow node referencing SSA definition `def` at index `i` in basic
+ * block `bb`. The node is either the definition itself, or a read of the
+ * definition.
+ */
+Node getSsaRefNode(Ssa::Definition def, BasicBlock bb, int i) {
+  def = result.(SsaDefinitionNode).getDefinition() and
+  def.definesAt(_, bb, i)
+  or
+  exists(CfgNodes::ExprCfgNode e |
+    e = result.asExpr() and
+    e = bb.getNode(i) and
+    e = def.getARead()
+  )
+}
+
 /** Provides predicates related to local data flow. */
 module LocalFlow {
   private import codeql.ruby.dataflow.internal.SsaImpl
@@ -80,15 +96,9 @@ module LocalFlow {
    * 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
-      def.definesAt(_, bb, i)
-      or
-      exists(CfgNodes::ExprCfgNode e |
-        e = nodeFrom.asExpr() and
-        e = bb.getNode(i) and
-        e.getExpr() instanceof VariableReadAccess
-      )
+    exists(BasicBlock bb, int i |
+      lastRefBeforeRedef(def, bb, i, next) and
+      nodeFrom = getSsaRefNode(def, bb, i)
     )
   }
 
@@ -142,18 +152,15 @@ module LocalFlow {
       // Flow into phi node
       exists(Ssa::PhiNode phi |
         localFlowSsaInput(nodeFrom, def, phi) and
-        phi = nodeTo.(SsaDefinitionNode).getDefinition() and
-        def = phi.getAnInput()
+        phi = nodeTo.(SsaDefinitionNode).getDefinition()
       )
+      // or
+      // // Flow into uncertain SSA definition
+      // exists(SsaImpl::UncertainWriteDefinition uncertain |
+      //   localFlowSsaInput(nodeFrom, def, uncertain) and
+      //   uncertain = nodeTo.(SsaDefinitionNode).getDefinition()
+      // )
     )
-    // TODO
-    // or
-    // // Flow into uncertain SSA definition
-    // exists(LocalFlow::UncertainExplicitSsaDefinition uncertain |
-    //   localFlowSsaInput(nodeFrom, def, uncertain) and
-    //   uncertain = nodeTo.(SsaDefinitionNode).getDefinition() and
-    //   def = uncertain.getPriorDefinition()
-    // )
   }
 
   predicate localFlowStepCommon(Node nodeFrom, Node nodeTo) {
@@ -996,12 +1003,19 @@ private module OutNodes {
 import OutNodes
 
 predicate jumpStep(Node pred, Node succ) {
-  SsaImpl::captureFlowIn(_, pred.(SsaDefinitionNode).getDefinition(),
-    succ.(SsaDefinitionNode).getDefinition())
+  // Flow into a method via a captured variable
+  exists(Ssa::Definition def, BasicBlock bb, int i |
+    SsaImpl::captureFlowIn(_, def, bb, i, succ.(SsaDefinitionNode).getDefinition()) and
+    [pred, pred.(PostUpdateNode).getPreUpdateNode()] = getSsaRefNode(def, bb, i)
+  )
   or
+  // Flow out of a method directly via a captured variable
   SsaImpl::captureFlowOut(_, pred.(SsaDefinitionNode).getDefinition(),
     succ.(SsaDefinitionNode).getDefinition())
   or
+  // Flow out of a method via content on a captured variable
+  SsaImpl::captureContentFlowOut(_, pred.(PostUpdateNode).getPreUpdateNode().asExpr(), succ.asExpr())
+  or
   succ.asExpr().getExpr().(ConstantReadAccess).getValue() = pred.asExpr().getExpr()
 }
 

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

@@ -468,7 +468,7 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
     |
       def.getARead() = testedNode and
       guardChecks(g, testedNode, branch) and
-      SsaImpl::captureFlowIn(call, def, result) and
+      SsaImpl::captureFlowIn(call, def, _, _, result) and
       guardControlsBlock(g, call.getBasicBlock(), branch) and
       result.getBasicBlock().getScope() = call.getExpr().(MethodCall).getBlock()
     )
@@ -533,7 +533,7 @@ abstract deprecated class BarrierGuard extends CfgNodes::ExprCfgNode {
     |
       def.getARead() = testedNode and
       this.checks(testedNode, branch) and
-      SsaImpl::captureFlowIn(call, def, result) and
+      SsaImpl::captureFlowIn(call, def, _, _, result) and
       this.controlsBlock(call.getBasicBlock(), branch) and
       result.getBasicBlock().getScope() = call.getExpr().(MethodCall).getBlock()
     )