Merge pull request #219 from max-schaefer/refine-virtual-dispatch

Refine potential targets for method call through interface

Author: max-schaefer

change-notes/2020-06-19-call-graph.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Resolution of method calls through interfaces has been improved, resulting in more precise call-graph information, which in turn may eliminate false positives from the security queries.

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

@@ -2,18 +2,20 @@ private import go
 private import DataFlowPrivate
 
 /**
- * Holds if `call` is an interface call to method `m`, meaning that its receiver `recv` has an
- * interface type.
+ * Holds if `call` is an interface call to method `m`, meaning that its receiver `recv` has
+ * interface type `tp`.
  */
-private predicate isInterfaceCallReceiver(DataFlow::CallNode call, DataFlow::Node recv, string m) {
+private predicate isInterfaceCallReceiver(
+  DataFlow::CallNode call, DataFlow::Node recv, InterfaceType tp, string m
+) {
   call.getReceiver() = recv and
-  recv.getType().getUnderlyingType() instanceof InterfaceType and
+  recv.getType().getUnderlyingType() = tp and
   m = call.getCalleeName()
 }
 
 /** Gets a data-flow node that may flow into the receiver value of `call`, which is an interface value. */
 private DataFlow::Node getInterfaceCallReceiverSource(DataFlow::CallNode call) {
-  isInterfaceCallReceiver(call, result.getASuccessor*(), _)
+  isInterfaceCallReceiver(call, result.getASuccessor*(), _, _)
 }
 
 /** Gets the type of `nd`, which must be a valid type and not an interface type. */
@@ -44,7 +46,7 @@ private predicate isConcreteValue(DataFlow::Node nd) {
  * types of `recv` can be established by local reasoning.
  */
 private predicate isConcreteInterfaceCall(DataFlow::Node call, DataFlow::Node recv, string m) {
-  isInterfaceCallReceiver(call, recv, m) and isConcreteValue(recv)
+  isInterfaceCallReceiver(call, recv, _, m) and isConcreteValue(recv)
 }
 
 /**
@@ -61,14 +63,36 @@ private FuncDecl getConcreteTarget(DataFlow::CallNode call) {
   )
 }
 
+/**
+ * Holds if `call` is a method call whose receiver has an interface type.
+ */
+private predicate isInterfaceMethodCall(DataFlow::CallNode call) {
+  isInterfaceCallReceiver(call, _, _, _)
+}
+
+/**
+ * Gets a method that might be called by `call`, where we restrict the result to
+ * implement the interface type of the receiver of `call`.
+ */
+private MethodDecl getRestrictedInterfaceTarget(DataFlow::CallNode call) {
+  exists(InterfaceType tp, Type recvtp, string m |
+    isInterfaceCallReceiver(call, _, tp, m) and
+    result = recvtp.getMethod(m).(DeclaredFunction).getFuncDecl() and
+    recvtp.implements(tp)
+  )
+}
+
 /**
  * Gets a function that might be called by `call`.
  */
 DataFlowCallable viableCallable(CallExpr ma) {
   exists(DataFlow::CallNode call | call.asExpr() = ma |
     if isConcreteInterfaceCall(call, _, _)
     then result = getConcreteTarget(call)
-    else result = call.getACallee()
+    else
+      if isInterfaceMethodCall(call)
+      then result = getRestrictedInterfaceTarget(call)
+      else result = call.getACallee()
   )
 }
 

ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll

@@ -64,6 +64,21 @@ class Node extends TNode {
     endcolumn = 0
   }
 
+  /** Gets the file in which this node appears. */
+  File getFile() { hasLocationInfo(result.getAbsolutePath(), _, _, _, _) }
+
+  /** Gets the start line of the location of this node. */
+  int getStartLine() { hasLocationInfo(_, result, _, _, _) }
+
+  /** Gets the start column of the location of this node. */
+  int getStartColumn() { hasLocationInfo(_, _, result, _, _) }
+
+  /** Gets the end line of the location of this node. */
+  int getEndLine() { hasLocationInfo(_, _, _, result, _) }
+
+  /** Gets the end column of the location of this node. */
+  int getEndColumn() { hasLocationInfo(_, _, _, _, result) }
+
   /**
    * Gets an upper bound on the type of this node.
    */

ql/test/library-tests/semmle/go/dataflow/CallGraph/getACallee.expected

@@ -3,3 +3,4 @@ spuriousCallee
 | main.go:44:3:44:7 | call to m | main.go:17:1:17:17 | function declaration |
 | main.go:44:3:44:7 | call to m | main.go:21:1:21:20 | function declaration |
 | main.go:56:2:56:6 | call to m | main.go:21:1:21:20 | function declaration |
+| test.go:42:2:42:13 | call to Write | test.go:36:1:39:1 | function declaration |

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

@@ -0,0 +1,49 @@
+package main
+
+import (
+	"fmt"
+	"hash"
+	"io"
+)
+
+type Resetter struct{}
+
+func (_ Resetter) Reset() {} // name: Resetter.Reset
+
+type MockHash struct {
+	Resetter
+}
+
+func (_ MockHash) Write(p []byte) (n int, err error) { // name: MockHash.Write
+	fmt.Println("MockHash.Write")
+	return 0, nil
+}
+
+func (_ MockHash) Sum(b []byte) []byte {
+	return nil
+}
+
+func (_ MockHash) Size() int {
+	return 0
+}
+
+func (_ MockHash) BlockSize() int {
+	return 0
+}
+
+type MockWriter struct{}
+
+func (_ MockWriter) Write(p []byte) (n int, err error) { // name: MockWriter.Write
+	fmt.Println("MockWriter.Write")
+	return 0, nil
+}
+
+func test5(h hash.Hash, w io.Writer) { // name: test5
+	h.Write(nil) // callee: MockHash.Write
+	w.Write(nil) // callee: MockWriter.Write callee: MockHash.Write
+	h.Reset()    // callee: Resetter.Reset
+}
+
+func test6(h MockHash, w MockWriter) {
+	test5(h, w) // callee: test5
+}