Ruby: Include content flow through captured variables

Author: hvitved

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

@@ -693,16 +693,30 @@ module ExprNodes {
 
     override VariableAccess e;
 
-    final override VariableAccess getExpr() { result = ExprCfgNode.super.getExpr() }
+    override VariableAccess 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 `VariableReadAccess` AST expression. */
-  class VariableReadAccessCfgNode extends ExprCfgNode {
+  class VariableReadAccessCfgNode extends VariableAccessCfgNode {
     override string getAPrimaryQlClass() { result = "VariableReadAccessCfgNode" }
 
     override VariableReadAccess e;
 
-    final override VariableReadAccess getExpr() { result = ExprCfgNode.super.getExpr() }
+    override VariableReadAccess getExpr() { result = VariableAccessCfgNode.super.getExpr() }
+  }
+
+  /** 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 {
@@ -728,21 +742,32 @@ module ExprNodes {
   }
 
   /** A control-flow node that wraps a `SelfVariableAccess` AST expression. */
-  class SelfVariableAccessCfgNode extends ExprCfgNode {
+  class SelfVariableAccessCfgNode extends VariableAccessCfgNode {
     final override string getAPrimaryQlClass() { result = "SelfVariableAccessCfgNode" }
 
     override SelfVariableAccessMapping e;
 
-    override SelfVariableAccess getExpr() { result = ExprCfgNode.super.getExpr() }
+    override SelfVariableAccess getExpr() { result = VariableAccessCfgNode.super.getExpr() }
   }
 
   /** A control-flow node that wraps a `VariableWriteAccess` AST expression. */
-  class VariableWriteAccessCfgNode extends ExprCfgNode {
+  class VariableWriteAccessCfgNode extends VariableAccessCfgNode {
     override string getAPrimaryQlClass() { result = "VariableWriteAccessCfgNode" }
 
     override VariableWriteAccess e;
 
-    final override VariableWriteAccess getExpr() { result = ExprCfgNode.super.getExpr() }
+    override VariableWriteAccess getExpr() { result = VariableAccessCfgNode.super.getExpr() }
+  }
+
+  /** A control-flow node that wraps a `LocalVariableWriteAccess` AST expression. */
+  class LocalVariableWriteAccessCfgNode extends VariableWriteAccessCfgNode {
+    override string getAPrimaryQlClass() { result = "LocalVariableWriteAccessCfgNode" }
+
+    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) {
@@ -995,10 +1002,8 @@ private module OutNodes {
 
 import OutNodes
 
-predicate jumpStep(Node pred, Node succ) {
-  SsaImpl::captureFlowIn(_, pred.(SsaDefinitionNode).getDefinition(),
-    succ.(SsaDefinitionNode).getDefinition())
-  or
+private predicate jumpStepCommon(Node pred, Node succ) {
+  // Flow out of a method directly via a captured variable
   SsaImpl::captureFlowOut(_, pred.(SsaDefinitionNode).getDefinition(),
     succ.(SsaDefinitionNode).getDefinition())
   or
@@ -1007,6 +1012,33 @@ predicate jumpStep(Node pred, Node succ) {
   FlowSummaryImpl::Private::Steps::summaryJumpStep(pred, succ)
 }
 
+predicate jumpStep(Node pred, Node succ) {
+  jumpStepCommon(pred, succ)
+  or
+  // 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 via content on a captured variable
+  exists(CfgNodes::ExprNodes::LocalVariableReadAccessCfgNode inner |
+    inner = pred.(PostUpdateNode).getPreUpdateNode().asExpr()
+  |
+    SsaImpl::captureContentFlowOut(_, inner, succ.asExpr())
+    or
+    SsaImpl::captureContentFlowOutPhi(_, inner, succ.(SsaDefinitionNode).getDefinition())
+  )
+}
+
+predicate jumpStepTypeTracker(Node nodeFrom, Node nodeTo) {
+  jumpStepCommon(nodeFrom, nodeTo)
+  or
+  // Flow into a method via a captured variable
+  SsaImpl::captureFlowIn(_, nodeFrom.(SsaDefinitionNode).getDefinition(), _, _,
+    nodeTo.(SsaDefinitionNode).getDefinition())
+}
+
 private ContentSet getKeywordContent(string name) {
   exists(ConstantValue::ConstantSymbolValue key |
     result.isSingleton(TKnownElementContent(key)) and

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

@@ -477,7 +477,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()
     )
@@ -542,7 +542,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()
     )