diff --git a/javascript/ql/src/semmle/javascript/Promises.qll b/javascript/ql/src/semmle/javascript/Promises.qll index 413938ec3f7..867fab13d66 100644 --- a/javascript/ql/src/semmle/javascript/Promises.qll +++ b/javascript/ql/src/semmle/javascript/Promises.qll @@ -3,6 +3,7 @@ */ import javascript +private import dataflow.internal.StepSummary /** * A definition of a `Promise` object. @@ -121,36 +122,156 @@ class AggregateES2015PromiseDefinition extends PromiseCreationCall { } /** - * This module defines how data-flow propagates into and out of a Promise. - * The data-flow is based on pseudo-properties rather than tainting the Promise object (which is what `PromiseTaintStep` does). + * Common predicates shared between type-tracking and data-flow for promises. */ -private module PromiseFlow { +module Promises { /** * Gets the pseudo-field used to describe resolved values in a promise. */ - string resolveField() { result = "$PromiseResolveField$" } + string valueProp() { result = "$PromiseResolveField$" } /** * Gets the pseudo-field used to describe rejected values in a promise. */ - string rejectField() { result = "$PromiseRejectField$" } + string errorProp() { result = "$PromiseRejectField$" } +} + +/** + * A module for supporting promises in type-tracking predicates. + * The `PromiseTypeTracking::promiseStep` predicate is used for type tracking in and out of promises, + * and is included in the standard type-tracking steps (`SourceNode::track`). + * The `TypeTracker::startInPromise()` predicate can be used to initiate a type-tracker + * where the tracked value is a promise. + * + * The below is an example of a type-tracking predicate where the initial value is a promise: + * ``` + * DataFlow::SourceNode myType(DataFlow::TypeTracker t) { + * t.startInPromise() and + * result = and + * or + * exists(DataFlow::TypeTracker t2 | result = myType(t2).track(t2, t)) + * } + * ``` + * + * The type-tracking predicate above will only end (`t = DataFlow::TypeTracker::end()`) after the tracked value has been + * extracted from the promise. + * + * The `PromiseTypeTracking::promiseStep` predicate can be used instead of `SourceNode::track` + * to get type-tracking only for promise steps. + * + * Replace `t.startInPromise()` in the above example with `t.start()` to create a type-tracking predicate + * where the value is not initially inside a promise. + */ +module PromiseTypeTracking { + /** + * Gets the result from a single step through a promise, from `pred` to `result` summarized by `summary`. + * This can be loading a resolved value from a promise, storing a value in a promise, or copying a resolved value from one promise to another. + */ + DataFlow::SourceNode promiseStep(DataFlow::SourceNode pred, StepSummary summary) { + exists(PromiseFlowStep step, string field | field = Promises::valueProp() | + summary = LoadStep(field) and + step.load(pred, result, field) + or + summary = StoreStep(field) and + step.store(pred, result, field) + or + summary = LevelStep() and + step.loadStore(pred, result, field) + ) + } + + /** + * Gets the result from a single step through a promise, from `pred` with tracker `t2` to `result` with tracker `t`. + * This can be loading a resolved value from a promise, storing a value in a promise, or copying a resolved value from one promise to another. + */ + DataFlow::SourceNode promiseStep( + DataFlow::SourceNode pred, DataFlow::TypeTracker t, DataFlow::TypeTracker t2 + ) { + exists(StepSummary summary | + result = PromiseTypeTracking::promiseStep(pred, summary) and + t = t2.append(summary) + ) + } + + /** + * A class enabling the use of the `resolveField` as a pseudo-property in type-tracking predicates. + */ + private class ResolveFieldAsTypeTrackingProperty extends TypeTrackingPseudoProperty { + ResolveFieldAsTypeTrackingProperty() { this = Promises::valueProp() } + } +} + +/** + * An `AdditionalFlowStep` used to model a data-flow step related to promises. + * + * The `loadStep`/`storeStep`/`loadStoreStep` methods are overloaded such that the new predicates + * `load`/`store`/`loadStore` can be used in the `PromiseTypeTracking` module. + * (Thereby avoiding conflicts with a "cousin" `AdditionalFlowStep` implementation.) + * + * The class is private and is only intended to be used inside the `PromiseTypeTracking` and `PromiseFlow` modules. + */ +abstract private class PromiseFlowStep extends DataFlow::AdditionalFlowStep { + final override predicate step(DataFlow::Node pred, DataFlow::Node succ) { none() } + + final override predicate step( + DataFlow::Node p, DataFlow::Node s, DataFlow::FlowLabel pl, DataFlow::FlowLabel sl + ) { + none() + } + + /** + * Holds if the property `prop` of the object `pred` should be loaded into `succ`. + */ + predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() } + + final override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { + this.load(pred, succ, prop) + } + + /** + * Holds if `pred` should be stored in the object `succ` under the property `prop`. + */ + predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() } + + final override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { + this.store(pred, succ, prop) + } + + /** + * Holds if the property `prop` should be copied from the object `pred` to the object `succ`. + */ + predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() } + + final override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { + this.loadStore(pred, succ, prop) + } +} + +/** + * This module defines how data-flow propagates into and out of a Promise. + * The data-flow is based on pseudo-properties rather than tainting the Promise object (which is what `PromiseTaintStep` does). + */ +private module PromiseFlow { + private predicate valueProp = Promises::valueProp/0; + + private predicate errorProp = Promises::errorProp/0; /** * A flow step describing a promise definition. * * The resolved/rejected value is written to a pseudo-field on the promise. */ - class PromiseDefitionStep extends DataFlow::AdditionalFlowStep { + class PromiseDefitionStep extends PromiseFlowStep { PromiseDefinition promise; PromiseDefitionStep() { this = promise } - override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { - prop = resolveField() and + override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) { + prop = valueProp() and pred = promise.getResolveParameter().getACall().getArgument(0) and succ = this or - prop = rejectField() and + prop = errorProp() and ( pred = promise.getRejectParameter().getACall().getArgument(0) or pred = promise.getExecutor().getExceptionalReturn() @@ -158,9 +279,9 @@ private module PromiseFlow { succ = this } - override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { + override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) { // Copy the value of a resolved promise to the value of this promise. - prop = resolveField() and + prop = valueProp() and pred = promise.getResolveParameter().getACall().getArgument(0) and succ = this } @@ -169,20 +290,20 @@ private module PromiseFlow { /** * A flow step describing the a Promise.resolve (and similar) call. */ - class CreationStep extends DataFlow::AdditionalFlowStep { + class CreationStep extends PromiseFlowStep { PromiseCreationCall promise; CreationStep() { this = promise } - override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { - prop = resolveField() and + override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) { + prop = valueProp() and pred = promise.getValue() and succ = this } - override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { + override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) { // Copy the value of a resolved promise to the value of this promise. - prop = resolveField() and + prop = valueProp() and pred = promise.getValue() and succ = this } @@ -192,7 +313,7 @@ private module PromiseFlow { * A load step loading the pseudo-field describing that the promise is rejected. * The rejected value is thrown as a exception. */ - class AwaitStep extends DataFlow::AdditionalFlowStep { + class AwaitStep extends PromiseFlowStep { DataFlow::Node operand; AwaitExpr await; @@ -201,12 +322,12 @@ private module PromiseFlow { operand.getEnclosingExpr() = await.getOperand() } - override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { - prop = resolveField() and + override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) { + prop = valueProp() and succ = this and pred = operand or - prop = rejectField() and + prop = errorProp() and succ = await.getExceptionTarget() and pred = operand } @@ -215,37 +336,37 @@ private module PromiseFlow { /** * A flow step describing the data-flow related to the `.then` method of a promise. */ - class ThenStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode { + class ThenStep extends PromiseFlowStep, DataFlow::MethodCallNode { ThenStep() { this.getMethodName() = "then" } - override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { - prop = resolveField() and + override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) { + prop = valueProp() and pred = getReceiver() and succ = getCallback(0).getParameter(0) or - prop = rejectField() and + prop = errorProp() and pred = getReceiver() and succ = getCallback(1).getParameter(0) } - override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { + override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) { not exists(this.getArgument(1)) and - prop = rejectField() and + prop = errorProp() and pred = getReceiver() and succ = this or // read the value of a resolved/rejected promise that is returned - (prop = rejectField() or prop = resolveField()) and + (prop = errorProp() or prop = valueProp()) and pred = getCallback([0 .. 1]).getAReturn() and succ = this } - override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { - prop = resolveField() and + override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) { + prop = valueProp() and pred = getCallback([0 .. 1]).getAReturn() and succ = this or - prop = rejectField() and + prop = errorProp() and pred = getCallback([0 .. 1]).getExceptionalReturn() and succ = this } @@ -254,32 +375,32 @@ private module PromiseFlow { /** * A flow step describing the data-flow related to the `.catch` method of a promise. */ - class CatchStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode { + class CatchStep extends PromiseFlowStep, DataFlow::MethodCallNode { CatchStep() { this.getMethodName() = "catch" } - override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { - prop = rejectField() and + override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) { + prop = errorProp() and pred = getReceiver() and succ = getCallback(0).getParameter(0) } - override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { - prop = resolveField() and + override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) { + prop = valueProp() and pred = getReceiver().getALocalSource() and succ = this or // read the value of a resolved/rejected promise that is returned - (prop = rejectField() or prop = resolveField()) and + (prop = errorProp() or prop = valueProp()) and pred = getCallback(0).getAReturn() and succ = this } - override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { - prop = rejectField() and + override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) { + prop = errorProp() and pred = getCallback(0).getExceptionalReturn() and succ = this or - prop = resolveField() and + prop = valueProp() and pred = getCallback(0).getAReturn() and succ = this } @@ -288,22 +409,22 @@ private module PromiseFlow { /** * A flow step describing the data-flow related to the `.finally` method of a promise. */ - class FinallyStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode { + class FinallyStep extends PromiseFlowStep, DataFlow::MethodCallNode { FinallyStep() { this.getMethodName() = "finally" } - override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { - (prop = resolveField() or prop = rejectField()) and + override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) { + (prop = valueProp() or prop = errorProp()) and pred = getReceiver() and succ = this or // read the value of a rejected promise that is returned - prop = rejectField() and + prop = errorProp() and pred = getCallback(0).getAReturn() and succ = this } - override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { - prop = rejectField() and + override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) { + prop = errorProp() and pred = getCallback(0).getExceptionalReturn() and succ = this } diff --git a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll index e7db4e2d660..ba49ac93dd5 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll @@ -8,147 +8,7 @@ private import javascript private import internal.FlowSteps - -private class PropertyName extends string { - PropertyName() { - this = any(DataFlow::PropRef pr).getPropertyName() - or - AccessPath::isAssignedInUniqueFile(this) - or - exists(AccessPath::getAnAssignmentTo(_, this)) - } -} - -private class OptionalPropertyName extends string { - OptionalPropertyName() { this instanceof PropertyName or this = "" } -} - -/** - * A description of a step on an inter-procedural data flow path. - */ -private newtype TStepSummary = - LevelStep() or - CallStep() or - ReturnStep() or - StoreStep(PropertyName prop) or - LoadStep(PropertyName prop) - -/** - * INTERNAL: Use `TypeTracker` or `TypeBackTracker` instead. - * - * A description of a step on an inter-procedural data flow path. - */ -class StepSummary extends TStepSummary { - /** Gets a textual representation of this step summary. */ - string toString() { - this instanceof LevelStep and result = "level" - or - this instanceof CallStep and result = "call" - or - this instanceof ReturnStep and result = "return" - or - exists(string prop | this = StoreStep(prop) | result = "store " + prop) - or - exists(string prop | this = LoadStep(prop) | result = "load " + prop) - } -} - -module StepSummary { - /** - * INTERNAL: Use `SourceNode.track()` or `SourceNode.backtrack()` instead. - */ - cached - predicate step(DataFlow::SourceNode pred, DataFlow::SourceNode succ, StepSummary summary) { - exists(DataFlow::Node mid | pred.flowsTo(mid) | smallstep(mid, succ, summary)) - } - - /** - * INTERNAL: Use `TypeBackTracker.smallstep()` instead. - */ - predicate smallstep(DataFlow::Node pred, DataFlow::Node succ, StepSummary summary) { - // Flow through properties of objects - propertyFlowStep(pred, succ) and - summary = LevelStep() - or - // Flow through global variables - globalFlowStep(pred, succ) and - summary = LevelStep() - or - // Flow into function - callStep(pred, succ) and - summary = CallStep() - or - // Flow out of function - returnStep(pred, succ) and - summary = ReturnStep() - or - // Flow through an instance field between members of the same class - DataFlow::localFieldStep(pred, succ) and - summary = LevelStep() - or - exists(string prop | - basicStoreStep(pred, succ, prop) and - summary = StoreStep(prop) - or - basicLoadStep(pred, succ, prop) and - summary = LoadStep(prop) - ) - or - any(AdditionalTypeTrackingStep st).step(pred, succ) and - summary = LevelStep() - or - // Store to global access path - exists(string name | - pred = AccessPath::getAnAssignmentTo(name) and - AccessPath::isAssignedInUniqueFile(name) and - succ = DataFlow::globalAccessPathRootPseudoNode() and - summary = StoreStep(name) - ) - or - // Load from global access path - exists(string name | - succ = AccessPath::getAReferenceTo(name) and - AccessPath::isAssignedInUniqueFile(name) and - pred = DataFlow::globalAccessPathRootPseudoNode() and - summary = LoadStep(name) - ) - or - // Store to non-global access path - exists(string name | - pred = AccessPath::getAnAssignmentTo(succ, name) and - summary = StoreStep(name) - ) - or - // Load from non-global access path - exists(string name | - succ = AccessPath::getAReferenceTo(pred, name) and - summary = LoadStep(name) and - name != "" - ) - or - // Summarize calls with flow directly from a parameter to a return. - exists(DataFlow::ParameterNode param, DataFlow::FunctionNode fun | - ( - param.flowsTo(fun.getAReturn()) and - summary = LevelStep() - or - exists(string prop | - param.getAPropertyRead(prop).flowsTo(fun.getAReturn()) and - summary = LoadStep(prop) - ) - ) and - if param = fun.getAParameter() - then - // Step from argument to call site. - argumentPassing(succ, pred, fun.getFunction(), param) - else ( - // Step from captured parameter to local call sites - pred = param and - succ = fun.getAnInvocation() - ) - ) - } -} +private import internal.StepSummary private newtype TTypeTracker = MkTypeTracker(Boolean hasCall, OptionalPropertyName prop) @@ -216,6 +76,18 @@ class TypeTracker extends TTypeTracker { */ predicate start() { hasCall = false and prop = "" } + /** + * Holds if this is the starting point of type tracking, and the value starts in the property named `propName`. + * The type tracking only ends after the property has been loaded. + */ + predicate startInProp(PropertyName propName) { hasCall = false and prop = propName } + + /** + * Holds if this is the starting point of type tracking, and the initial value is a promise. + * The type tracking only ends after the value has been extracted from the promise. + */ + predicate startInPromise() { startInProp(Promises::valueProp()) } + /** * Holds if this is the starting point of type tracking * when tracking a parameter into a call, but not out of it. diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/CallGraphs.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/CallGraphs.qll index 944214a94af..44c9b364c2c 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/internal/CallGraphs.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/CallGraphs.qll @@ -3,6 +3,7 @@ */ private import javascript +private import semmle.javascript.dataflow.internal.StepSummary cached module CallGraph { @@ -83,7 +84,7 @@ module CallGraph { getAFunctionReference(function, 0, t.continue()).flowsTo(callback) ) or - exists(DataFlow::StepSummary summary, DataFlow::TypeTracker t2 | + exists(StepSummary summary, DataFlow::TypeTracker t2 | result = getABoundFunctionReferenceAux(function, boundArgs, t2, summary) and t = t2.append(summary) ) @@ -91,12 +92,11 @@ module CallGraph { pragma[noinline] private DataFlow::SourceNode getABoundFunctionReferenceAux( - DataFlow::FunctionNode function, int boundArgs, DataFlow::TypeTracker t, - DataFlow::StepSummary summary + DataFlow::FunctionNode function, int boundArgs, DataFlow::TypeTracker t, StepSummary summary ) { exists(DataFlow::SourceNode prev | prev = getABoundFunctionReferenceAux(function, boundArgs, t) and - DataFlow::StepSummary::step(prev, result, summary) + StepSummary::step(prev, result, summary) ) } diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/StepSummary.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/StepSummary.qll new file mode 100644 index 00000000000..a848cd57434 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/dataflow/internal/StepSummary.qll @@ -0,0 +1,157 @@ +import javascript +private import semmle.javascript.dataflow.TypeTracking +private import FlowSteps + +class PropertyName extends string { + PropertyName() { + this = any(DataFlow::PropRef pr).getPropertyName() + or + AccessPath::isAssignedInUniqueFile(this) + or + exists(AccessPath::getAnAssignmentTo(_, this)) + or + this instanceof TypeTrackingPseudoProperty + } +} + +class OptionalPropertyName extends string { + OptionalPropertyName() { this instanceof PropertyName or this = "" } +} + +/** + * A pseudo-property that can be used in type-tracking. + */ +abstract class TypeTrackingPseudoProperty extends string { + bindingset[this] + TypeTrackingPseudoProperty() { any() } +} + +/** + * A description of a step on an inter-procedural data flow path. + */ +newtype TStepSummary = + LevelStep() or + CallStep() or + ReturnStep() or + StoreStep(PropertyName prop) or + LoadStep(PropertyName prop) + +/** + * INTERNAL: Use `TypeTracker` or `TypeBackTracker` instead. + * + * A description of a step on an inter-procedural data flow path. + */ +class StepSummary extends TStepSummary { + /** Gets a textual representation of this step summary. */ + string toString() { + this instanceof LevelStep and result = "level" + or + this instanceof CallStep and result = "call" + or + this instanceof ReturnStep and result = "return" + or + exists(string prop | this = StoreStep(prop) | result = "store " + prop) + or + exists(string prop | this = LoadStep(prop) | result = "load " + prop) + } +} + +module StepSummary { + /** + * INTERNAL: Use `SourceNode.track()` or `SourceNode.backtrack()` instead. + */ + cached + predicate step(DataFlow::SourceNode pred, DataFlow::SourceNode succ, StepSummary summary) { + exists(DataFlow::Node mid | pred.flowsTo(mid) | smallstep(mid, succ, summary)) + } + + /** + * INTERNAL: Use `TypeBackTracker.smallstep()` instead. + */ + predicate smallstep(DataFlow::Node pred, DataFlow::Node succ, StepSummary summary) { + // Flow through properties of objects + propertyFlowStep(pred, succ) and + summary = LevelStep() + or + // Flow through global variables + globalFlowStep(pred, succ) and + summary = LevelStep() + or + // Flow into function + callStep(pred, succ) and + summary = CallStep() + or + // Flow out of function + returnStep(pred, succ) and + summary = ReturnStep() + or + // Flow through an instance field between members of the same class + DataFlow::localFieldStep(pred, succ) and + summary = LevelStep() + or + exists(string prop | + basicStoreStep(pred, succ, prop) and + summary = StoreStep(prop) + or + basicLoadStep(pred, succ, prop) and + summary = LoadStep(prop) + ) + or + any(AdditionalTypeTrackingStep st).step(pred, succ) and + summary = LevelStep() + or + // Store to global access path + exists(string name | + pred = AccessPath::getAnAssignmentTo(name) and + AccessPath::isAssignedInUniqueFile(name) and + succ = DataFlow::globalAccessPathRootPseudoNode() and + summary = StoreStep(name) + ) + or + // Load from global access path + exists(string name | + succ = AccessPath::getAReferenceTo(name) and + AccessPath::isAssignedInUniqueFile(name) and + pred = DataFlow::globalAccessPathRootPseudoNode() and + summary = LoadStep(name) + ) + or + // Store to non-global access path + exists(string name | + pred = AccessPath::getAnAssignmentTo(succ, name) and + summary = StoreStep(name) + ) + or + // Load from non-global access path + exists(string name | + succ = AccessPath::getAReferenceTo(pred, name) and + summary = LoadStep(name) and + name != "" + ) + or + // Step in/out of a promise + succ = PromiseTypeTracking::promiseStep(pred, summary) + or + // Summarize calls with flow directly from a parameter to a return. + exists(DataFlow::ParameterNode param, DataFlow::FunctionNode fun | + ( + param.flowsTo(fun.getAReturn()) and + summary = LevelStep() + or + exists(string prop | + param.getAPropertyRead(prop).flowsTo(fun.getAReturn()) and + summary = LoadStep(prop) + ) + ) and + if param = fun.getAParameter() + then + // Step from argument to call site. + argumentPassing(succ, pred, fun.getFunction(), param) + else ( + // Step from captured parameter to local call sites + pred = param and + succ = fun.getAnInvocation() + ) + ) + } +} diff --git a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll index e97169264e0..938f877420c 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll @@ -566,36 +566,18 @@ module ClientRequest { * The `isPromise` parameter reflects whether the reference is a promise containing * an instance of `chrome-remote-interface`, or an instance of `chrome-remote-interface`. */ - private DataFlow::SourceNode chromeRemoteInterface(DataFlow::TypeTracker t, boolean isPromise) { - t.start() and + private DataFlow::SourceNode chromeRemoteInterface(DataFlow::TypeTracker t) { exists(DataFlow::CallNode call | call = DataFlow::moduleImport("chrome-remote-interface").getAnInvocation() | - result = call and isPromise = true + // the client is inside in a promise. + t.startInPromise() and result = call or - result = call.getCallback([0 .. 1]).getParameter(0) and isPromise = false + // the client is accessed directly using a callback. + t.start() and result = call.getCallback([0 .. 1]).getParameter(0) ) or - exists(DataFlow::TypeTracker t2 | result = chromeRemoteInterface(t2, isPromise).track(t2, t)) - or - // Simple promise tracking. - exists(DataFlow::TypeTracker t2, DataFlow::SourceNode pred | - pred = chromeRemoteInterface(t2, true) and - isPromise = false and - ( - t2 = t and - exists(AwaitExpr await | DataFlow::valueNode(await.getOperand()).getALocalSource() = pred | - result.getEnclosingExpr() = await - ) - or - t2 = t and - exists(DataFlow::MethodCallNode thenCall | - thenCall.getMethodName() = "then" and pred = thenCall.getReceiver().getALocalSource() - | - result = thenCall.getCallback(0).getParameter(0) - ) - ) - ) + exists(DataFlow::TypeTracker t2 | result = chromeRemoteInterface(t2).track(t2, t)) } /** @@ -606,7 +588,7 @@ module ClientRequest { ChromeRemoteInterfaceRequest() { exists(DataFlow::SourceNode instance | - instance = chromeRemoteInterface(DataFlow::TypeTracker::end(), false) + instance = chromeRemoteInterface(DataFlow::TypeTracker::end()) | optionsArg = 0 and this = instance.getAPropertyRead("Page").getAMemberCall("navigate") diff --git a/javascript/ql/src/semmle/javascript/frameworks/Files.qll b/javascript/ql/src/semmle/javascript/frameworks/Files.qll index 2a3de5cc3e0..55479a9f12a 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/Files.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/Files.qll @@ -74,54 +74,74 @@ private class GlobFileNameSource extends FileNameSource { } } +/** + * Gets a file name or an array of file names from the `globby` library. + * The predicate uses type-tracking. However, type-tracking is only used to track a step out of a promise. + */ +private DataFlow::SourceNode globbyFileNameSource(DataFlow::TypeTracker t) { + exists(string moduleName | moduleName = "globby" | + // `require('globby').sync(_)` + t.start() and + result = DataFlow::moduleMember(moduleName, "sync").getACall() + or + // `files` in `require('globby')(_).then(files => ...)` + t.startInPromise() and + result = DataFlow::moduleImport(moduleName).getACall() + ) + or + // Tracking out of a promise + exists(DataFlow::TypeTracker t2 | + result = PromiseTypeTracking::promiseStep(globbyFileNameSource(t2), t, t2) + ) +} + /** * A file name or an array of file names from the `globby` library. */ private class GlobbyFileNameSource extends FileNameSource { - GlobbyFileNameSource() { - exists(string moduleName | moduleName = "globby" | - // `require('globby').sync(_)` - this = DataFlow::moduleMember(moduleName, "sync").getACall() + GlobbyFileNameSource() { this = globbyFileNameSource(DataFlow::TypeTracker::end()) } +} + +/** + * Gets a file name or an array of file names from the `fast-glob` library. + * The predicate uses type-tracking. However, type-tracking is only used to track a step out of a promise. + */ +private DataFlow::Node fastGlobFileNameSource(DataFlow::TypeTracker t) { + exists(string moduleName | moduleName = "fast-glob" | + // `require('fast-glob').sync(_) + t.start() and result = DataFlow::moduleMember(moduleName, "sync").getACall() + or + exists(DataFlow::SourceNode f | + f = DataFlow::moduleImport(moduleName) or - // `files` in `require('globby')(_).then(files => ...)` - this = - DataFlow::moduleImport(moduleName) - .getACall() - .getAMethodCall("then") - .getCallback(0) - .getParameter(0) + f = DataFlow::moduleMember(moduleName, "async") + | + // `files` in `require('fast-glob')(_).then(files => ...)` and + // `files` in `require('fast-glob').async(_).then(files => ...)` + t.startInPromise() and result = f.getACall() ) - } + or + // `file` in `require('fast-glob').stream(_).on(_, file => ...)` + t.start() and + result = + DataFlow::moduleMember(moduleName, "stream") + .getACall() + .getAMethodCall(EventEmitter::on()) + .getCallback(1) + .getParameter(0) + ) + or + // Tracking out of a promise + exists(DataFlow::TypeTracker t2 | + result = PromiseTypeTracking::promiseStep(fastGlobFileNameSource(t2), t, t2) + ) } /** * A file name or an array of file names from the `fast-glob` library. */ private class FastGlobFileNameSource extends FileNameSource { - FastGlobFileNameSource() { - exists(string moduleName | moduleName = "fast-glob" | - // `require('fast-glob').sync(_)` - this = DataFlow::moduleMember(moduleName, "sync").getACall() - or - exists(DataFlow::SourceNode f | - f = DataFlow::moduleImport(moduleName) - or - f = DataFlow::moduleMember(moduleName, "async") - | - // `files` in `require('fast-glob')(_).then(files => ...)` and - // `files` in `require('fast-glob').async(_).then(files => ...)` - this = f.getACall().getAMethodCall("then").getCallback(0).getParameter(0) - ) - or - // `file` in `require('fast-glob').stream(_).on(_, file => ...)` - this = - DataFlow::moduleMember(moduleName, "stream") - .getACall() - .getAMethodCall(EventEmitter::on()) - .getCallback(1) - .getParameter(0) - ) - } + FastGlobFileNameSource() { this = fastGlobFileNameSource(DataFlow::TypeTracker::end()) } } /** diff --git a/javascript/ql/test/library-tests/frameworks/Concepts/FileNameSource.expected b/javascript/ql/test/library-tests/frameworks/Concepts/FileNameSource.expected index 9987a574f8d..5f7498ff65b 100644 --- a/javascript/ql/test/library-tests/frameworks/Concepts/FileNameSource.expected +++ b/javascript/ql/test/library-tests/frameworks/Concepts/FileNameSource.expected @@ -11,3 +11,7 @@ | tst-file-names.js:25:18:25:22 | files | | tst-file-names.js:27:24:27:28 | files | | tst-file-names.js:29:27:29:30 | file | +| tst-file-names.js:32:34:32:38 | files | +| tst-file-names.js:34:15:34:29 | await globby(_) | +| tst-file-names.js:36:16:36:38 | await f ... sync(_) | +| tst-file-names.js:38:16:38:57 | await f ... => {}) | diff --git a/javascript/ql/test/library-tests/frameworks/Concepts/tst-file-names.js b/javascript/ql/test/library-tests/frameworks/Concepts/tst-file-names.js index 293f2aa86d1..efecab23471 100644 --- a/javascript/ql/test/library-tests/frameworks/Concepts/tst-file-names.js +++ b/javascript/ql/test/library-tests/frameworks/Concepts/tst-file-names.js @@ -27,3 +27,13 @@ fastGlob(_).then(files => files); fastGlob.async(_).then(files => files); fastGlob.stream(_).on(_, file => file); // XXX + +async function foo() { + globby(_).catch(() => {}).then(files => files); + + var files = await globby(_); + + var files2 = await fastGlob.async(_); + + var files2 = await fastGlob.async(_).catch((wat) => {}); +} diff --git a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected index 80d9d41e196..57ff328afe3 100644 --- a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected +++ b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected @@ -47,6 +47,8 @@ nodes | tst.js:61:29:61:35 | tainted | | tst.js:64:30:64:36 | tainted | | tst.js:64:30:64:36 | tainted | +| tst.js:68:30:68:36 | tainted | +| tst.js:68:30:68:36 | tainted | edges | tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted | | tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted | @@ -89,6 +91,8 @@ edges | tst.js:58:9:58:52 | tainted | tst.js:61:29:61:35 | tainted | | tst.js:58:9:58:52 | tainted | tst.js:64:30:64:36 | tainted | | tst.js:58:9:58:52 | tainted | tst.js:64:30:64:36 | tainted | +| tst.js:58:9:58:52 | tainted | tst.js:68:30:68:36 | tainted | +| tst.js:58:9:58:52 | tainted | tst.js:68:30:68:36 | tainted | | tst.js:58:19:58:42 | url.par ... , true) | tst.js:58:19:58:48 | url.par ... ).query | | tst.js:58:19:58:48 | url.par ... ).query | tst.js:58:19:58:52 | url.par ... ery.url | | tst.js:58:19:58:52 | url.par ... ery.url | tst.js:58:9:58:52 | tainted | @@ -109,3 +113,4 @@ edges | tst.js:45:5:45:57 | request ... ainted) | tst.js:14:29:14:35 | req.url | tst.js:45:13:45:56 | 'http:/ ... tainted | The $@ of this request depends on $@. | tst.js:45:13:45:56 | 'http:/ ... tainted | URL | tst.js:14:29:14:35 | req.url | a user-provided value | | tst.js:61:2:61:37 | client. ... inted}) | tst.js:58:29:58:35 | req.url | tst.js:61:29:61:35 | tainted | The $@ of this request depends on $@. | tst.js:61:29:61:35 | tainted | URL | tst.js:58:29:58:35 | req.url | a user-provided value | | tst.js:64:3:64:38 | client. ... inted}) | tst.js:58:29:58:35 | req.url | tst.js:64:30:64:36 | tainted | The $@ of this request depends on $@. | tst.js:64:30:64:36 | tainted | URL | tst.js:58:29:58:35 | req.url | a user-provided value | +| tst.js:68:3:68:38 | client. ... inted}) | tst.js:58:29:58:35 | req.url | tst.js:68:30:68:36 | tainted | The $@ of this request depends on $@. | tst.js:68:30:68:36 | tainted | URL | tst.js:58:29:58:35 | req.url | a user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-918/tst.js b/javascript/ql/test/query-tests/Security/CWE-918/tst.js index b663d536632..53a1ce5477c 100644 --- a/javascript/ql/test/query-tests/Security/CWE-918/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-918/tst.js @@ -60,6 +60,10 @@ var server = http.createServer(async function(req, res) { var client = await CDP(options); client.Page.navigate({url: tainted}); // NOT OK. + CDP(options).catch((ignored) => {}).then((client) => { + client.Page.navigate({url: tainted}); // NOT OK. + }) + CDP(options, (client) => { client.Page.navigate({url: tainted}); // NOT OK. });