Skip to content

Commit 0a0e9bb

Browse files
owen-mcowen-mc
authored
Merge pull request #13767 from owen-mc/go/missing-flow-through-receiver
Go: Fix missing flow through receiver for function variable
2 parents a9c76d4 + b9027a0 commit 0a0e9bb

File tree

16 files changed

+79
-42
lines changed

16 files changed

+79
-42
lines changed

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: fix
3+
---
4+
* 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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -744,7 +744,7 @@ module IR {
744744

745745
override Type getResultType() {
746746
exists(CallExpr c | this.getBase() = evalExprInstruction(c) |
747-
result = c.getTarget().getResultType(i)
747+
result = c.getCalleeType().getResultType(i)
748748
)
749749
or
750750
exists(Expr e | this.getBase() = evalExprInstruction(e) |

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ private import DataFlowPrivate
88
private predicate isInterfaceCallReceiver(
99
DataFlow::CallNode call, DataFlow::Node recv, InterfaceType tp, string m
1010
) {
11-
call.getReceiver() = recv and
11+
call.(DataFlow::MethodCallNode).getReceiver() = recv and
1212
recv.getType().getUnderlyingType() = tp and
1313
m = call.getACalleeIncludingExternals().asFunction().getName()
1414
}
@@ -149,7 +149,9 @@ predicate golangSpecificParamArgFilter(
149149
// Interface methods calls may be passed strictly to that exact method's model receiver:
150150
arg.getPosition() != -1
151151
or
152-
exists(Function callTarget | callTarget = call.getNode().(DataFlow::CallNode).getTarget() |
152+
exists(Function callTarget |
153+
callTarget = call.getNode().(DataFlow::CallNode).getACalleeIncludingExternals().asFunction()
154+
|
153155
not isInterfaceMethod(callTarget)
154156
or
155157
callTarget = p.getCallable().asSummarizedCallable().asFunction()

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

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -439,8 +439,8 @@ module Public {
439439
CallNode getCallNode() { result = call }
440440

441441
override Type getType() {
442-
exists(Function f | f = call.getTarget() |
443-
result = f.getParameterType(f.getNumParameter() - 1)
442+
exists(SignatureType t | t = call.getCall().getCalleeType() |
443+
result = t.getParameterType(t.getNumParameter() - 1)
444444
)
445445
}
446446

@@ -474,7 +474,11 @@ module Public {
474474
class CallNode extends ExprNode {
475475
override CallExpr expr;
476476

477-
/** Gets the declared target of this call */
477+
/**
478+
* Gets the declared target of this call, if it exists.
479+
*
480+
* This doesn't exist when a function is called via a variable.
481+
*/
478482
Function getTarget() { result = expr.getTarget() }
479483

480484
private DataFlow::Node getACalleeSource() { result = getACalleeSource(this) }
@@ -622,16 +626,40 @@ module Public {
622626
/** Gets a result of this call. */
623627
Node getAResult() { result = this.getResult(_) }
624628

625-
/** Gets the data flow node corresponding to the receiver of this call, if any. */
629+
/**
630+
* Gets the data flow node corresponding to the receiver of this call, if any.
631+
*
632+
* When a method value is assigned to a variable then when it is called it
633+
* looks like a function call, as in the following example.
634+
* ```go
635+
* file, _ := os.Open("test.txt")
636+
* f := file.Close
637+
* f()
638+
* ```
639+
* In this case we use local flow to try to find the receiver (`file` in
640+
* the above example).
641+
*/
626642
Node getReceiver() { result = this.getACalleeSource().(MethodReadNode).getReceiver() }
627643

628644
/** Holds if this call has an ellipsis after its last argument. */
629645
predicate hasEllipsis() { expr.hasEllipsis() }
630646
}
631647

632-
/** A data flow node that represents a call to a method. */
648+
/**
649+
* A data flow node that represents a call to a method.
650+
*
651+
* When a method value is assigned to a variable then when it is called it
652+
* syntactically looks like a function call, as in the following example.
653+
* ```go
654+
* file, _ := os.Open("test.txt")
655+
* f := file.Close
656+
* f()
657+
* ```
658+
* In this case we use local flow to see whether a method is assigned to the
659+
* function.
660+
*/
633661
class MethodCallNode extends CallNode {
634-
MethodCallNode() { expr.getTarget() instanceof Method }
662+
MethodCallNode() { getACalleeSource(this) instanceof MethodReadNode }
635663

636664
override Method getTarget() { result = expr.getTarget() }
637665

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ module Gqlgen {
77
/** An autogenerated file containing gqlgen code. */
88
private class GqlgenGeneratedFile extends File {
99
GqlgenGeneratedFile() {
10-
exists(DataFlow::CallNode call |
10+
exists(DataFlow::MethodCallNode call |
1111
call.getReceiver().getType().hasQualifiedName("github.com/99designs/gqlgen/graphql", _) and
1212
call.getFile() = this
1313
)

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ module NetHttp {
131131
)
132132
or
133133
stack = SummaryComponentStack::argument(-1) and
134-
result = call.getReceiver()
134+
result = call.(DataFlow::MethodCallNode).getReceiver()
135135
}
136136

137137
private class ResponseBody extends Http::ResponseBody::Range {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ class ExternalApiDataNode extends DataFlow::Node {
8686
this = call.getArgument(i)
8787
or
8888
// Receiver to a call to a method which returns non trivial value
89-
this = call.getReceiver() and
89+
this = call.(DataFlow::MethodCallNode).getReceiver() and
9090
i = -1
9191
) and
9292
// Not defined in the code that is being analyzed

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,9 @@ module SafeUrlFlow {
3333

3434
/** A function model step using `UnsafeUrlMethod`, considered as a sanitizer for safe URL flow. */
3535
private class UnsafeUrlMethodEdge extends SanitizerEdge {
36-
UnsafeUrlMethodEdge() { this = any(UnsafeUrlMethod um).getACall().getReceiver() }
36+
UnsafeUrlMethodEdge() {
37+
this = any(UnsafeUrlMethod um).getACall().(DataFlow::MethodCallNode).getReceiver()
38+
}
3739
}
3840

3941
/** Any slicing of the URL, considered as a sanitizer for safe URL flow. */

‎go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ predicate isWritableFileHandle(DataFlow::Node source, DataFlow::CallNode call) {
9090
/**
9191
* Holds if `os.File.Close` is called on `sink`.
9292
*/
93-
predicate isCloseSink(DataFlow::Node sink, DataFlow::CallNode closeCall) {
93+
predicate isCloseSink(DataFlow::Node sink, DataFlow::MethodCallNode closeCall) {
9494
// find calls to the os.File.Close function
9595
closeCall = any(CloseFileFun f).getACall() and
9696
// that are unhandled
@@ -115,7 +115,7 @@ predicate isCloseSink(DataFlow::Node sink, DataFlow::CallNode closeCall) {
115115
* Holds if `os.File.Sync` is called on `sink` and the result of the call is neither
116116
* deferred nor discarded.
117117
*/
118-
predicate isHandledSync(DataFlow::Node sink, DataFlow::CallNode syncCall) {
118+
predicate isHandledSync(DataFlow::Node sink, DataFlow::MethodCallNode syncCall) {
119119
// find a call of the `os.File.Sync` function
120120
syncCall = any(SyncFileFun f).getACall() and
121121
// match the sink with the object on which the method is called

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ class PrivateUrlFlowsToAuthCodeUrlCall extends DataFlow::Configuration {
113113
)
114114
}
115115

116-
predicate isSinkCall(DataFlow::Node sink, DataFlow::CallNode call) {
116+
predicate isSinkCall(DataFlow::Node sink, DataFlow::MethodCallNode call) {
117117
exists(AuthCodeUrl m | call = m.getACall() | sink = call.getReceiver())
118118
}
119119

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -189,11 +189,11 @@ class GorillaCookieStoreSaveTrackingConfiguration extends DataFlow::Configuratio
189189
}
190190

191191
override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
192-
exists(DataFlow::MethodCallNode cn |
193-
cn.getTarget()
192+
exists(DataFlow::MethodCallNode mcn |
193+
mcn.getTarget()
194194
.hasQualifiedName(package("github.com/gorilla/sessions", ""), "CookieStore", "Get") and
195-
pred = cn.getReceiver() and
196-
succ = cn.getResult(0)
195+
pred = mcn.getReceiver() and
196+
succ = mcn.getResult(0)
197197
)
198198
}
199199
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ class PamStartToAcctMgmtConfig extends TaintTracking::Configuration {
4141
}
4242

4343
override predicate isSink(DataFlow::Node sink) {
44-
exists(PamAcctMgmt p | p.getACall().getReceiver() = sink)
44+
exists(PamAcctMgmt p | p.getACall().(DataFlow::MethodCallNode).getReceiver() = sink)
4545
}
4646
}
4747

@@ -53,7 +53,7 @@ class PamStartToAuthenticateConfig extends TaintTracking::Configuration {
5353
}
5454

5555
override predicate isSink(DataFlow::Node sink) {
56-
exists(PamAuthenticate p | p.getACall().getReceiver() = sink)
56+
exists(PamAuthenticate p | p.getACall().(DataFlow::MethodCallNode).getReceiver() = sink)
5757
}
5858
}
5959

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

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ private module CleverGo {
174174
/**
175175
* Models HTTP redirects.
176176
*/
177-
private class HttpRedirect extends Http::Redirect::Range, DataFlow::CallNode {
177+
private class HttpRedirect extends Http::Redirect::Range, DataFlow::MethodCallNode {
178178
DataFlow::Node urlNode;
179179

180180
HttpRedirect() {
@@ -211,7 +211,7 @@ private module CleverGo {
211211
string package, string receiverName, DataFlow::Node bodyNode, string contentTypeString,
212212
DataFlow::Node receiverNode
213213
) {
214-
exists(string methodName, Method met, DataFlow::CallNode bodySetterCall |
214+
exists(string methodName, Method met, DataFlow::MethodCallNode bodySetterCall |
215215
met.hasQualifiedName(package, receiverName, methodName) and
216216
bodySetterCall = met.getACall() and
217217
receiverNode = bodySetterCall.getReceiver()
@@ -317,7 +317,7 @@ private module CleverGo {
317317
string package, string receiverName, DataFlow::Node bodyNode, DataFlow::Node contentTypeNode,
318318
DataFlow::Node receiverNode
319319
) {
320-
exists(string methodName, Method met, DataFlow::CallNode bodySetterCall |
320+
exists(string methodName, Method met, DataFlow::MethodCallNode bodySetterCall |
321321
met.hasQualifiedName(package, receiverName, methodName) and
322322
bodySetterCall = met.getACall() and
323323
receiverNode = bodySetterCall.getReceiver()
@@ -356,7 +356,7 @@ private module CleverGo {
356356
private predicate setsBody(
357357
string package, string receiverName, DataFlow::Node receiverNode, DataFlow::Node bodyNode
358358
) {
359-
exists(string methodName, Method met, DataFlow::CallNode bodySetterCall |
359+
exists(string methodName, Method met, DataFlow::MethodCallNode bodySetterCall |
360360
met.hasQualifiedName(package, receiverName, methodName) and
361361
bodySetterCall = met.getACall() and
362362
receiverNode = bodySetterCall.getReceiver()
@@ -400,7 +400,7 @@ private module CleverGo {
400400

401401
// Holds for a call that sets a header with a key-value combination.
402402
private predicate setsHeaderDynamicKeyValue(
403-
string package, string receiverName, DataFlow::CallNode headerSetterCall,
403+
string package, string receiverName, DataFlow::MethodCallNode headerSetterCall,
404404
DataFlow::Node headerNameNode, DataFlow::Node headerValueNode, DataFlow::Node receiverNode
405405
) {
406406
exists(string methodName, Method met |
@@ -446,7 +446,7 @@ private module CleverGo {
446446

447447
// Holds for a call that sets the content-type header (implicit).
448448
private predicate setsStaticHeaderContentType(
449-
string package, string receiverName, DataFlow::CallNode setterCall, string valueString,
449+
string package, string receiverName, DataFlow::MethodCallNode setterCall, string valueString,
450450
DataFlow::Node receiverNode
451451
) {
452452
exists(string methodName, Method met |
@@ -501,8 +501,8 @@ private module CleverGo {
501501

502502
// Holds for a call that sets the content-type header via a parameter.
503503
private predicate setsDynamicHeaderContentType(
504-
string package, string receiverName, DataFlow::CallNode setterCall, DataFlow::Node valueNode,
505-
DataFlow::Node receiverNode
504+
string package, string receiverName, DataFlow::MethodCallNode setterCall,
505+
DataFlow::Node valueNode, DataFlow::Node receiverNode
506506
) {
507507
exists(string methodName, Method met |
508508
met.hasQualifiedName(package, receiverName, methodName) and

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ private module Fiber {
129129
/**
130130
* Models HTTP redirects.
131131
*/
132-
private class Redirect extends Http::Redirect::Range, DataFlow::CallNode {
132+
private class Redirect extends Http::Redirect::Range, DataFlow::MethodCallNode {
133133
DataFlow::Node urlNode;
134134

135135
Redirect() {
@@ -167,7 +167,7 @@ private module Fiber {
167167

168168
// Holds for a call that sets a header with a key-value combination.
169169
private predicate setsHeaderDynamicKeyValue(
170-
string package, string receiverName, DataFlow::CallNode headerSetterCall,
170+
string package, string receiverName, DataFlow::MethodCallNode headerSetterCall,
171171
DataFlow::Node headerNameNode, DataFlow::Node headerValueNode, DataFlow::Node receiverNode
172172
) {
173173
exists(string methodName, Method met |
@@ -215,7 +215,7 @@ private module Fiber {
215215
string package, string receiverName, DataFlow::Node bodyNode, string contentTypeString,
216216
DataFlow::Node receiverNode
217217
) {
218-
exists(string methodName, Method met, DataFlow::CallNode bodySetterCall |
218+
exists(string methodName, Method met, DataFlow::MethodCallNode bodySetterCall |
219219
met.hasQualifiedName(package, receiverName, methodName) and
220220
bodySetterCall = met.getACall() and
221221
receiverNode = bodySetterCall.getReceiver()
@@ -254,7 +254,7 @@ private module Fiber {
254254
private predicate setsBody(
255255
string package, string receiverName, DataFlow::Node receiverNode, DataFlow::Node bodyNode
256256
) {
257-
exists(string methodName, Method met, DataFlow::CallNode bodySetterCall |
257+
exists(string methodName, Method met, DataFlow::MethodCallNode bodySetterCall |
258258
met.hasQualifiedName(package, receiverName, methodName) and
259259
bodySetterCall = met.getACall() and
260260
receiverNode = bodySetterCall.getReceiver()

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func main() {
5151
gs1 := GenericStruct1[string]{""}
5252
gs1.Field = source()
5353
f := gs1.Getter
54-
sink(f()) // $ MISSING: hasValueFlow="call to f"
54+
sink(f()) // $ hasValueFlow="call to f"
5555
}
5656
{
5757
gs1 := GenericStruct1[string]{""}
@@ -62,7 +62,7 @@ func main() {
6262
gs1 := GenericStruct1[string]{""}
6363
f := gs1.Setter
6464
f(source())
65-
sink(gs1.Field) // $ MISSING: hasValueFlow="selection of Field"
65+
sink(gs1.Field) // $ hasValueFlow="selection of Field"
6666
}
6767

6868
{
@@ -87,7 +87,7 @@ func main() {
8787
gs2 := GenericStruct2[string, string]{"", ""}
8888
gs2.Field1 = source()
8989
f := gs2.Getter1
90-
sink(f()) // $ MISSING: hasValueFlow="call to f"
90+
sink(f()) // $ hasValueFlow="call to f"
9191
}
9292
{
9393
gs2 := GenericStruct2[string, string]{"", ""}
@@ -98,7 +98,7 @@ func main() {
9898
gs2 := GenericStruct2[string, string]{"", ""}
9999
f := gs2.Setter1
100100
f(source())
101-
sink(gs2.Field1) // $ MISSING: hasValueFlow="selection of Field1"
101+
sink(gs2.Field1) // $ hasValueFlow="selection of Field1"
102102
}
103103

104104
{
@@ -123,7 +123,7 @@ func main() {
123123
gs2 := GenericStruct2[string, string]{"", ""}
124124
gs2.Field2 = source()
125125
f := gs2.Getter2
126-
sink(f()) // $ MISSING: hasValueFlow="call to f"
126+
sink(f()) // $ hasValueFlow="call to f"
127127
}
128128
{
129129
gs2 := GenericStruct2[string, string]{"", ""}
@@ -134,6 +134,6 @@ func main() {
134134
gs2 := GenericStruct2[string, string]{"", ""}
135135
f := gs2.Setter2
136136
f(source())
137-
sink(gs2.Field2) // $ MISSING: hasValueFlow="selection of Field2"
137+
sink(gs2.Field2) // $ hasValueFlow="selection of Field2"
138138
}
139139
}

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,10 @@ DataFlow::CallNode getAYamlCall() {
1313

1414
predicate isSourceSinkPair(DataFlow::Node inNode, DataFlow::Node outNode) {
1515
exists(DataFlow::CallNode cn | cn = getAYamlCall() |
16-
inNode = [cn.getAnArgument(), cn.getReceiver()] and
16+
inNode = [cn.getAnArgument(), cn.(DataFlow::MethodCallNode).getReceiver()] and
1717
(
18-
outNode.(DataFlow::PostUpdateNode).getPreUpdateNode() = [cn.getAnArgument(), cn.getReceiver()]
18+
outNode.(DataFlow::PostUpdateNode).getPreUpdateNode() =
19+
[cn.getAnArgument(), cn.(DataFlow::MethodCallNode).getReceiver()]
1920
or
2021
outNode = cn.getAResult()
2122
)

0 commit comments

Comments
 (0)