Ruby: Extend barrier guards to handle phi inputs

Author: hvitved

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

@@ -72,31 +72,33 @@ CfgNodes::ExprCfgNode getAPostUpdateNodeForArg(Argument arg) {
   not exists(getALastEvalNode(result))
 }
 
-/** Provides predicates related to local data flow. */
-module LocalFlow {
-  private import codeql.ruby.dataflow.internal.SsaImpl
+/** An SSA definition into which another SSA definition may flow. */
+class SsaInputDefinitionExt extends SsaImpl::DefinitionExt {
+  SsaInputDefinitionExt() {
+    this instanceof Ssa::PhiNode
+    or
+    this instanceof SsaImpl::PhiReadNode
+  }
 
-  /** An SSA definition into which another SSA definition may flow. */
-  private class SsaInputDefinitionExtNode extends SsaDefinitionExtNode {
-    SsaInputDefinitionExtNode() {
-      def instanceof Ssa::PhiNode
-      or
-      def instanceof SsaImpl::PhiReadNode
-    }
+  predicate hasInputFromBlock(SsaImpl::DefinitionExt def, BasicBlock bb, int i, BasicBlock input) {
+    SsaImpl::lastRefBeforeRedefExt(def, bb, i, input, this)
   }
+}
 
+/** Provides predicates related to local data flow. */
+module LocalFlow {
   /**
    * Holds if `nodeFrom` is a node for SSA definition `def`, which can reach `next`.
    */
   pragma[nomagic]
   private predicate localFlowSsaInputFromDef(
-    SsaDefinitionExtNode nodeFrom, SsaImpl::DefinitionExt def, SsaInputDefinitionExtNode next
+    SsaDefinitionExtNode nodeFrom, SsaImpl::DefinitionExt def, SsaInputNode nodeTo
   ) {
-    exists(BasicBlock bb, int i |
-      lastRefBeforeRedefExt(def, bb, i, next.getDefinitionExt()) and
+    exists(BasicBlock bb, int i, BasicBlock input, SsaInputDefinitionExt next |
+      next.hasInputFromBlock(def, bb, i, input) and
       def = nodeFrom.getDefinitionExt() and
       def.definesAt(_, bb, i, _) and
-      nodeFrom != next
+      nodeTo = TSsaInputNode(next, input)
     )
   }
 
@@ -105,14 +107,16 @@ module LocalFlow {
    * can reach `next`.
    */
   pragma[nomagic]
-  predicate localFlowSsaInputFromRead(
-    SsaImpl::DefinitionExt def, Node nodeFrom, SsaInputDefinitionExtNode next
-  ) {
-    exists(BasicBlock bb, int i, CfgNodes::ExprCfgNode exprFrom |
-      SsaImpl::lastRefBeforeRedefExt(def, bb, i, next.getDefinitionExt()) and
+  predicate localFlowSsaInputFromRead(SsaImpl::DefinitionExt def, Node nodeFrom, SsaInputNode nodeTo) {
+    exists(
+      BasicBlock bb, int i, CfgNodes::ExprCfgNode exprFrom, BasicBlock input,
+      SsaImpl::DefinitionExt next
+    |
+      SsaImpl::lastRefBeforeRedefExt(def, bb, i, input, next) and
       exprFrom = bb.getNode(i) and
       exprFrom.getExpr() instanceof VariableReadAccess and
-      exprFrom = [nodeFrom.asExpr(), nodeFrom.(PostUpdateNodeImpl).getPreUpdateNode().asExpr()]
+      exprFrom = [nodeFrom.asExpr(), nodeFrom.(PostUpdateNodeImpl).getPreUpdateNode().asExpr()] and
+      nodeTo = TSsaInputNode(next, input)
     )
   }
 
@@ -181,14 +185,17 @@ module LocalFlow {
     or
     // Flow from SSA definition to first read
     def = nodeFrom.(SsaDefinitionExtNode).getDefinitionExt() and
-    firstReadExt(def, nodeTo.asExpr())
+    SsaImpl::firstReadExt(def, nodeTo.asExpr())
     or
     // Flow from post-update read to next read
     localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNodeImpl).getPreUpdateNode(), nodeTo)
     or
     // Flow into phi (read) SSA definition node from def
     localFlowSsaInputFromDef(nodeFrom, def, nodeTo)
     or
+    nodeTo.(SsaDefinitionExtNode).getDefinitionExt() = def and
+    def = nodeFrom.(SsaInputNode).getDefinitionExt()
+    or
     localFlowSsaParamInput(nodeFrom, nodeTo) and
     def = nodeTo.(SsaDefinitionExtNode).getDefinitionExt()
   }
@@ -530,6 +537,9 @@ private module Cached {
     TExprNode(CfgNodes::ExprCfgNode n) or
     TReturningNode(CfgNodes::ReturningCfgNode n) { exists(n.getReturnedValueNode()) } or
     TSsaDefinitionExtNode(SsaImpl::DefinitionExt def) or
+    TSsaInputNode(SsaInputDefinitionExt def, BasicBlock input) {
+      def.hasInputFromBlock(_, _, _, input)
+    } or
     TCapturedVariableNode(VariableCapture::CapturedVariable v) or
     TNormalParameterNode(Parameter p) {
       p instanceof SimpleParameter or
@@ -802,6 +812,8 @@ import Cached
 predicate nodeIsHidden(Node n) {
   n.(SsaDefinitionExtNode).isHidden()
   or
+  n instanceof SsaInputNode
+  or
   n = LocalFlow::getParameterDefNode(_)
   or
   exists(AstNode desug |
@@ -863,6 +875,57 @@ class SsaDefinitionExtNode extends NodeImpl, TSsaDefinitionExtNode {
   override string toStringImpl() { result = def.toString() }
 }
 
+/**
+ * A node that represents an input to an SSA phi (read) definition.
+ *
+ * This allows for barrier guards to filter input to phi nodes. For example, in
+ *
+ * ```rb
+ * x = taint
+ * if x != "safe" then
+ *     x = "safe"
+ * end
+ * sink x
+ * ```
+ *
+ * the `false` edge out of `x != "safe"` guards the input from `x = taint` into the
+ * `phi` node after the condition.
+ *
+ * It is also relevant to filter input into phi read nodes:
+ *
+ * ```rb
+ * x = taint
+ * if b then
+ *     if x != "safe1" then
+ *         return
+ *     end
+ * else
+ *     if x != "safe2" then
+ *         return
+ *     end
+ * end
+ *
+ * sink x
+ * ```
+ *
+ * both inputs into the phi read node after the outer condition are guarded.
+ */
+class SsaInputNode extends NodeImpl, TSsaInputNode {
+  SsaImpl::DefinitionExt def;
+  BasicBlock input;
+
+  SsaInputNode() { this = TSsaInputNode(def, input) }
+
+  /** Gets the underlying SSA definition. */
+  SsaImpl::DefinitionExt getDefinitionExt() { result = def }
+
+  override CfgScope getCfgScope() { result = input.getScope() }
+
+  override Location getLocationImpl() { result = input.getLastNode().getLocation() }
+
+  override string toStringImpl() { result = "[input] " + def }
+}
+
 /** An SSA definition for a `self` variable. */
 class SsaSelfDefinitionNode extends SsaDefinitionExtNode {
   private SelfVariable self;

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

@@ -856,24 +856,52 @@ private predicate sameSourceVariable(Ssa::Definition def1, Ssa::Definition def2)
  * in data flow and taint tracking.
  */
 module BarrierGuard<guardChecksSig/3 guardChecks> {
+  private import SsaImpl as SsaImpl
+
   pragma[nomagic]
   private predicate guardChecksSsaDef(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def) {
     guardChecks(g, def.getARead(), branch)
   }
 
   pragma[nomagic]
-  private predicate guardControlsSsaDef(
+  private predicate guardControlsSsaRead(
     CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, Node n
   ) {
     def.getARead() = n.asExpr() and
     guardControlsBlock(g, n.asExpr().getBasicBlock(), branch)
   }
 
+  pragma[nomagic]
+  private predicate guardControlsPhiInput(
+    CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, BasicBlock input,
+    SsaInputDefinitionExt phi
+  ) {
+    phi.hasInputFromBlock(def, _, _, input) and
+    (
+      guardControlsBlock(g, input, branch)
+      or
+      exists(SuccessorTypes::ConditionalSuccessor s |
+        g = input.getLastNode() and
+        s.getValue() = branch and
+        input.getASuccessor(s) = phi.getBasicBlock()
+      )
+    )
+  }
+
   /** Gets a node that is safely guarded by the given guard check. */
   Node getABarrierNode() {
     exists(CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def |
       guardChecksSsaDef(g, branch, def) and
-      guardControlsSsaDef(g, branch, def, result)
+      guardControlsSsaRead(g, branch, def, result)
+    )
+    or
+    exists(
+      CfgNodes::AstCfgNode g, boolean branch, Ssa::Definition def, BasicBlock input,
+      SsaInputDefinitionExt phi
+    |
+      guardChecksSsaDef(g, branch, def) and
+      guardControlsPhiInput(g, branch, def, input, phi) and
+      result = TSsaInputNode(phi, input)
     )
     or
     result.asExpr() = getAMaybeGuardedCapturedDef().getARead()

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

@@ -459,14 +459,16 @@ private module Cached {
    * The reference is either a read of `def` or `def` itself.
    */
   cached
-  predicate lastRefBeforeRedefExt(DefinitionExt def, Cfg::BasicBlock bb, int i, DefinitionExt next) {
+  predicate lastRefBeforeRedefExt(
+    DefinitionExt def, Cfg::BasicBlock bb, int i, Cfg::BasicBlock input, DefinitionExt next
+  ) {
     exists(LocalVariable v |
-      Impl::lastRefRedefExt(def, v, bb, i, next) and
+      Impl::lastRefRedefExt(def, v, bb, i, input, next) and
       not SsaInput::variableRead(bb, i, v, false)
     )
     or
     exists(SsaInput::BasicBlock bb0, int i0 |
-      Impl::lastRefRedefExt(def, _, bb0, i0, next) and
+      Impl::lastRefRedefExt(def, _, bb0, i0, input, next) and
       adjacentDefReachesUncertainReadExt(def, bb, i, bb0, i0)
     )
   }

ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.expected

@@ -6,14 +6,6 @@ edges
 | barrier_flow.rb:8:9:8:17 | call to source | barrier_flow.rb:8:5:8:5 | x | provenance |  |
 | barrier_flow.rb:24:5:24:5 | x | barrier_flow.rb:26:10:26:10 | x | provenance |  |
 | barrier_flow.rb:24:9:24:17 | call to source | barrier_flow.rb:24:5:24:5 | x | provenance |  |
-| barrier_flow.rb:36:5:36:5 | x | barrier_flow.rb:42:10:42:10 | x | provenance |  |
-| barrier_flow.rb:36:9:36:17 | call to source | barrier_flow.rb:36:5:36:5 | x | provenance |  |
-| barrier_flow.rb:46:5:46:5 | x | barrier_flow.rb:50:10:50:10 | x | provenance |  |
-| barrier_flow.rb:46:9:46:17 | call to source | barrier_flow.rb:46:5:46:5 | x | provenance |  |
-| barrier_flow.rb:54:5:54:5 | x | barrier_flow.rb:62:10:62:10 | x | provenance |  |
-| barrier_flow.rb:54:9:54:17 | call to source | barrier_flow.rb:54:5:54:5 | x | provenance |  |
-| barrier_flow.rb:66:5:66:5 | x | barrier_flow.rb:78:10:78:10 | x | provenance |  |
-| barrier_flow.rb:66:9:66:17 | call to source | barrier_flow.rb:66:5:66:5 | x | provenance |  |
 nodes
 | barrier_flow.rb:2:5:2:5 | x | semmle.label | x |
 | barrier_flow.rb:2:9:2:17 | call to source | semmle.label | call to source |
@@ -24,24 +16,8 @@ nodes
 | barrier_flow.rb:24:5:24:5 | x | semmle.label | x |
 | barrier_flow.rb:24:9:24:17 | call to source | semmle.label | call to source |
 | barrier_flow.rb:26:10:26:10 | x | semmle.label | x |
-| barrier_flow.rb:36:5:36:5 | x | semmle.label | x |
-| barrier_flow.rb:36:9:36:17 | call to source | semmle.label | call to source |
-| barrier_flow.rb:42:10:42:10 | x | semmle.label | x |
-| barrier_flow.rb:46:5:46:5 | x | semmle.label | x |
-| barrier_flow.rb:46:9:46:17 | call to source | semmle.label | call to source |
-| barrier_flow.rb:50:10:50:10 | x | semmle.label | x |
-| barrier_flow.rb:54:5:54:5 | x | semmle.label | x |
-| barrier_flow.rb:54:9:54:17 | call to source | semmle.label | call to source |
-| barrier_flow.rb:62:10:62:10 | x | semmle.label | x |
-| barrier_flow.rb:66:5:66:5 | x | semmle.label | x |
-| barrier_flow.rb:66:9:66:17 | call to source | semmle.label | call to source |
-| barrier_flow.rb:78:10:78:10 | x | semmle.label | x |
 subpaths
 #select
 | barrier_flow.rb:4:10:4:10 | x | barrier_flow.rb:2:9:2:17 | call to source | barrier_flow.rb:4:10:4:10 | x | $@ | barrier_flow.rb:2:9:2:17 | call to source | call to source |
 | barrier_flow.rb:11:14:11:14 | x | barrier_flow.rb:8:9:8:17 | call to source | barrier_flow.rb:11:14:11:14 | x | $@ | barrier_flow.rb:8:9:8:17 | call to source | call to source |
 | barrier_flow.rb:26:10:26:10 | x | barrier_flow.rb:24:9:24:17 | call to source | barrier_flow.rb:26:10:26:10 | x | $@ | barrier_flow.rb:24:9:24:17 | call to source | call to source |
-| barrier_flow.rb:42:10:42:10 | x | barrier_flow.rb:36:9:36:17 | call to source | barrier_flow.rb:42:10:42:10 | x | $@ | barrier_flow.rb:36:9:36:17 | call to source | call to source |
-| barrier_flow.rb:50:10:50:10 | x | barrier_flow.rb:46:9:46:17 | call to source | barrier_flow.rb:50:10:50:10 | x | $@ | barrier_flow.rb:46:9:46:17 | call to source | call to source |
-| barrier_flow.rb:62:10:62:10 | x | barrier_flow.rb:54:9:54:17 | call to source | barrier_flow.rb:62:10:62:10 | x | $@ | barrier_flow.rb:54:9:54:17 | call to source | call to source |
-| barrier_flow.rb:78:10:78:10 | x | barrier_flow.rb:66:9:66:17 | call to source | barrier_flow.rb:78:10:78:10 | x | $@ | barrier_flow.rb:66:9:66:17 | call to source | call to source |

ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.expected

@@ -1,13 +1,15 @@
 testFailures
 failures
 newStyleBarrierGuards
+| barrier-guards.rb:3:16:4:19 | [input] SSA phi read(foo) |
 | barrier-guards.rb:4:5:4:7 | foo |
 | barrier-guards.rb:10:5:10:7 | foo |
 | barrier-guards.rb:18:5:18:7 | foo |
 | barrier-guards.rb:24:5:24:7 | foo |
 | barrier-guards.rb:28:5:28:7 | foo |
 | barrier-guards.rb:38:5:38:7 | foo |
 | barrier-guards.rb:45:9:45:11 | foo |
+| barrier-guards.rb:70:22:71:19 | [input] SSA phi read(foo) |
 | barrier-guards.rb:71:5:71:7 | foo |
 | barrier-guards.rb:83:5:83:7 | foo |
 | barrier-guards.rb:91:5:91:7 | foo |
@@ -38,6 +40,12 @@ newStyleBarrierGuards
 | barrier-guards.rb:292:5:292:7 | foo |
 | barrier_flow.rb:19:14:19:14 | x |
 | barrier_flow.rb:32:10:32:10 | x |
+| barrier_flow.rb:38:8:38:18 | [input] phi |
+| barrier_flow.rb:48:23:48:33 | [input] phi |
+| barrier_flow.rb:56:10:57:34 | [input] SSA phi read(x) |
+| barrier_flow.rb:58:5:59:34 | [input] SSA phi read(x) |
+| barrier_flow.rb:68:10:71:11 | [input] SSA phi read(x) |
+| barrier_flow.rb:72:5:75:11 | [input] SSA phi read(x) |
 controls
 | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:4:5:4:7 | foo | true |
 | barrier-guards.rb:3:4:3:15 | ... == ... | barrier-guards.rb:6:5:6:7 | foo | false |

ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.ql

@@ -1,3 +1,4 @@
+import codeql.ruby.dataflow.internal.DataFlowPrivate
 import codeql.ruby.dataflow.internal.DataFlowPublic
 import codeql.ruby.dataflow.BarrierGuards
 import codeql.ruby.controlflow.CfgNodes
@@ -25,6 +26,7 @@ module BarrierGuardTest implements TestSig {
     tag = "guarded" and
     exists(DataFlow::Node n |
       newStyleBarrierGuards(n) and
+      not n instanceof SsaInputNode and
       location = n.getLocation() and
       element = n.toString() and
       value = ""

ruby/ql/test/library-tests/dataflow/barrier-guards/barrier_flow.rb

@@ -39,15 +39,15 @@ def m6
         x = "safe"
     end
 
-    sink x # $ SPURIOUS hasValueFlow=6
+    sink x
 end
 
 def m7
     x = source(7)
 
     x = "safe" unless x == "safe"
 
-    sink x # $ SPURIOUS hasValueFlow=7
+    sink x
 end
 
 def m8(b)
@@ -59,7 +59,7 @@ def m8(b)
         return unless x == "safe2"
     end
 
-    sink x # $ SPURIOUS hasValueFlow=8
+    sink x
 end
 
 def m9(b)
@@ -75,5 +75,5 @@ def m9(b)
         end
     end
 
-    sink x # $ SPURIOUS hasValueFlow=9
+    sink x
 end