Merge pull request #10085 from smowton/smowton/fix/dont-use-write-instruction-for-channel-flow

Go: don't use WriteNode for channel writes

Author: smowton

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

@@ -166,21 +166,11 @@ module ControlFlow {
       )
     }
 
-    /**
-     * Holds if this node writes `rhs` to `channel`.
-     */
-    predicate writesToChannel(DataFlow::ExprNode channel, DataFlow::ExprNode rhs) {
-      exists(SendStmt send |
-        send.getChannel() = channel.asExpr() and
-        send.getValue() = rhs.asExpr()
-      )
-    }
-
     /**
      * Holds if this node sets any field or element of `base` to `rhs`.
      */
     predicate writesComponent(DataFlow::Node base, DataFlow::Node rhs) {
-      writesElement(base, _, rhs) or writesField(base, _, rhs) or writesToChannel(base, rhs)
+      writesElement(base, _, rhs) or writesField(base, _, rhs)
     }
   }
 

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

@@ -24,7 +24,9 @@ predicate containerStoreStep(Node node1, Node node2, Content c) {
   )
   or
   c instanceof CollectionContent and
-  exists(Write w | w.writesToChannel(node2, node1))
+  exists(SendStmt send |
+    send.getChannel() = node2.(ExprNode).asExpr() and send.getValue() = node1.(ExprNode).asExpr()
+  )
   or
   c instanceof MapKeyContent and
   node2.getType() instanceof MapType and

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

@@ -634,7 +634,10 @@ module Public {
     predicate isReceiverOf(MethodDecl m) { parm.isReceiverOf(m) }
   }
 
-  private Node getADirectlyWrittenNode() { exists(Write w | w.writesComponent(result, _)) }
+  private Node getADirectlyWrittenNode() {
+    exists(Write w | w.writesComponent(result, _)) or
+    result = DataFlow::exprNode(any(SendStmt s).getChannel())
+  }
 
   private DataFlow::Node getAccessPathPredecessor(DataFlow::Node node) {
     result = node.(PointerDereferenceNode).getOperand()

go/ql/test/library-tests/semmle/go/dataflow/ChannelField/test.expected

@@ -0,0 +1,22 @@
+edges
+| test.go:9:9:9:11 | selection of c [collection] : string | test.go:9:7:9:11 | <-... |
+| test.go:13:16:13:16 | definition of s [pointer, c, collection] : string | test.go:16:2:16:2 | s [pointer, c, collection] : string |
+| test.go:15:10:15:17 | call to source : string | test.go:16:9:16:12 | data : string |
+| test.go:16:2:16:2 | implicit dereference [c, collection] : string | test.go:13:16:13:16 | definition of s [pointer, c, collection] : string |
+| test.go:16:2:16:2 | implicit dereference [c, collection] : string | test.go:16:2:16:4 | selection of c [collection] : string |
+| test.go:16:2:16:2 | s [pointer, c, collection] : string | test.go:16:2:16:2 | implicit dereference [c, collection] : string |
+| test.go:16:2:16:4 | selection of c [collection] : string | test.go:9:9:9:11 | selection of c [collection] : string |
+| test.go:16:2:16:4 | selection of c [collection] : string | test.go:16:2:16:2 | implicit dereference [c, collection] : string |
+| test.go:16:9:16:12 | data : string | test.go:16:2:16:4 | selection of c [collection] : string |
+nodes
+| test.go:9:7:9:11 | <-... | semmle.label | <-... |
+| test.go:9:9:9:11 | selection of c [collection] : string | semmle.label | selection of c [collection] : string |
+| test.go:13:16:13:16 | definition of s [pointer, c, collection] : string | semmle.label | definition of s [pointer, c, collection] : string |
+| test.go:15:10:15:17 | call to source : string | semmle.label | call to source : string |
+| test.go:16:2:16:2 | implicit dereference [c, collection] : string | semmle.label | implicit dereference [c, collection] : string |
+| test.go:16:2:16:2 | s [pointer, c, collection] : string | semmle.label | s [pointer, c, collection] : string |
+| test.go:16:2:16:4 | selection of c [collection] : string | semmle.label | selection of c [collection] : string |
+| test.go:16:9:16:12 | data : string | semmle.label | data : string |
+subpaths
+#select
+| test.go:15:10:15:17 | call to source : string | test.go:15:10:15:17 | call to source : string | test.go:9:7:9:11 | <-... | path |

go/ql/test/library-tests/semmle/go/dataflow/ChannelField/test.go

@@ -0,0 +1,26 @@
+package test
+
+type State struct {
+	c chan string
+}
+
+func handler(s *State) {
+
+	sink(<-s.c)
+
+}
+
+func requester(s *State) {
+
+	data := source()
+	s.c <- data
+
+}
+
+func source() string {
+
+	return "tainted"
+
+}
+
+func sink(s string) {}

go/ql/test/library-tests/semmle/go/dataflow/ChannelField/test.ql

@@ -0,0 +1,18 @@
+import go
+import DataFlow::PathGraph
+
+class TestConfig extends DataFlow::Configuration {
+  TestConfig() { this = "test config" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source.(DataFlow::CallNode).getTarget().getName() = "source"
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sink = any(DataFlow::CallNode c | c.getTarget().getName() = "sink").getAnArgument()
+  }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, TestConfig c
+where c.hasFlowPath(source, sink)
+select source, source, sink, "path"