From 9ba3d80bad6372aac509c7396285542224acf72f Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Mon, 9 Jul 2018 09:07:42 +0100 Subject: [PATCH] JavaScript: Lift call graph library to data flow graph. --- change-notes/1.18/analysis-javascript.md | 1 + .../ql/src/Expressions/ExprHasNoEffect.ql | 3 +- .../src/LanguageFeatures/IllegalInvocation.ql | 17 +- .../src/LanguageFeatures/InconsistentNew.ql | 22 +-- .../src/LanguageFeatures/SpuriousArguments.ql | 34 ++-- ...UnsupportedStateUpdateInLifecycleMethod.ql | 17 +- .../ql/src/Statements/ImplicitReturn.ql | 6 +- .../src/semmle/javascript/StandardLibrary.qll | 2 +- .../semmle/javascript/dataflow/CallGraph.qll | 6 +- .../javascript/dataflow/Configuration.qll | 2 +- .../semmle/javascript/dataflow/DataFlow.qll | 167 ++++++++++++++++++ .../src/semmle/javascript/dataflow/Nodes.qll | 139 ++++++++++++--- .../semmle/javascript/dataflow/Sources.qll | 7 +- .../javascript/dataflow/TrackedNodes.qll | 2 +- .../dataflow/internal/FlowSteps.qll | 22 ++- .../frameworks/ConnectExpressShared.qll | 2 +- .../javascript/frameworks/NodeJSLib.qll | 2 +- .../semmle/javascript/frameworks/React.qll | 6 +- .../javascript/security/SensitiveActions.qll | 2 +- .../ql/test/library-tests/CallGraphs/Basic.ql | 4 - .../test/library-tests/CallGraphs/es2015.js | 9 +- .../{Basic.expected => getACallee.expected} | 8 + .../library-tests/CallGraphs/getACallee.ql | 4 + .../CallGraphs/getAnArgument.expected | 18 ++ .../library-tests/CallGraphs/getAnArgument.ql | 4 + .../CallGraphs/getArgument.expected | 17 ++ .../library-tests/CallGraphs/getArgument.ql | 4 + .../CallGraphs/getCalleeName.expected | 49 +++++ .../library-tests/CallGraphs/getCalleeName.ql | 4 + .../CallGraphs/getCalleeNode.expected | 57 ++++++ .../library-tests/CallGraphs/getCalleeNode.ql | 4 + .../CallGraphs/getLastArgument.expected | 11 ++ .../CallGraphs/getLastArgument.ql | 4 + .../CallGraphs/getNumArgument.expected | 52 ++++++ .../CallGraphs/getNumArgument.ql | 4 + .../CallGraphs/isImprecise.expected | 1 + .../library-tests/CallGraphs/isImprecise.ql | 5 + .../CallGraphs/isIncomplete.expected | 30 ++++ .../library-tests/CallGraphs/isIncomplete.ql | 5 + .../CallGraphs/isUncertain.expected | 31 ++++ .../library-tests/CallGraphs/isUncertain.ql | 5 + .../library-tests/CallGraphs/reflection.js | 8 + .../ql/test/library-tests/CallGraphs/tst.js | 5 +- .../ql/test/library-tests/CallGraphs/tst2.js | 1 + .../ql/test/library-tests/CallGraphs/tst3.js | 2 + .../TypeScript/CallGraph/CallGraph.ql | 2 +- .../TypeScript/ImportEquals/Resolution.ql | 4 +- .../IllegalInvocation.expected | 4 +- .../SpuriousArguments.expected | 7 +- .../SpuriousArguments/globals.js | 4 +- .../SpuriousArguments/reflection.js | 24 +++ .../query14.ql | 2 +- .../SpuriousArguments/reflection.js | 24 +++ 53 files changed, 761 insertions(+), 115 deletions(-) delete mode 100644 javascript/ql/test/library-tests/CallGraphs/Basic.ql rename javascript/ql/test/library-tests/CallGraphs/{Basic.expected => getACallee.expected} (80%) create mode 100644 javascript/ql/test/library-tests/CallGraphs/getACallee.ql create mode 100644 javascript/ql/test/library-tests/CallGraphs/getAnArgument.expected create mode 100644 javascript/ql/test/library-tests/CallGraphs/getAnArgument.ql create mode 100644 javascript/ql/test/library-tests/CallGraphs/getArgument.expected create mode 100644 javascript/ql/test/library-tests/CallGraphs/getArgument.ql create mode 100644 javascript/ql/test/library-tests/CallGraphs/getCalleeName.expected create mode 100644 javascript/ql/test/library-tests/CallGraphs/getCalleeName.ql create mode 100644 javascript/ql/test/library-tests/CallGraphs/getCalleeNode.expected create mode 100644 javascript/ql/test/library-tests/CallGraphs/getCalleeNode.ql create mode 100644 javascript/ql/test/library-tests/CallGraphs/getLastArgument.expected create mode 100644 javascript/ql/test/library-tests/CallGraphs/getLastArgument.ql create mode 100644 javascript/ql/test/library-tests/CallGraphs/getNumArgument.expected create mode 100644 javascript/ql/test/library-tests/CallGraphs/getNumArgument.ql create mode 100644 javascript/ql/test/library-tests/CallGraphs/isImprecise.expected create mode 100644 javascript/ql/test/library-tests/CallGraphs/isImprecise.ql create mode 100644 javascript/ql/test/library-tests/CallGraphs/isIncomplete.expected create mode 100644 javascript/ql/test/library-tests/CallGraphs/isIncomplete.ql create mode 100644 javascript/ql/test/library-tests/CallGraphs/isUncertain.expected create mode 100644 javascript/ql/test/library-tests/CallGraphs/isUncertain.ql create mode 100644 javascript/ql/test/library-tests/CallGraphs/reflection.js create mode 100644 javascript/ql/test/library-tests/CallGraphs/tst2.js create mode 100644 javascript/ql/test/library-tests/CallGraphs/tst3.js create mode 100644 javascript/ql/test/query-tests/LanguageFeatures/SpuriousArguments/reflection.js create mode 100644 javascript/ql/test/violation-tests/LanguageFeatures/SpuriousArguments/reflection.js diff --git a/change-notes/1.18/analysis-javascript.md b/change-notes/1.18/analysis-javascript.md index d507351eaa0..56190b0521f 100644 --- a/change-notes/1.18/analysis-javascript.md +++ b/change-notes/1.18/analysis-javascript.md @@ -50,3 +50,4 @@ * HTTP header names are now always normalized to lower case to reflect the fact that they are case insensitive. In particular, the result of `HeaderDefinition.getAHeaderName`, and the first parameter of `HeaderDefinition.defines`, `ExplicitHeaderDefinition.definesExplicitly` and `RouteHandler.getAResponseHeader` is now always a lower-case string. * The class `JsonParseCall` has been deprecated. Use `JsonParserCall` instead. +* The handling of spread arguments in the data flow library has been changed: `DataFlow::InvokeNode.getArgument(i)` is now only defined when there is no spread argument at or before argument position `i`, and similarly `InvokeNode.getNumArgument` is only defined for invocations without spread arguments. diff --git a/javascript/ql/src/Expressions/ExprHasNoEffect.ql b/javascript/ql/src/Expressions/ExprHasNoEffect.ql index 3f6786425ab..49dbfc53fef 100644 --- a/javascript/ql/src/Expressions/ExprHasNoEffect.ql +++ b/javascript/ql/src/Expressions/ExprHasNoEffect.ql @@ -121,8 +121,7 @@ predicate noSideEffects(Expr e) { e.isPure() or // `new Error(...)`, `new SyntaxError(...)`, etc. - e instanceof NewExpr and - forex (Function f | f = e.(CallSite).getACallee() | + forex (Function f | f = e.flow().(DataFlow::NewNode).getACallee() | f.(ExternalType).getASupertype*().getName() = "Error" ) } diff --git a/javascript/ql/src/LanguageFeatures/IllegalInvocation.ql b/javascript/ql/src/LanguageFeatures/IllegalInvocation.ql index 83710322c03..58290a6fd19 100644 --- a/javascript/ql/src/LanguageFeatures/IllegalInvocation.ql +++ b/javascript/ql/src/LanguageFeatures/IllegalInvocation.ql @@ -16,13 +16,13 @@ import javascript /** * Holds if call site `cs` may invoke function `callee` as specified by `how`. */ -predicate calls(CallSite cs, Function callee, string how) { +predicate calls(DataFlow::InvokeNode cs, Function callee, string how) { callee = cs.getACallee() and ( - cs instanceof CallExpr and not cs instanceof SuperCall and + cs instanceof DataFlow::CallNode and not cs.asExpr() instanceof SuperCall and how = "as a function" or - cs instanceof NewExpr and + cs instanceof DataFlow::NewNode and how = "using 'new'" ) } @@ -31,7 +31,7 @@ predicate calls(CallSite cs, Function callee, string how) { * Holds if call site `cs` may illegally invoke function `callee` as specified by `how`; * `calleeDesc` describes what kind of function `callee` is. */ -predicate illegalInvocation(CallSite cs, Function callee, string calleeDesc, string how) { +predicate illegalInvocation(DataFlow::InvokeNode cs, Function callee, string calleeDesc, string how) { calls(cs, callee, how) and ( how = "as a function" and @@ -44,15 +44,16 @@ predicate illegalInvocation(CallSite cs, Function callee, string calleeDesc, str } /** - * Holds if `ce` has at least one call target that isn't a constructor. + * Holds if `ce` is a call with at least one call target that isn't a constructor. */ -predicate isCallToFunction(CallExpr ce) { - exists (Function f | f = ce.(CallSite).getACallee() | +predicate isCallToFunction(DataFlow::InvokeNode ce) { + ce instanceof DataFlow::CallNode and + exists (Function f | f = ce.getACallee() | not f instanceof Constructor ) } -from CallSite cs, Function callee, string calleeDesc, string how +from DataFlow::InvokeNode cs, Function callee, string calleeDesc, string how where illegalInvocation(cs, callee, calleeDesc, how) and // filter out some easy cases not isCallToFunction(cs) and diff --git a/javascript/ql/src/LanguageFeatures/InconsistentNew.ql b/javascript/ql/src/LanguageFeatures/InconsistentNew.ql index f49b414de0d..05ab926cc2b 100644 --- a/javascript/ql/src/LanguageFeatures/InconsistentNew.ql +++ b/javascript/ql/src/LanguageFeatures/InconsistentNew.ql @@ -22,8 +22,8 @@ import semmle.javascript.RestrictedLocations * have to call itself using `new`, so that is what we look for. */ predicate guardsAgainstMissingNew(Function f) { - exists (CallSite new | - new.(NewExpr).getEnclosingFunction() = f and + exists (DataFlow::NewNode new | + new.asExpr().getEnclosingFunction() = f and f = new.getACallee() ) } @@ -34,13 +34,13 @@ predicate guardsAgainstMissingNew(Function f) { * is only suggested as a potential callee due to imprecise analysis of global * variables and is not, in fact, a viable callee at all. */ -predicate calls(CallSite cs, Function callee, int imprecision) { +predicate calls(DataFlow::InvokeNode cs, Function callee, int imprecision) { callee = cs.getACallee() and ( // if global flow was used to derive the callee, we may be imprecise if cs.isIndefinite("global") then // callees within the same file are probably genuine - callee.getFile() = cs.(Locatable).getFile() and imprecision = 0 + callee.getFile() = cs.getFile() and imprecision = 0 or // calls to global functions declared in an externs file are fairly // safe as well @@ -58,11 +58,11 @@ predicate calls(CallSite cs, Function callee, int imprecision) { * Gets a function that may be invoked at `cs`, preferring callees that * are less likely to be derived due to analysis imprecision. */ -Function getALikelyCallee(CallSite cs) { +Function getALikelyCallee(DataFlow::InvokeNode cs) { calls(cs, result, min(int p | calls(cs, _, p))) } -from Function f, NewExpr new, CallExpr call +from Function f, DataFlow::NewNode new, DataFlow::CallNode call where // externs are special, so don't flag them not f.inExternsFile() and // illegal constructor calls are flagged by query 'Illegal invocation', @@ -71,11 +71,11 @@ where // externs are special, so don't flag them f = getALikelyCallee(new) and f = getALikelyCallee(call) and not guardsAgainstMissingNew(f) and - not new.(CallSite).isUncertain() and - not call.(CallSite).isUncertain() and + not new.isUncertain() and + not call.isUncertain() and // super constructor calls behave more like `new`, so don't flag them - not call instanceof SuperCall and - // reflective calls provide an explicit receiver object, so don't flag them either - not call instanceof ReflectiveCallSite + not call.asExpr() instanceof SuperCall and + // don't flag if there is a receiver object + not exists(call.getReceiver()) select (FirstLineOf)f, capitalize(f.describe()) + " is invoked as a constructor here $@, " + "and as a normal function here $@.", new, new.toString(), call, call.toString() diff --git a/javascript/ql/src/LanguageFeatures/SpuriousArguments.ql b/javascript/ql/src/LanguageFeatures/SpuriousArguments.ql index ff9a98da317..284aa32f116 100644 --- a/javascript/ql/src/LanguageFeatures/SpuriousArguments.ql +++ b/javascript/ql/src/LanguageFeatures/SpuriousArguments.ql @@ -29,23 +29,11 @@ predicate isFixedArity(Function fn) { * * This is only defined if all potential callees have a fixed arity. */ -int maxArity(CallSite invk) { +int maxArity(DataFlow::InvokeNode invk) { forall (Function callee | callee = invk.getACallee() | isFixedArity(callee)) and result = max(invk.getACallee().getNumParameter()) } -/** - * Holds if call site `invk` has more arguments than the maximum arity - * of any function that it may invoke, and the first of those - * arguments is `arg`. - * - * This predicate is only defined for call sites where callee information is complete. - */ -predicate firstSpuriousArgument(CallSite invk, Expr arg) { - arg = invk.getArgumentNode(maxArity(invk)).asExpr() and - not invk.isIncomplete() -} - /** * A list of spurious arguments passed at a call site. * @@ -54,15 +42,18 @@ predicate firstSpuriousArgument(CallSite invk, Expr arg) { * defined to cover all subsequent arguments as well. */ class SpuriousArguments extends Expr { + DataFlow::InvokeNode invk; + SpuriousArguments() { - firstSpuriousArgument(_, this) + this = invk.getArgument(maxArity(invk)).asExpr() and + not invk.isIncomplete() } /** * Gets the call site at which the spurious arguments are passed. */ - CallSite getCall() { - firstSpuriousArgument(result, this) + DataFlow::InvokeNode getCall() { + result = invk } /** @@ -71,7 +62,7 @@ class SpuriousArguments extends Expr { * expected by any potential callee. */ int getCount() { - result = getCall().(InvokeExpr).getNumArgument() - maxArity(getCall()) + result = count(int i | exists(invk.getArgument(i)) and i >= maxArity(getCall())) } /** @@ -82,9 +73,10 @@ class SpuriousArguments extends Expr { * [LGTM locations](https://lgtm.com/help/ql/locations). */ predicate hasLocationInfo(string filepath, int startline, int startcolumn, int endline, int endcolumn) { - this.getLocation().hasLocationInfo(filepath, startline, startcolumn, _, _) and - exists (Expr lastArg | lastArg = getCall().(InvokeExpr).getLastArgument() | - lastArg.getLocation().hasLocationInfo(_, _, _, endline, endcolumn) + getLocation().hasLocationInfo(filepath, startline, startcolumn, _, _) and + exists (DataFlow::Node lastArg | + lastArg = max(DataFlow::Node arg, int i | arg = invk.getArgument(i) | arg order by i) | + lastArg.hasLocationInfo(_, _, _, endline, endcolumn) ) } } @@ -92,4 +84,4 @@ class SpuriousArguments extends Expr { from SpuriousArguments args, Function f, string arguments where f = args.getCall().getACallee() and if args.getCount() = 1 then arguments = "argument" else arguments = "arguments" -select args, "Superfluous " + arguments + " passed to $@.", f, f.describe() \ No newline at end of file +select args, "Superfluous " + arguments + " passed to $@.", f, f.describe() diff --git a/javascript/ql/src/React/UnsupportedStateUpdateInLifecycleMethod.ql b/javascript/ql/src/React/UnsupportedStateUpdateInLifecycleMethod.ql index e6063019e70..bffe9e0b2e7 100644 --- a/javascript/ql/src/React/UnsupportedStateUpdateInLifecycleMethod.ql +++ b/javascript/ql/src/React/UnsupportedStateUpdateInLifecycleMethod.ql @@ -14,17 +14,17 @@ import javascript /** * A call that invokes a method on its own receiver. */ -class CallOnSelf extends CallExpr { +class CallOnSelf extends DataFlow::CallNode { CallOnSelf() { exists (Function binder | binder = getEnclosingFunction().getThisBinder() | exists (DataFlow::ThisNode thiz | - this = thiz.getAMethodCall(_).asExpr() and + this = thiz.getAMethodCall(_) and thiz.getBinder().getAstNode() = binder ) or - this.(CallSite).getACallee().(ArrowFunctionExpr).getThisBinder() = binder + this.getACallee().(ArrowFunctionExpr).getThisBinder() = binder ) } @@ -32,7 +32,7 @@ class CallOnSelf extends CallExpr { * Gets a `CallOnSelf` in the callee of this call. */ CallOnSelf getACalleCallOnSelf() { - result.getEnclosingFunction() = this.(CallSite).getACallee() + result.getEnclosingFunction() = this.getACallee() } } @@ -55,15 +55,15 @@ class UnconditionalCallOnSelf extends CallOnSelf { /** * Holds if `call` is guaranteed to occur in its enclosing function, unless an exception occurs. */ -predicate isUnconditionalCall(CallExpr call) { +predicate isUnconditionalCall(DataFlow::CallNode call) { exists (ReachableBasicBlock callBlock, ReachableBasicBlock entryBlock | callBlock.postDominates(entryBlock) and - callBlock.getANode() = call and + callBlock = call.getBasicBlock() and entryBlock = call.getEnclosingFunction().getEntryBB() ) } -predicate isStateUpdateMethodCall(MethodCallExpr mce) { +predicate isStateUpdateMethodCall(DataFlow::MethodCallNode mce) { exists (string updateMethodName | updateMethodName = "setState" or updateMethodName = "replaceState" or @@ -111,7 +111,8 @@ class StateUpdateVolatileMethod extends Function { } -from StateUpdateVolatileMethod root, CallOnSelf initCall, MethodCallExpr stateUpdate, string callDescription +from StateUpdateVolatileMethod root, CallOnSelf initCall, DataFlow::MethodCallNode stateUpdate, + string callDescription where initCall.getEnclosingFunction() = root and stateUpdate = initCall.getACalleCallOnSelf*() and isStateUpdateMethodCall(stateUpdate) and diff --git a/javascript/ql/src/Statements/ImplicitReturn.ql b/javascript/ql/src/Statements/ImplicitReturn.ql index 5d202bbfc64..1c3096a5713 100644 --- a/javascript/ql/src/Statements/ImplicitReturn.ql +++ b/javascript/ql/src/Statements/ImplicitReturn.ql @@ -55,9 +55,9 @@ int numRet(Function f) { */ predicate isDualUseConstructor(Function f) { numRet(f) = 1 and - exists (ReturnStmt ret, NewExpr new | ret.getContainer() = f | - new = ret.getExpr().stripParens() and - new.(CallSite).getACallee() = f + exists (ReturnStmt ret, DataFlow::NewNode new | ret.getContainer() = f | + new.asExpr() = ret.getExpr().stripParens() and + new.getACallee() = f ) } diff --git a/javascript/ql/src/semmle/javascript/StandardLibrary.qll b/javascript/ql/src/semmle/javascript/StandardLibrary.qll index 0609aa82dca..e9d38d02c4f 100644 --- a/javascript/ql/src/semmle/javascript/StandardLibrary.qll +++ b/javascript/ql/src/semmle/javascript/StandardLibrary.qll @@ -9,7 +9,7 @@ class CallToObjectDefineProperty extends DataFlow::MethodCallNode { CallToObjectDefineProperty() { exists (GlobalVariable obj | obj.getName() = "Object" and - astNode.calls(obj.getAnAccess(), "defineProperty") + calls(DataFlow::valueNode(obj.getAnAccess()), "defineProperty") ) } diff --git a/javascript/ql/src/semmle/javascript/dataflow/CallGraph.qll b/javascript/ql/src/semmle/javascript/dataflow/CallGraph.qll index b1d25af0e4c..43ac39cf882 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/CallGraph.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/CallGraph.qll @@ -6,11 +6,13 @@ import javascript private import InferredTypes /** + * DEPRECATED: Use `DataFlow::InvokeNode` instead. + * * A function call or `new` expression, with information about its potential callees. * * Both direct calls and reflective calls using `call` or `apply` are modelled. */ -class CallSite extends @invokeexpr { +deprecated class CallSite extends @invokeexpr { InvokeExpr invk; CallSite() { invk = this } @@ -120,7 +122,7 @@ class CallSite extends @invokeexpr { /** * A reflective function call using `call` or `apply`. */ -class ReflectiveCallSite extends CallSite { +deprecated class ReflectiveCallSite extends CallSite { DataFlow::AnalyzedNode callee; string callMode; diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll index 924ff483921..09dff7b0120 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll @@ -407,7 +407,7 @@ private predicate callInputStep(Function f, DataFlow::Node invk, or exists (SsaDefinition prevDef, SsaDefinition def | pred = DataFlow::ssaDefinitionNode(prevDef) and - calls(invk.asExpr(), f) and captures(f, prevDef, def) and + calls(invk, f) and captures(f, prevDef, def) and succ = DataFlow::ssaDefinitionNode(def) ) ) and diff --git a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll b/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll index c6fb3c62efc..7686f7e323e 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll @@ -29,6 +29,9 @@ module DataFlow { or TDestructuringPatternNode(DestructuringPattern dp) or TElementPatternNode(ArrayPattern ap, Expr p) { p = ap.getElement(_) } or TElementNode(ArrayExpr arr, Expr e) { e = arr.getAnElement() } + or TReflectiveCallNode(MethodCallExpr ce, string kind) { + ce.getMethodName() = kind and (kind = "call" or kind = "apply") + } /** * A node in the data flow graph. @@ -366,6 +369,30 @@ module DataFlow { } } + /** + * A node in the data flow graph which corresponds to the reflective call performed + * by a `.call` or `.apply` invocation. + */ + private class ReflectiveCallNode extends Node, TReflectiveCallNode { + MethodCallExpr call; + string kind; + + ReflectiveCallNode() { this = TReflectiveCallNode(call, kind) } + + override BasicBlock getBasicBlock() { + result = call.getBasicBlock() + } + + override predicate hasLocationInfo(string filepath, int startline, int startcolumn, + int endline, int endcolumn) { + call.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) + } + + override string toString() { + result = "reflective call" + } + } + /** * A data flow node that reads or writes an object property or class member. * @@ -643,6 +670,146 @@ module DataFlow { override string getPropertyName() { none() } } + /** + * Provides classes representing various kinds of calls. + * + * Subclass the classes in this module to introduce new kinds of calls. If you want to + * refine the behaviour of the analysis on existing kinds of calls, subclass `InvokeNode` + * instead. + */ + module Impl { + /** + * A data flow node representing a function invocation, either explicitly or reflectively, + * and either with or without `new`. + */ + abstract class InvokeNodeDef extends DataFlow::Node { + /** Gets the name of the function or method being invoked, if it can be determined. */ + abstract string getCalleeName(); + + /** Gets the data flow node specifying the function to be called. */ + abstract DataFlow::Node getCalleeNode(); + + /** Gets the data flow node corresponding to the `i`th argument of this invocation. */ + abstract DataFlow::Node getArgument(int i); + + /** Gets the data flow node corresponding to an argument of this invocation. */ + abstract DataFlow::Node getAnArgument(); + + /** Gets the number of arguments of this invocation, if it can be determined. */ + abstract int getNumArgument(); + } + + /** + * A data flow node representing a function call without `new`, either explicitly or + * reflectively. + */ + abstract class CallNodeDef extends InvokeNodeDef { + /** Gets the data flow node corresponding to the receiver of this call, if any. */ + DataFlow::Node getReceiver() { none() } + } + + /** + * A data flow node representing a method call. + */ + abstract class MethodCallNodeDef extends CallNodeDef { + /** Gets the name of the method being invoked, if it can be determined. */ + abstract string getMethodName(); + } + + /** + * A data flow node representing a function invocation with `new`. + */ + abstract class NewNodeDef extends InvokeNodeDef { + } + + /** + * A data flow node representing an explicit (that is, non-reflective) function invocation. + */ + class ExplicitInvokeNode extends InvokeNodeDef, DataFlow::ValueNode { + override InvokeExpr astNode; + + override string getCalleeName() { + result = astNode.getCalleeName() + } + + override DataFlow::Node getCalleeNode() { + result = DataFlow::valueNode(astNode.getCallee()) + } + + override DataFlow::Node getArgument(int i) { + not astNode.isSpreadArgument([0..i]) and result = DataFlow::valueNode(astNode.getArgument(i)) + } + + override DataFlow::Node getAnArgument() { + exists (Expr arg | arg = astNode.getAnArgument() | + not arg instanceof SpreadElement and + result = DataFlow::valueNode(arg) + ) + } + + override int getNumArgument() { + not astNode.isSpreadArgument(_) and result = astNode.getNumArgument() + } + } + + /** + * A data flow node representing an explicit (that is, non-reflective) function call. + */ + private class ExplicitCallNode extends CallNodeDef, ExplicitInvokeNode { + override CallExpr astNode; + } + + /** + * A data flow node representing an explicit (that is, non-reflective) method call. + */ + private class ExplicitMethodCallNode extends MethodCallNodeDef, ExplicitCallNode { + override MethodCallExpr astNode; + override DataFlow::Node getReceiver() { result = DataFlow::valueNode(astNode.getReceiver()) } + override string getMethodName() { result = astNode.getMethodName() } + } + + /** + * A data flow node representing a `new` expression. + */ + private class ExplicitNewNode extends NewNodeDef, ExplicitInvokeNode { + override NewExpr astNode; + } + + /** + * A data flow node representing a reflective function call. + */ + private class ReflectiveCallNodeDef extends CallNodeDef { + ExplicitMethodCallNode originalCall; + string kind; + + ReflectiveCallNodeDef() { + this = TReflectiveCallNode(originalCall.asExpr(), kind) + } + + override string getCalleeName() { none() } + + override DataFlow::Node getCalleeNode() { + result = originalCall.getReceiver() + } + + override DataFlow::Node getReceiver() { + result = originalCall.getArgument(0) + } + + override DataFlow::Node getArgument(int i) { + i >= 0 and kind = "call" and result = originalCall.getArgument(i+1) + } + + override DataFlow::Node getAnArgument() { + kind = "call" and result = originalCall.getAnArgument() and result != getReceiver() + } + + override int getNumArgument() { + result >= 0 and kind = "call" and result = originalCall.getNumArgument()-1 + } + } + } + /** * An array element viewed as a property write; for instance, in * `var arr = ["first", , "third"]`, `"first"` is a write of property 0 of `arr` diff --git a/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll b/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll index f092caa776c..de4982c57cd 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll @@ -20,32 +20,67 @@ class ParameterNode extends DataFlow::DefaultSourceNode { } /** A data flow node corresponding to a function invocation (with or without `new`). */ -class InvokeNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode { - override InvokeExpr astNode; +class InvokeNode extends DataFlow::DefaultSourceNode { + DataFlow::Impl::InvokeNodeDef impl; + + InvokeNode() { this = impl } /** Gets the name of the function or method being invoked, if it can be determined. */ string getCalleeName() { - result = astNode.getCalleeName() + result = impl.getCalleeName() + } + + /** DEPRECATED: Use `getCalleeNode()` instead. */ + deprecated + DataFlow::Node getCallee() { + result = getCalleeNode() } /** Gets the data flow node specifying the function to be called. */ - DataFlow::ValueNode getCallee() { - result = DataFlow::valueNode(astNode.getCallee()) + DataFlow::Node getCalleeNode() { + result = impl.getCalleeNode() } - /** Gets the data flow node corresponding to the `i`th argument of this invocation. */ - DataFlow::ValueNode getArgument(int i) { - result = DataFlow::valueNode(astNode.getArgument(i)) + /** + * Gets the data flow node corresponding to the `i`th argument of this invocation. + * + * For direct calls, this is the `i`th argument to the call itself: for instance, + * for a call `f(x, y)`, the 0th argument node is `x` and the first argument node is `y`. + * + * For reflective calls using `call`, the 0th argument to the call denotes the + * receiver, so argument positions are shifted by one: for instance, for a call + * `f.call(x, y, z)`, the 0th argument node is `y` and the first argument node is `z`, + * while `x` is not an argument node at all. + * + * For reflective calls using `apply` we cannot, in general, tell which argument + * occurs at which position, so this predicate is not defined for such calls. + * + * Note that this predicate is not defined for arguments following a spread + * argument: for instance, for a call `f(x, ...y, z)`, the 0th argument node is `x`, + * but the position of `z` cannot be determined, hence there are no first and second + * argument nodes. + */ + DataFlow::Node getArgument(int i) { + result = impl.getArgument(i) } /** Gets the data flow node corresponding to an argument of this invocation. */ - DataFlow::ValueNode getAnArgument() { - result = getArgument(_) + DataFlow::Node getAnArgument() { + result = impl.getAnArgument() } - /** Gets the number of arguments of this invocation. */ + /** Gets the data flow node corresponding to the last argument of this invocation. */ + DataFlow::Node getLastArgument() { + result = getArgument(getNumArgument()-1) + } + + /** Gets the number of arguments of this invocation, if it can be determined. */ int getNumArgument() { - result = astNode.getNumArgument() + result = impl.getNumArgument() + } + + Function getEnclosingFunction() { + result = getBasicBlock().getContainer() } /** Gets a function passed as the `i`th argument of this invocation. */ @@ -63,16 +98,73 @@ class InvokeNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode { obj.hasPropertyWrite(name, result) ) } + + /** Gets an abstract value representing possible callees of this call site. */ + cached AbstractValue getACalleeValue() { + result = impl.getCalleeNode().analyze().getAValue() + } + + /** Gets a potential callee based on dataflow analysis results. */ + private Function getACalleeFromDataflow() { + result = getACalleeValue().(AbstractCallable).getFunction() + } + + /** Gets a potential callee of this call site. */ + Function getACallee() { + result = getACalleeFromDataflow() + or + not exists(getACalleeFromDataflow()) and + result = impl.(DataFlow::Impl::ExplicitInvokeNode).asExpr().(InvokeExpr).getResolvedCallee() + } + + /** + * Holds if the approximation of possible callees for this call site is + * affected by the given analysis incompleteness `cause`. + */ + predicate isIndefinite(DataFlow::Incompleteness cause) { + getACalleeValue().isIndefinite(cause) + } + + /** + * Holds if our approximation of possible callees for this call site is + * likely to be imprecise. + * + * We currently track one specific source of imprecision: call + * resolution relies on flow through global variables, and the flow + * analysis finds possible callees that are not functions. + * This usually means that a global variable is used in multiple + * independent contexts, so tracking flow through it leads to + * imprecision. + */ + predicate isImprecise() { + isIndefinite("global") and + exists (DefiniteAbstractValue v | v = getACalleeValue() | + not v instanceof AbstractCallable + ) + } + + /** + * Holds if our approximation of possible callees for this call site is + * likely to be incomplete. + */ + predicate isIncomplete() { + // the flow analysis identifies a source of incompleteness other than + // global flow (which usually leads to imprecision rather than incompleteness) + any (DataFlow::Incompleteness cause | isIndefinite(cause)) != "global" + } + + /** + * Holds if our approximation of possible callees for this call site is + * likely to be imprecise or incomplete. + */ + predicate isUncertain() { + isImprecise() or isIncomplete() + } } /** A data flow node corresponding to a function call without `new`. */ class CallNode extends InvokeNode { - override CallExpr astNode; -} - -/** A data flow node corresponding to a method call. */ -class MethodCallNode extends CallNode { - override MethodCallExpr astNode; + override DataFlow::Impl::CallNodeDef impl; /** * Gets the data flow node corresponding to the receiver expression of this method call. @@ -80,11 +172,16 @@ class MethodCallNode extends CallNode { * For example, the receiver of `x.m()` is `x`. */ DataFlow::Node getReceiver() { - result = DataFlow::valueNode(astNode.getReceiver()) + result = impl.getReceiver() } +} + +/** A data flow node corresponding to a method call. */ +class MethodCallNode extends CallNode { + override DataFlow::Impl::MethodCallNodeDef impl; /** Gets the name of the invoked method, if it can be determined. */ - string getMethodName() { result = astNode.getMethodName() } + string getMethodName() { result = impl.getMethodName() } /** * Holds if this data flow node calls method `methodName` on receiver node `receiver`. @@ -98,7 +195,7 @@ class MethodCallNode extends CallNode { /** A data flow node corresponding to a `new` expression. */ class NewNode extends InvokeNode { - override NewExpr astNode; + override DataFlow::Impl::NewNodeDef impl; } /** A data flow node corresponding to a `this` expression. */ diff --git a/javascript/ql/src/semmle/javascript/dataflow/Sources.qll b/javascript/ql/src/semmle/javascript/dataflow/Sources.qll index f259ba8f378..b4fa213f955 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Sources.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Sources.qll @@ -125,7 +125,7 @@ abstract class SourceNode extends DataFlow::Node { */ DataFlow::CallNode getAMethodCall(string methodName) { exists (PropAccess pacc | - pacc = result.getCallee().asExpr().stripParens() and + pacc = result.getCalleeNode().asExpr().stripParens() and flowsToExpr(pacc.getBase()) and pacc.getPropertyName() = methodName ) @@ -142,7 +142,7 @@ abstract class SourceNode extends DataFlow::Node { * Gets an invocation (with our without `new`) of this node. */ DataFlow::InvokeNode getAnInvocation() { - flowsTo(result.getCallee()) + flowsTo(result.getCalleeNode()) } /** @@ -182,7 +182,6 @@ class DefaultSourceNode extends SourceNode { astNode instanceof PropAccess or astNode instanceof Function or astNode instanceof ClassDefinition or - astNode instanceof InvokeExpr or astNode instanceof ObjectExpr or astNode instanceof ArrayExpr or astNode instanceof JSXNode or @@ -197,5 +196,7 @@ class DefaultSourceNode extends SourceNode { ) or DataFlow::parameterNode(this, _) + or + this instanceof DataFlow::Impl::InvokeNodeDef } } diff --git a/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll b/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll index 627e8b55cd7..92a6b8c463a 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll @@ -123,7 +123,7 @@ private module NodeTracking { or exists (SsaDefinition prevDef, SsaDefinition def | pred = DataFlow::ssaDefinitionNode(prevDef) and - calls(invk.asExpr(), f) and captures(f, prevDef, def) and + calls(invk, f) and captures(f, prevDef, def) and succ = DataFlow::ssaDefinitionNode(def) ) ) diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll index 2f421388c35..1251766cef6 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll @@ -20,13 +20,11 @@ predicate shouldTrackProperties(AbstractValue obj) { /** * Holds if `invk` may invoke `f`. */ -predicate calls(InvokeExpr invk, Function f) { - exists (CallSite cs | cs = invk | - if cs.isIndefinite("global") then - (f = cs.getACallee() and f.getFile() = invk.getFile()) - else - f = cs.getACallee() - ) +predicate calls(DataFlow::InvokeNode invk, Function f) { + if invk.isIndefinite("global") then + (f = invk.getACallee() and f.getFile() = invk.getFile()) + else + f = invk.getACallee() } /** @@ -65,11 +63,11 @@ predicate localFlowStep(DataFlow::Node pred, DataFlow::Node succ, * Holds if `arg` is passed as an argument into parameter `parm` * through invocation `invk` of function `f`. */ -predicate argumentPassing(DataFlow::ValueNode invk, DataFlow::ValueNode arg, Function f, Parameter parm) { - exists (int i, CallSite cs | - cs = invk.asExpr() and calls(cs, f) and +predicate argumentPassing(DataFlow::InvokeNode invk, DataFlow::ValueNode arg, Function f, Parameter parm) { + calls(invk, f) and + exists (int i | f.getParameter(i) = parm and not parm.isRestParameter() and - arg = cs.getArgumentNode(i) + arg = invk.getArgument(i) ) } @@ -92,7 +90,7 @@ predicate callStep(DataFlow::Node pred, DataFlow::Node succ) { predicate returnStep(DataFlow::Node pred, DataFlow::Node succ) { exists (Function f | returnExpr(f, pred, _) and - calls(succ.asExpr(), f) + calls(succ, f) ) } diff --git a/javascript/ql/src/semmle/javascript/frameworks/ConnectExpressShared.qll b/javascript/ql/src/semmle/javascript/frameworks/ConnectExpressShared.qll index 8fa702e5dbc..25e223513fe 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/ConnectExpressShared.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/ConnectExpressShared.qll @@ -56,7 +56,7 @@ module ConnectExpressShared { // heuristic: does not return anything (the server will not use the return value) exists(astNode.getAReturnStmt().getExpr()) or // heuristic: is not invoked (the server invokes this at a call site we cannot reason precisely about) - exists(CallSite cs | cs.getACallee() = astNode) + exists(DataFlow::InvokeNode cs | cs.getACallee() = astNode) ) ) } diff --git a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll index 34e691341c2..4f9c6772d5b 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll @@ -424,7 +424,7 @@ module NodeJSLib { // heuristic: does not return anything (Node.js will not use the return value) exists(astNode.getAReturnStmt().getExpr()) or // heuristic: is not invoked (Node.js invokes this at a call site we cannot reason precisely about) - exists(CallSite cs | cs.getACallee() = astNode) + exists(DataFlow::InvokeNode cs | cs.getACallee() = astNode) ) ) } diff --git a/javascript/ql/src/semmle/javascript/frameworks/React.qll b/javascript/ql/src/semmle/javascript/frameworks/React.qll index 076eaf05739..a95d9e02c1a 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/React.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/React.qll @@ -192,10 +192,8 @@ abstract class ReactComponent extends ASTNode { * constructor of this component. */ DataFlow::SourceNode getACandidatePropsSource() { - exists (DataFlow::InvokeNode call | - getComponentCreatorSource().flowsTo(call.getCallee()) and - result.flowsTo(call.getArgument(0)) - ) or + result.flowsTo(getComponentCreatorSource().getAnInvocation().getArgument(0)) + or result = getADefaultPropsSource() } diff --git a/javascript/ql/src/semmle/javascript/security/SensitiveActions.qll b/javascript/ql/src/semmle/javascript/security/SensitiveActions.qll index 248f56fc2a4..d2a13400fd7 100644 --- a/javascript/ql/src/semmle/javascript/security/SensitiveActions.qll +++ b/javascript/ql/src/semmle/javascript/security/SensitiveActions.qll @@ -140,7 +140,7 @@ abstract class SensitiveAction extends DataFlow::Node { } class AuthorizationCall extends SensitiveAction, DataFlow::CallNode { AuthorizationCall() { - exists(string s | s = astNode.getCalleeName() | + exists(string s | s = getCalleeName() | // name contains `login` or `auth`, but not as part of `loginfo` or `unauth`; // also exclude `author` s.regexpMatch("(?i).*(login(?!fo)|(?