Go: Fix missing flow through receiver for function variable (try 2)

go/ql/lib/change-notes/2023-07-19-method-call-node.md

@@ -0,0 +1,4 @@
+---
+category: fix
+---
+* The definition of `DataFlow::MethodCallNode` has been expanded to include `DataFlow::CallNode`s where what is being called is a variable that has a method assigned to it within the calling function. This means we can now follow data flow into the receiver of such a method call.

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

@@ -744,7 +744,7 @@ module IR {
 
     override Type getResultType() {
       exists(CallExpr c | this.getBase() = evalExprInstruction(c) |
-        result = c.getTarget().getResultType(i)
+        result = c.getCalleeType().getResultType(i)
       )
       or
       exists(Expr e | this.getBase() = evalExprInstruction(e) |

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

@@ -8,7 +8,7 @@ private import DataFlowPrivate
 private predicate isInterfaceCallReceiver(
   DataFlow::CallNode call, DataFlow::Node recv, InterfaceType tp, string m
 ) {
-  call.getReceiver() = recv and
+  call.(DataFlow::MethodCallNode).getReceiver() = recv and
   recv.getType().getUnderlyingType() = tp and
   m = call.getACalleeIncludingExternals().asFunction().getName()
 }

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

@@ -445,8 +445,8 @@ module Public {
     CallNode getCallNode() { result = call }
 
     override Type getType() {
-      exists(Function f | f = call.getTarget() |
-        result = f.getParameterType(f.getNumParameter() - 1)
+      exists(SignatureType t | t = call.getCall().getCalleeType() |
+        result = t.getParameterType(t.getNumParameter() - 1)
       )
     }
 
@@ -480,7 +480,11 @@ module Public {
   class CallNode extends ExprNode {
     override CallExpr expr;
 
-    /** Gets the declared target of this call */
+    /**
+     * Gets the declared target of this call, if it exists.
+     *
+     * This doesn't exist when a function is called via a variable.
+     */
     Function getTarget() { result = expr.getTarget() }
 
     private DataFlow::Node getACalleeSource() { result = getACalleeSource(this) }
@@ -637,16 +641,40 @@ module Public {
     /** Gets a result of this call. */
     Node getAResult() { result = this.getResult(_) }
 
-    /** Gets the data flow node corresponding to the receiver of this call, if any. */
+    /**
+     * Gets the data flow node corresponding to the receiver of this call, if any.
+     *
+     * When a method value is assigned to a variable then when it is called it
+     * looks like a function call, as in the following example.
+     * ```go
+     * file, _ := os.Open("test.txt")
+     * f := file.Close
+     * f()
+     * ```
+     * In this case we use local flow to try to find the receiver (`file` in
+     * the  above example).
+     */
     Node getReceiver() { result = this.getACalleeSource().(MethodReadNode).getReceiver() }
 
     /** Holds if this call has an ellipsis after its last argument. */
     predicate hasEllipsis() { expr.hasEllipsis() }
   }
 
-  /** A data flow node that represents a call to a method. */
+  /**
+   * A data flow node that represents a call to a method.
+   *
+   * When a method value is assigned to a variable then when it is called it
+   * syntactically looks like a function call, as in the following example.
+   * ```go
+   * file, _ := os.Open("test.txt")
+   * f := file.Close
+   * f()
+   * ```
+   * In this case we use local flow to see whether a method is assigned to the
+   * function.
+   */
   class MethodCallNode extends CallNode {
-    MethodCallNode() { expr.getTarget() instanceof Method }
+    MethodCallNode() { getACalleeSource(this) instanceof MethodReadNode }
 
     override Method getTarget() { result = expr.getTarget() }
 

go/ql/lib/semmle/go/frameworks/Gqlgen.qll

@@ -7,7 +7,7 @@ module Gqlgen {
   /** An autogenerated file containing gqlgen code. */
   private class GqlgenGeneratedFile extends File {
     GqlgenGeneratedFile() {
-      exists(DataFlow::CallNode call |
+      exists(DataFlow::MethodCallNode call |
         call.getReceiver().getType().hasQualifiedName("github.com/99designs/gqlgen/graphql", _) and
         call.getFile() = this
       )

go/ql/lib/semmle/go/frameworks/stdlib/NetHttp.qll

@@ -131,7 +131,7 @@ module NetHttp {
     )
     or
     stack = SummaryComponentStack::argument(-1) and
-    result = call.getReceiver()
+    result = call.(DataFlow::MethodCallNode).getReceiver()
   }
 
   private class ResponseBody extends Http::ResponseBody::Range {

go/ql/lib/semmle/go/security/ExternalAPIs.qll

@@ -86,7 +86,7 @@ class ExternalApiDataNode extends DataFlow::Node {
       this = call.getArgument(i)
       or
       // Receiver to a call to a method which returns non trivial value
-      this = call.getReceiver() and
+      this = call.(DataFlow::MethodCallNode).getReceiver() and
       i = -1
     ) and
     // Not defined in the code that is being analyzed

go/ql/lib/semmle/go/security/SafeUrlFlowCustomizations.qll

@@ -33,7 +33,9 @@ module SafeUrlFlow {
 
   /** A function model step using `UnsafeUrlMethod`, considered as a sanitizer for safe URL flow. */
   private class UnsafeUrlMethodEdge extends SanitizerEdge {
-    UnsafeUrlMethodEdge() { this = any(UnsafeUrlMethod um).getACall().getReceiver() }
+    UnsafeUrlMethodEdge() {
+      this = any(UnsafeUrlMethod um).getACall().(DataFlow::MethodCallNode).getReceiver()
+    }
   }
 
   /** Any slicing of the URL, considered as a sanitizer for safe URL flow. */

go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql

@@ -89,7 +89,7 @@ predicate isWritableFileHandle(DataFlow::Node source, DataFlow::CallNode call) {
 /**
  * Holds if `os.File.Close` is called on `sink`.
  */
-predicate isCloseSink(DataFlow::Node sink, DataFlow::CallNode closeCall) {
+predicate isCloseSink(DataFlow::Node sink, DataFlow::MethodCallNode closeCall) {
   // find calls to the os.File.Close function
   closeCall = any(CloseFileFun f).getACall() and
   // that are unhandled
@@ -114,7 +114,7 @@ predicate isCloseSink(DataFlow::Node sink, DataFlow::CallNode closeCall) {
  * Holds if `os.File.Sync` is called on `sink` and the result of the call is neither
  * deferred nor discarded.
  */
-predicate isHandledSync(DataFlow::Node sink, DataFlow::CallNode syncCall) {
+predicate isHandledSync(DataFlow::Node sink, DataFlow::MethodCallNode syncCall) {
   // find a call of the `os.File.Sync` function
   syncCall = any(SyncFileFun f).getACall() and
   // match the sink with the object on which the method is called

go/ql/src/Security/CWE-352/ConstantOauth2State.ql

@@ -105,7 +105,7 @@ module PrivateUrlFlowsToAuthCodeUrlCallConfig implements DataFlow::ConfigSig {
     )
   }
 
-  additional predicate isSinkCall(DataFlow::Node sink, DataFlow::CallNode call) {
+  additional predicate isSinkCall(DataFlow::Node sink, DataFlow::MethodCallNode call) {
     exists(AuthCodeUrl m | call = m.getACall() | sink = call.getReceiver())
   }
 

go/ql/src/experimental/CWE-1004/AuthCookie.qll

@@ -275,11 +275,11 @@ deprecated class GorillaCookieStoreSaveTrackingConfiguration extends DataFlow::C
   }
 
   override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
-    exists(DataFlow::MethodCallNode cn |
-      cn.getTarget()
+    exists(DataFlow::MethodCallNode mcn |
+      mcn.getTarget()
           .hasQualifiedName(package("github.com/gorilla/sessions", ""), "CookieStore", "Get") and
-      pred = cn.getReceiver() and
-      succ = cn.getResult(0)
+      pred = mcn.getReceiver() and
+      succ = mcn.getResult(0)
     )
   }
 }

go/ql/src/experimental/CWE-285/PamAuthBypass.ql

@@ -40,7 +40,7 @@ module PamStartToAcctMgmtConfig implements DataFlow::ConfigSig {
   }
 
   predicate isSink(DataFlow::Node sink) {
-    exists(PamAcctMgmt p | p.getACall().getReceiver() = sink)
+    exists(PamAcctMgmt p | p.getACall().(DataFlow::MethodCallNode).getReceiver() = sink)
   }
 }
 
@@ -53,7 +53,7 @@ module PamStartToAuthenticateConfig implements DataFlow::ConfigSig {
   }
 
   predicate isSink(DataFlow::Node sink) {
-    exists(PamAuthenticate p | p.getACall().getReceiver() = sink)
+    exists(PamAuthenticate p | p.getACall().(DataFlow::MethodCallNode).getReceiver() = sink)
   }
 }
 

go/ql/src/experimental/frameworks/CleverGo.qll

@@ -174,7 +174,7 @@ private module CleverGo {
   /**
    * Models HTTP redirects.
    */
-  private class HttpRedirect extends Http::Redirect::Range, DataFlow::CallNode {
+  private class HttpRedirect extends Http::Redirect::Range, DataFlow::MethodCallNode {
     DataFlow::Node urlNode;
 
     HttpRedirect() {
@@ -211,7 +211,7 @@ private module CleverGo {
     string package, string receiverName, DataFlow::Node bodyNode, string contentTypeString,
     DataFlow::Node receiverNode
   ) {
-    exists(string methodName, Method met, DataFlow::CallNode bodySetterCall |
+    exists(string methodName, Method met, DataFlow::MethodCallNode bodySetterCall |
       met.hasQualifiedName(package, receiverName, methodName) and
       bodySetterCall = met.getACall() and
       receiverNode = bodySetterCall.getReceiver()
@@ -317,7 +317,7 @@ private module CleverGo {
     string package, string receiverName, DataFlow::Node bodyNode, DataFlow::Node contentTypeNode,
     DataFlow::Node receiverNode
   ) {
-    exists(string methodName, Method met, DataFlow::CallNode bodySetterCall |
+    exists(string methodName, Method met, DataFlow::MethodCallNode bodySetterCall |
       met.hasQualifiedName(package, receiverName, methodName) and
       bodySetterCall = met.getACall() and
       receiverNode = bodySetterCall.getReceiver()
@@ -356,7 +356,7 @@ private module CleverGo {
   private predicate setsBody(
     string package, string receiverName, DataFlow::Node receiverNode, DataFlow::Node bodyNode
   ) {
-    exists(string methodName, Method met, DataFlow::CallNode bodySetterCall |
+    exists(string methodName, Method met, DataFlow::MethodCallNode bodySetterCall |
       met.hasQualifiedName(package, receiverName, methodName) and
       bodySetterCall = met.getACall() and
       receiverNode = bodySetterCall.getReceiver()
@@ -400,7 +400,7 @@ private module CleverGo {
 
   // Holds for a call that sets a header with a key-value combination.
   private predicate setsHeaderDynamicKeyValue(
-    string package, string receiverName, DataFlow::CallNode headerSetterCall,
+    string package, string receiverName, DataFlow::MethodCallNode headerSetterCall,
     DataFlow::Node headerNameNode, DataFlow::Node headerValueNode, DataFlow::Node receiverNode
   ) {
     exists(string methodName, Method met |
@@ -446,7 +446,7 @@ private module CleverGo {
 
   // Holds for a call that sets the content-type header (implicit).
   private predicate setsStaticHeaderContentType(
-    string package, string receiverName, DataFlow::CallNode setterCall, string valueString,
+    string package, string receiverName, DataFlow::MethodCallNode setterCall, string valueString,
     DataFlow::Node receiverNode
   ) {
     exists(string methodName, Method met |
@@ -501,8 +501,8 @@ private module CleverGo {
 
   // Holds for a call that sets the content-type header via a parameter.
   private predicate setsDynamicHeaderContentType(
-    string package, string receiverName, DataFlow::CallNode setterCall, DataFlow::Node valueNode,
-    DataFlow::Node receiverNode
+    string package, string receiverName, DataFlow::MethodCallNode setterCall,
+    DataFlow::Node valueNode, DataFlow::Node receiverNode
   ) {
     exists(string methodName, Method met |
       met.hasQualifiedName(package, receiverName, methodName) and

go/ql/src/experimental/frameworks/Fiber.qll

@@ -129,7 +129,7 @@ private module Fiber {
   /**
    * Models HTTP redirects.
    */
-  private class Redirect extends Http::Redirect::Range, DataFlow::CallNode {
+  private class Redirect extends Http::Redirect::Range, DataFlow::MethodCallNode {
     DataFlow::Node urlNode;
 
     Redirect() {
@@ -167,7 +167,7 @@ private module Fiber {
 
   // Holds for a call that sets a header with a key-value combination.
   private predicate setsHeaderDynamicKeyValue(
-    string package, string receiverName, DataFlow::CallNode headerSetterCall,
+    string package, string receiverName, DataFlow::MethodCallNode headerSetterCall,
     DataFlow::Node headerNameNode, DataFlow::Node headerValueNode, DataFlow::Node receiverNode
   ) {
     exists(string methodName, Method met |
@@ -215,7 +215,7 @@ private module Fiber {
     string package, string receiverName, DataFlow::Node bodyNode, string contentTypeString,
     DataFlow::Node receiverNode
   ) {
-    exists(string methodName, Method met, DataFlow::CallNode bodySetterCall |
+    exists(string methodName, Method met, DataFlow::MethodCallNode bodySetterCall |
       met.hasQualifiedName(package, receiverName, methodName) and
       bodySetterCall = met.getACall() and
       receiverNode = bodySetterCall.getReceiver()
@@ -254,7 +254,7 @@ private module Fiber {
   private predicate setsBody(
     string package, string receiverName, DataFlow::Node receiverNode, DataFlow::Node bodyNode
   ) {
-    exists(string methodName, Method met, DataFlow::CallNode bodySetterCall |
+    exists(string methodName, Method met, DataFlow::MethodCallNode bodySetterCall |
       met.hasQualifiedName(package, receiverName, methodName) and
       bodySetterCall = met.getACall() and
       receiverNode = bodySetterCall.getReceiver()

go/ql/test/library-tests/semmle/go/dataflow/GenericFunctionsAndTypes/generictypesandmethods.go

@@ -51,7 +51,7 @@ func main() {
 		gs1 := GenericStruct1[string]{""}
 		gs1.Field = source()
 		f := gs1.Getter
-		sink(f()) // $ MISSING: hasValueFlow="call to f"
+		sink(f()) // $ hasValueFlow="call to f"
 	}
 	{
 		gs1 := GenericStruct1[string]{""}
@@ -62,7 +62,7 @@ func main() {
 		gs1 := GenericStruct1[string]{""}
 		f := gs1.Setter
 		f(source())
-		sink(gs1.Field) // $ MISSING: hasValueFlow="selection of Field"
+		sink(gs1.Field) // $ hasValueFlow="selection of Field"
 	}
 
 	{
@@ -87,7 +87,7 @@ func main() {
 		gs2 := GenericStruct2[string, string]{"", ""}
 		gs2.Field1 = source()
 		f := gs2.Getter1
-		sink(f()) // $ MISSING: hasValueFlow="call to f"
+		sink(f()) // $ hasValueFlow="call to f"
 	}
 	{
 		gs2 := GenericStruct2[string, string]{"", ""}
@@ -98,7 +98,7 @@ func main() {
 		gs2 := GenericStruct2[string, string]{"", ""}
 		f := gs2.Setter1
 		f(source())
-		sink(gs2.Field1) // $ MISSING: hasValueFlow="selection of Field1"
+		sink(gs2.Field1) // $ hasValueFlow="selection of Field1"
 	}
 
 	{
@@ -123,7 +123,7 @@ func main() {
 		gs2 := GenericStruct2[string, string]{"", ""}
 		gs2.Field2 = source()
 		f := gs2.Getter2
-		sink(f()) // $ MISSING: hasValueFlow="call to f"
+		sink(f()) // $ hasValueFlow="call to f"
 	}
 	{
 		gs2 := GenericStruct2[string, string]{"", ""}
@@ -134,6 +134,6 @@ func main() {
 		gs2 := GenericStruct2[string, string]{"", ""}
 		f := gs2.Setter2
 		f(source())
-		sink(gs2.Field2) // $ MISSING: hasValueFlow="selection of Field2"
+		sink(gs2.Field2) // $ hasValueFlow="selection of Field2"
 	}
 }

go/ql/test/library-tests/semmle/go/frameworks/Yaml/tests.ql

@@ -13,9 +13,10 @@ DataFlow::CallNode getAYamlCall() {
 
 predicate isSourceSinkPair(DataFlow::Node inNode, DataFlow::Node outNode) {
   exists(DataFlow::CallNode cn | cn = getAYamlCall() |
-    inNode = [cn.getAnArgument(), cn.getReceiver()] and
+    inNode = [cn.getAnArgument(), cn.(DataFlow::MethodCallNode).getReceiver()] and
     (
-      outNode.(DataFlow::PostUpdateNode).getPreUpdateNode() = [cn.getAnArgument(), cn.getReceiver()]
+      outNode.(DataFlow::PostUpdateNode).getPreUpdateNode() =
+        [cn.getAnArgument(), cn.(DataFlow::MethodCallNode).getReceiver()]
       or
       outNode = cn.getAResult()
     )