Go: Use DataFlowIntegration for local SSA flow

go/ql/consistency-queries/SsaConsistency.ql

@@ -0,0 +1,4 @@
+import go
+import semmle.go.dataflow.SsaImpl
+import Consistency
+import DataFlowIntegration::DfConsistency

go/ql/lib/semmle/go/controlflow/IR.qll

@@ -245,7 +245,10 @@ module IR {
     /** Gets the expression underlying this instruction. */
     Expr getExpr() { result = e }
 
-    override predicate reads(ValueEntity v) { e = v.getAReference() }
+    override predicate reads(ValueEntity v) {
+      e = v.getAReference() and
+      e.(ReferenceExpr).isRvalue()
+    }
 
     override Type getResultType() { result = e.getType() }
 

go/ql/lib/semmle/go/dataflow/SsaImpl.qll

@@ -125,7 +125,9 @@ private module Internal {
     }
 
     cached
-    predicate ssaCachedStageRevRef() { Ssa::ssaCachedStageRevRef1() }
+    predicate ssaCachedStageRevRef() {
+      Ssa::ssaCachedStageRevRef1() and DataFlowIntegrationImpl::ssaCachedStageRevRef2()
+    }
   }
 }
 
@@ -157,6 +159,48 @@ private module PublicSsaInput implements Impl::SsaInputSig {
 
 module Ssa = Impl::MakeSsa<PublicSsaInput>;
 
+private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInputSig {
+  private import codeql.util.Void
+
+  predicate ssaDefHasSource(Impl::WriteDefinition def) { def instanceof Ssa::SsaExplicitWrite }
+
+  predicate allowFlowIntoUncertainDef(Impl::UncertainWriteDefinition def) { any() }
+
+  // Keep Go's field-sensitive barrier guards rather than adding definition-level barriers.
+  class GuardValue = Void;
+
+  class Guard extends Void {
+    string toString() { none() }
+
+    predicate hasValueBranchEdge(BasicBlock bb1, BasicBlock bb2, GuardValue val) { none() }
+
+    predicate valueControlsBranchEdge(BasicBlock bb1, BasicBlock bb2, GuardValue val) { none() }
+  }
+
+  predicate guardDirectlyControlsBlock(Guard guard, BasicBlock bb, GuardValue val) { none() }
+
+  predicate supportBarrierGuardsOnPhiEdges() { none() }
+}
+
+private module DataFlowIntegrationImpl = Impl::DataFlowIntegration<DataFlowIntegrationInput>;
+
+/** Provides shared SSA nodes and local flow steps for Go data-flow analysis. */
+cached
+module DataFlowIntegration {
+  import DataFlowIntegrationImpl
+
+  /**
+   * Holds if the value of `v` flows from `nodeFrom` to `nodeTo` in one local SSA step.
+   *
+   * `isUseStep` is `true` when `nodeFrom` is a (post-update) read node and
+   * `nodeTo` is a read node or phi (read) node.
+   */
+  cached
+  predicate localFlowStep(SsaSourceVariable v, Node nodeFrom, Node nodeTo, boolean isUseStep) {
+    DataFlowIntegrationImpl::localFlowStep(v, nodeFrom, nodeTo, isUseStep)
+  }
+}
+
 /** Gets the control-flow root containing `def`. */
 ControlFlow::Root getSsaDefinitionRoot(Ssa::SsaDefinition def) {
   result = def.getBasicBlock().getScope()

go/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll

@@ -31,18 +31,34 @@ private Type getConcreteType(DataFlow::Node nd) {
 }
 
 /**
- * Holds if all concrete (that is, non-interface) types of `nd` concrete types can be determined by
- * local reasoning.
+ * Holds if local flow into `nd` may originate from a value whose concrete type is unknown.
+ * Values with concrete static types stop propagation.
+ */
+private predicate hasUnknownSource(DataFlow::Node nd) {
+  nd = getInterfaceCallReceiverSource(_) and
+  not exists(getConcreteType(nd)) and
+  (
+    not basicLocalFlowStep(_, nd)
+    or
+    // Uncertain writes may also receive values through non-local capture flow.
+    nd.(DataFlow::SsaNode).getDefinition() instanceof SsaUncertainWrite
+    or
+    exists(DataFlow::Node pred | basicLocalFlowStep(pred, nd) and hasUnknownSource(pred))
+  )
+}
+
+/**
+ * Holds if all concrete (that is, non-interface) types of `nd` can be determined by local reasoning.
+ * Checking for unknown origins rather than recursively proving every predecessor handles SSA cycles.
  *
  * `nd` is restricted to nodes that flow into the receiver value of an interface call, since that is
  * all we are ultimately interested in.
  */
 private predicate isConcreteValue(DataFlow::Node nd) {
   nd = getInterfaceCallReceiverSource(_) and
-  (
-    exists(getConcreteType(nd))
-    or
-    forex(DataFlow::Node pred | basicLocalFlowStep(pred, nd) | isConcreteValue(pred))
+  not hasUnknownSource(nd) and
+  exists(DataFlow::Node source |
+    exists(getConcreteType(source)) and basicLocalFlowStep*(source, nd)
   )
 }
 

go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll

@@ -9,10 +9,13 @@ private import DataFlowPrivate
 private import FlowSummaryImpl as FlowSummaryImpl
 private import semmle.go.dataflow.ExternalFlow
 
+private module SsaFlow = SsaImpl::DataFlowIntegration;
+
 cached
 private newtype TNode =
   MkInstructionNode(IR::Instruction insn) or
   MkSsaNode(SsaDefinition ssa) or
+  MkSsaSynthReadNode(SsaFlow::SsaSynthReadNode ssa) or
   MkGlobalFunctionNode(Function f) or
   MkImplicitVarargsSlice(IR::EvalInstruction ins) {
     // We only use CallExprs with an EvalInstruction to guarantee reachability.
@@ -26,6 +29,55 @@ private newtype TNode =
 module Private {
   private import DataFlowDispatch
 
+  /** A synthetic SSA read at a control-flow merge. */
+  class SsaSynthReadNode extends Node, MkSsaSynthReadNode {
+    private SsaFlow::SsaSynthReadNode ssa;
+
+    SsaSynthReadNode() { this = MkSsaSynthReadNode(ssa) }
+
+    SsaFlow::SsaSynthReadNode getSsaNode() { result = ssa }
+
+    override ControlFlow::Root getRoot() { result = ssa.getBasicBlock().getScope() }
+
+    override BasicBlock getBasicBlock() { result = ssa.getBasicBlock() }
+
+    override Type getType() { result = ssa.getSourceVariable().getType() }
+
+    override string getNodeKind() { result = "SSA synthetic read" }
+
+    override string toString() { result = ssa.toString() }
+
+    override Location getLocation() { result = ssa.getLocation() }
+  }
+
+  /** Gets the Go data-flow node corresponding to `n`. */
+  Node getNodeFromSsaNode(SsaFlow::Node n) {
+    result.(SsaNode).getDefinition() = n.(SsaFlow::SsaDefinitionNode).getDefinition()
+    or
+    result.(SsaSynthReadNode).getSsaNode() = n
+    or
+    exists(IR::Instruction insn, SsaSourceVariable v |
+      n.(SsaFlow::ReadNode).readsAt(insn, v) and
+      insn.reads(v) and
+      result.asInstruction() = insn
+    )
+    or
+    exists(IR::Instruction insn, SsaSourceVariable v |
+      n.(SsaFlow::PostUpdateReadNode).readsAt(insn, v) and
+      insn.reads(v) and
+      result.(PostUpdateNode).getPreUpdateNode().asInstruction() = insn
+    )
+    or
+    exists(IR::Instruction rhs |
+      rhs =
+        n.(SsaFlow::WriteDefSourceNode).getDefinition().(SsaExplicitWrite).getDefinition().getRhs()
+    |
+      result.asInstruction() = rhs
+      or
+      result.(PostUpdateNode).getPreUpdateNode().asInstruction() = rhs
+    )
+  }
+
   /** Gets the callable in which this node occurs. */
   DataFlowCallable nodeGetEnclosingCallable(Node n) {
     result.asCallable() = n.getEnclosingCallable()

go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll

@@ -70,32 +70,10 @@ predicate basicLocalFlowStep(Node nodeFrom, Node nodeTo) {
     else nodeTo.asInstruction() = evalAssert
   )
   or
-  // Instruction -> SSA defn
-  exists(IR::Instruction pred, SsaExplicitWrite succ |
-    succ.getDefinition().getRhs() = pred and
-    (
-      nodeFrom = instructionNode(pred) or
-      nodeFrom.(PostUpdateNode).getPreUpdateNode() = instructionNode(pred)
-    ) and
-    nodeTo = ssaNode(succ)
-  )
-  or
-  // SSA defn -> first SSA use
-  exists(SsaDefinition pred, IR::Instruction succ | SsaImpl::firstUse(pred, succ) |
-    (pred instanceof SsaExplicitWrite or pred instanceof SsaUncertainWrite) and
-    nodeFrom = ssaNode(pred) and
-    nodeTo = instructionNode(succ)
-  )
-  or
-  // SSA use -> successive SSA use
-  // Note this case includes Phi node traversal
-  exists(IR::Instruction pred, IR::Instruction succ | succ = getAnAdjacentUse(pred) |
-    (
-      nodeFrom = instructionNode(pred) or
-      nodeFrom.(PostUpdateNode).getPreUpdateNode() = instructionNode(pred)
-    ) and
-    nodeTo = instructionNode(succ) and
-    nodeTo != nodeFrom
+  exists(SsaImpl::DataFlowIntegration::Node pred, SsaImpl::DataFlowIntegration::Node succ |
+    SsaImpl::DataFlowIntegration::localFlowStep(_, pred, succ, _) and
+    nodeFrom = getNodeFromSsaNode(pred) and
+    nodeTo = getNodeFromSsaNode(succ)
   )
 }
 
@@ -448,7 +426,13 @@ Node getArgument(CallNode c, int i) {
 }
 
 /** Holds if `n` should be hidden from path explanations. */
-predicate nodeIsHidden(Node n) { n instanceof FlowSummaryNode }
+predicate nodeIsHidden(Node n) {
+  n instanceof FlowSummaryNode
+  or
+  n instanceof SsaSynthReadNode
+  or
+  n.(SsaNode).getDefinition() instanceof SsaImpl::PhiNode
+}
 
 class LambdaCallKind = Unit;
 

go/ql/src/RedundantCode/ImpossibleInterfaceNilCheck.ql

@@ -30,21 +30,49 @@ predicate flowsToInterfaceNilCheck(DataFlow::Node nd) {
   flowsToInterfaceNilCheck(nd.getASuccessor())
 }
 
+/**
+ * Holds if wrapping `nd` in an interface produces a non-nil interface value.
+ */
+private predicate isNonNilWrapperSource(DataFlow::Node nd) {
+  exists(Type tp | tp = nd.getType().getUnderlyingType() |
+    not tp instanceof InterfaceType and
+    not tp instanceof NilLiteralType and
+    not tp instanceof InvalidType
+  )
+}
+
+private DataFlow::Node getWrapperPredecessor(DataFlow::Node nd) {
+  result = nd.getAPredecessor() and not result instanceof DataFlow::PostUpdateNode
+}
+
+/**
+ * Holds if local flow into `nd` may originate from a nil or unknown interface
+ * value.
+ */
+private predicate hasPossiblyNilSource(DataFlow::Node nd) {
+  flowsToInterfaceNilCheck(nd) and
+  not isNonNilWrapperSource(nd) and
+  (
+    not exists(getWrapperPredecessor(nd))
+    or
+    // Uncertain writes may receive replacement values through non-local capture flow.
+    nd.(DataFlow::SsaNode).getDefinition() instanceof SsaUncertainWrite
+    or
+    hasPossiblyNilSource(getWrapperPredecessor(nd))
+  )
+}
+
 /**
  * Holds if `nd` is a non-nil interface value.
+ *
+ * A concrete origin and no possibly nil origins establish this even through
+ * SSA cycles.
  */
 predicate nonNilWrapper(DataFlow::Node nd) {
   flowsToInterfaceNilCheck(nd) and
-  forex(DataFlow::Node pred |
-    pred = nd.getAPredecessor() and not pred instanceof DataFlow::PostUpdateNode
-  |
-    exists(Type predtp | predtp = pred.getType().getUnderlyingType() |
-      not predtp instanceof InterfaceType and
-      not predtp instanceof NilLiteralType and
-      not predtp instanceof InvalidType
-    )
-    or
-    nonNilWrapper(pred)
+  not hasPossiblyNilSource(nd) and
+  exists(DataFlow::Node source |
+    source = getWrapperPredecessor*(nd) and isNonNilWrapperSource(source)
   )
 }
 

go/ql/test/experimental/CWE-942/CONSISTENCY/SsaConsistency.expected

@@ -0,0 +1,4 @@
+uselessPhiNode
+| CorsMisconfiguration.go:13:1:229:1 | SSA phi(allowedOrigins) | 1 |
+phiWithoutTwoPriorRefs
+| CorsMisconfiguration.go:13:1:229:1 | SSA phi(allowedOrigins) | CorsMisconfiguration.go:13:1:229:1 | Exceptional Exit | CorsMisconfiguration.go:94:3:94:16 | allowedOrigins | 1 |

go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/CONSISTENCY/SsaConsistency.expected

@@ -0,0 +1,4 @@
+uselessPhiNode
+| epilogues.go:89:3:91:3 | SSA phi(result) | 1 |
+phiWithoutTwoPriorRefs
+| epilogues.go:89:3:91:3 | SSA phi(result) | epilogues.go:89:3:91:3 | After if statement | epilogues.go:87:30:87:35 | result | 1 |

go/ql/test/library-tests/semmle/go/dataflow/InterfaceDispatch/Flows.ql

@@ -0,0 +1,3 @@
+import go
+import utils.test.InlineFlowTest
+import ValueFlowTest<DefaultFlowConfig>

go/ql/test/library-tests/semmle/go/dataflow/InterfaceDispatch/Targets.expected

@@ -0,0 +1,10 @@
+| capturedOverwrite | main.go:20:7:20:15 | call to Greet | main.go:3:6:3:12 | Greeter |
+| capturedOverwrite | main.go:20:7:20:15 | call to Greet | main.go:7:6:7:16 | SafeGreeter |
+| capturedOverwrite | main.go:20:7:20:15 | call to Greet | main.go:8:6:8:19 | TaintedGreeter |
+| overwriteLoop | main.go:38:7:38:15 | call to Greet | main.go:7:6:7:16 | SafeGreeter |
+| overwriteLoop | main.go:38:7:38:15 | call to Greet | main.go:8:6:8:19 | TaintedGreeter |
+| readOnlyLoop | main.go:27:8:27:16 | call to Greet | main.go:7:6:7:16 | SafeGreeter |
+| readOnlyLoop | main.go:30:7:30:15 | call to Greet | main.go:7:6:7:16 | SafeGreeter |
+| unknownReceiver | main.go:45:7:45:15 | call to Greet | main.go:3:6:3:12 | Greeter |
+| unknownReceiver | main.go:45:7:45:15 | call to Greet | main.go:7:6:7:16 | SafeGreeter |
+| unknownReceiver | main.go:45:7:45:15 | call to Greet | main.go:8:6:8:19 | TaintedGreeter |

go/ql/test/library-tests/semmle/go/dataflow/InterfaceDispatch/Targets.ql

@@ -0,0 +1,9 @@
+import go
+import semmle.go.dataflow.internal.DataFlowDispatch
+
+from DataFlow::CallNode call, Callable target
+where
+  call.getTarget().getName() = "Greet" and
+  target = viableCallable(call.asExpr()).asCallable()
+select call.getEnclosingCallable().getName(), call,
+  target.asFunction().(Method).getReceiverBaseType()

go/ql/test/library-tests/semmle/go/dataflow/InterfaceDispatch/main.go

@@ -0,0 +1,48 @@
+package main
+
+type Greeter interface {
+	Greet() string
+}
+
+type SafeGreeter struct{}
+type TaintedGreeter struct{}
+
+func source() string { return "tainted" }
+func sink(string)    {}
+
+func (SafeGreeter) Greet() string    { return "safe" }
+func (TaintedGreeter) Greet() string { return source() }
+
+func capturedOverwrite() {
+	var g Greeter = SafeGreeter{}
+	set := func() { g = TaintedGreeter{} }
+	set()
+	sink(g.Greet()) // $ hasValueFlow="call to Greet"
+}
+
+func readOnlyLoop(b bool) {
+	var g Greeter = SafeGreeter{}
+	for b {
+		x := g
+		sink(x.Greet())
+	}
+	x := g
+	sink(x.Greet())
+}
+
+func overwriteLoop(b bool) {
+	var g Greeter = SafeGreeter{}
+	for b {
+		g = TaintedGreeter{}
+	}
+	sink(g.Greet()) // $ hasValueFlow="call to Greet"
+}
+
+func unknownReceiver(g Greeter, b bool) {
+	for b {
+		g = SafeGreeter{}
+	}
+	sink(g.Greet()) // $ hasValueFlow="call to Greet"
+}
+
+func main() {}

go/ql/test/library-tests/semmle/go/dataflow/SSADataFlowIntegration/CONSISTENCY/SsaConsistency.expected

@@ -0,0 +1,8 @@
+uselessPhiNode
+| main.go:57:9:59:2 | SSA phi(value) | 1 |
+| main.go:64:1:71:1 | SSA phi(value) | 1 |
+| main.go:76:3:78:3 | SSA phi(value) | 1 |
+phiWithoutTwoPriorRefs
+| main.go:57:9:59:2 | SSA phi(value) | main.go:57:9:59:2 | Exit | main.go:56:2:56:6 | value | 1 |
+| main.go:64:1:71:1 | SSA phi(value) | main.go:64:1:71:1 | Exceptional Exit | main.go:65:2:65:6 | value | 1 |
+| main.go:76:3:78:3 | SSA phi(value) | main.go:76:3:78:3 | After if statement | main.go:74:2:74:6 | value | 1 |