From 3cf1459c4ffff2e5bc5ad97f971d9033d91a230e Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Thu, 2 Dec 2021 12:28:05 +0000 Subject: [PATCH] Revert getACallee type change --- ql/lib/semmle/go/Scopes.qll | 2 +- .../go/dataflow/internal/DataFlowDispatch.qll | 2 +- .../semmle/go/dataflow/internal/DataFlowNodes.qll | 14 ++++++++++---- .../security/UnsafeUnzipSymlinkCustomizations.qll | 2 +- ql/src/InconsistentCode/MissingErrorCheck.ql | 4 ++-- ql/src/Security/CWE-322/InsecureHostKeyCallback.ql | 2 +- ql/src/Security/CWE-601/BadRedirectCheck.ql | 8 ++------ .../semmle/go/dataflow/CallGraph/getACall.ql | 4 ++-- .../semmle/go/dataflow/CallGraph/getACallee.ql | 4 ++-- .../go/dataflow/ExternalFlowVarArgs/Flows.ql | 2 +- 10 files changed, 23 insertions(+), 21 deletions(-) diff --git a/ql/lib/semmle/go/Scopes.qll b/ql/lib/semmle/go/Scopes.qll index cb3468150f4..bc1aba221a4 100644 --- a/ql/lib/semmle/go/Scopes.qll +++ b/ql/lib/semmle/go/Scopes.qll @@ -371,7 +371,7 @@ class Function extends ValueEntity, @functionobject { DataFlow::CallNode getACall() { this = result.getTarget() or - this.(DeclaredFunction).getFuncDecl() = result.getACallee() + this = result.getACalleeIncludingExternals().asFunction() } /** Gets the declaration of this function, if any. */ diff --git a/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll b/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll index 03055721351..c6b1c76ddaa 100644 --- a/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll +++ b/ql/lib/semmle/go/dataflow/internal/DataFlowDispatch.qll @@ -93,7 +93,7 @@ DataFlowCallable viableCallable(CallExpr ma) { else if isInterfaceMethodCall(call) then result = getRestrictedInterfaceTarget(call) - else result.getFuncDef() = call.getACallee() + else result = call.getACalleeIncludingExternals() ) } diff --git a/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll b/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll index 35d464a82b4..9276c87e8cb 100644 --- a/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll +++ b/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll @@ -449,20 +449,26 @@ module Public { * For virtual calls, we look up possible targets in all types that implement the receiver * interface type. */ - FuncDef getACallee() { - result = this.getTarget().(DeclaredFunction).getFuncDecl() + DataFlowCallable getACalleeIncludingExternals() { + result.asFunction() = this.getTarget() or exists(DataFlow::Node calleeSource | calleeSource = this.getACalleeSource() | - result = calleeSource.asExpr() + result.asFuncLit() = calleeSource.asExpr() or exists(Method declared, Method actual | calleeSource = declared.getARead() and actual.implements(declared) and - result = actual.(DeclaredFunction).getFuncDecl() + result.asFunction() = actual ) ) } + /** + * As `getACalleeIncludingExternals`, except excluding external functions (those for which + * we lack a definition, such as standard library functions). + */ + FuncDef getACallee() { result = this.getACalleeIncludingExternals().getFuncDef() } + /** Gets the name of the function or method being called, if it can be determined. */ string getCalleeName() { result = expr.getTarget().getName() or result = expr.getCalleeName() } diff --git a/ql/lib/semmle/go/security/UnsafeUnzipSymlinkCustomizations.qll b/ql/lib/semmle/go/security/UnsafeUnzipSymlinkCustomizations.qll index ebd413a629c..9e257745da0 100644 --- a/ql/lib/semmle/go/security/UnsafeUnzipSymlinkCustomizations.qll +++ b/ql/lib/semmle/go/security/UnsafeUnzipSymlinkCustomizations.qll @@ -86,7 +86,7 @@ module UnsafeUnzipSymlink { * Gets a `CallNode` that may call `node`'s enclosing function. */ private DataFlow::CallNode getACaller(DataFlow::CallNode node) { - result.getACallee().getFuncDef() = node.getRoot() + result.getACallee() = node.getRoot() } /** diff --git a/ql/src/InconsistentCode/MissingErrorCheck.ql b/ql/src/InconsistentCode/MissingErrorCheck.ql index 97cdc8cdcce..604d4c7f8a9 100644 --- a/ql/src/InconsistentCode/MissingErrorCheck.ql +++ b/ql/src/InconsistentCode/MissingErrorCheck.ql @@ -26,9 +26,9 @@ predicate isNil(DataFlow::Node node) { node = Builtin::nil().getARead() } * `nil` for the pointer return value at some return site. */ predicate calleeMayReturnNilWithError(DataFlow::CallNode call) { - not exists(call.getACallee().getFuncDef()) + not exists(call.getACallee()) or - exists(FuncDef callee | callee = call.getACallee().getFuncDef() | + exists(FuncDef callee | callee = call.getACallee() | not exists(callee.getBody()) or exists(IR::ReturnInstruction ret, DataFlow::Node ptrReturn, DataFlow::Node errReturn | diff --git a/ql/src/Security/CWE-322/InsecureHostKeyCallback.ql b/ql/src/Security/CWE-322/InsecureHostKeyCallback.ql index 3309ce1e5c4..f27ca8b9042 100644 --- a/ql/src/Security/CWE-322/InsecureHostKeyCallback.ql +++ b/ql/src/Security/CWE-322/InsecureHostKeyCallback.ql @@ -31,7 +31,7 @@ class HostKeyCallbackFunc extends DataFlow::Node { ( this instanceof DataFlow::FunctionNode or - exists(DataFlow::CallNode call | not exists(call.getACallee().getFuncDef().getBody()) | + exists(DataFlow::CallNode call | not exists(call.getACallee().getBody()) | this = call.getAResult() ) ) diff --git a/ql/src/Security/CWE-601/BadRedirectCheck.ql b/ql/src/Security/CWE-601/BadRedirectCheck.ql index 22cfcbe96db..a35d78519bb 100644 --- a/ql/src/Security/CWE-601/BadRedirectCheck.ql +++ b/ql/src/Security/CWE-601/BadRedirectCheck.ql @@ -54,9 +54,7 @@ predicate isCleaned(DataFlow::Node nd) { isCleaned(nd.getAPredecessor()) or exists(FuncDef f, FunctionInput inp | nd = inp.getExitNode(f) | - forex(DataFlow::CallNode call | call.getACallee().getFuncDef() = f | - isCleaned(inp.getEntryNode(call)) - ) + forex(DataFlow::CallNode call | call.getACallee() = f | isCleaned(inp.getEntryNode(call))) ) } @@ -89,9 +87,7 @@ predicate urlPath(DataFlow::Node nd) { urlPath(nd.getAPredecessor()) or exists(FuncDef f, FunctionInput inp | nd = inp.getExitNode(f) | - forex(DataFlow::CallNode call | call.getACallee().getFuncDef() = f | - urlPath(inp.getEntryNode(call)) - ) + forex(DataFlow::CallNode call | call.getACallee() = f | urlPath(inp.getEntryNode(call))) ) } diff --git a/ql/test/library-tests/semmle/go/dataflow/CallGraph/getACall.ql b/ql/test/library-tests/semmle/go/dataflow/CallGraph/getACall.ql index b509a2e839f..82e50120d5e 100644 --- a/ql/test/library-tests/semmle/go/dataflow/CallGraph/getACall.ql +++ b/ql/test/library-tests/semmle/go/dataflow/CallGraph/getACall.ql @@ -1,11 +1,11 @@ import go query predicate missingCall(DeclaredFunction f, DataFlow::CallNode call) { - call.getACallee().asFunction() = f and + call.getACallee() = f.getFuncDecl() and not call = f.getACall() } query predicate spuriousCall(DeclaredFunction f, DataFlow::CallNode call) { call = f.getACall() and - not call.getACallee().asFunction() = f + exists(FuncDecl fd | fd = f.getFuncDecl() | not call.getACallee() = fd) } diff --git a/ql/test/library-tests/semmle/go/dataflow/CallGraph/getACallee.ql b/ql/test/library-tests/semmle/go/dataflow/CallGraph/getACallee.ql index 5399c0cfebc..99e85587d55 100644 --- a/ql/test/library-tests/semmle/go/dataflow/CallGraph/getACallee.ql +++ b/ql/test/library-tests/semmle/go/dataflow/CallGraph/getACallee.ql @@ -16,10 +16,10 @@ string metadata(Locatable l, string key) { query predicate missingCallee(DataFlow::CallNode call, FuncDef callee) { metadata(call.asExpr(), "callee") = metadata(callee, "name") and - not call.getACallee().getFuncDef() = callee + not call.getACallee() = callee } query predicate spuriousCallee(DataFlow::CallNode call, FuncDef callee) { - call.getACallee().getFuncDef() = callee and + call.getACallee() = callee and not metadata(call.asExpr(), "callee") = metadata(callee, "name") } diff --git a/ql/test/library-tests/semmle/go/dataflow/ExternalFlowVarArgs/Flows.ql b/ql/test/library-tests/semmle/go/dataflow/ExternalFlowVarArgs/Flows.ql index d7e78660a05..17aadd75852 100644 --- a/ql/test/library-tests/semmle/go/dataflow/ExternalFlowVarArgs/Flows.ql +++ b/ql/test/library-tests/semmle/go/dataflow/ExternalFlowVarArgs/Flows.ql @@ -12,7 +12,7 @@ class SummaryModelTest extends SummaryModelCsv { "github.com/nonexistent/test;;false;FunctionWithSliceParameter;;;ArrayElement of Argument[0];ReturnValue;value", "github.com/nonexistent/test;;false;FunctionWithVarArgsParameter;;;ArrayElement of Argument[0];ReturnValue;value", "github.com/nonexistent/test;;false;FunctionWithSliceOfStructsParameter;;;Field[github.com/nonexistent/test.A.Field] of ArrayElement of Argument[0];ReturnValue;value", - "github.com/nonexistent/test;;false;FunctionWithVarArgsOfStructsParameter;;;Field[github.com/nonexistent/test.A.Field] of ArrayElement of Argument[0];ReturnValue;value", + "github.com/nonexistent/test;;false;FunctionWithVarArgsOfStructsParameter;;;Field[github.com/nonexistent/test.A.Field] of ArrayElement of Argument[0];ReturnValue;value" ] } }