diff --git a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll index 4ef187de4d89..974fdd7c0cbf 100644 --- a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll +++ b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll @@ -318,6 +318,11 @@ module API { Node getParameter(int i) { Stages::ApiStage::ref() and result = this.getASuccessor(Label::parameter(i)) + or + exists(int spreadIndex, string arrayProp | + result = this.getASuccessor(Label::spreadArgument(spreadIndex)).getMember(arrayProp) and + i = spreadIndex + arrayProp.toInt() + ) } /** @@ -860,6 +865,23 @@ module API { .getStaticMember(name, DataFlow::MemberKind::getter()) .getAReturn() ) + or + // Handle rest parameters escaping into external code. For example: + // + // function foo(...rest) { + // externalFunc(rest); + // } + // + // Here, 'rest' reaches a def-node at the call to externalFunc, so we need to ensure + // the arguments passed to 'foo' are stored in the 'rest' array. + exists(Function fun, DataFlow::InvokeNode invoke, int argIndex, Parameter rest | + fun.getRestParameter() = rest and + rest.flow() = pred and + invoke.getACallee() = fun and + invoke.getArgument(argIndex) = rhs and + argIndex >= rest.getIndex() and + lbl = Label::member((argIndex - rest.getIndex()).toString()) + ) ) or exists(DataFlow::ClassNode cls, string name | @@ -888,6 +910,11 @@ module API { i = -1 and lbl = Label::receiver() ) or + exists(int i | + spreadArgumentPassing(base, i, rhs) and + lbl = Label::spreadArgument(i) + ) + or exists(DataFlow::SourceNode src, DataFlow::PropWrite pw | use(base, src) and pw = trackUseNode(src).getAPropertyWrite() and rhs = pw.getRhs() | @@ -931,6 +958,29 @@ module API { ) } + pragma[nomagic] + private int firstSpreadIndex(InvokeExpr expr) { + result = min(int i | expr.getArgument(i) instanceof SpreadElement) + } + + pragma[nomagic] + private InvokeExpr getAnInvocationWithSpread(DataFlow::SourceNode node, int i) { + result = node.getAnInvocation().asExpr() and + i = firstSpreadIndex(result) + } + + private predicate spreadArgumentPassing(TApiNode base, int i, DataFlow::Node spreadArray) { + exists( + DataFlow::Node use, DataFlow::SourceNode pred, int bound, InvokeExpr invoke, int spreadPos + | + use(base, use) and + pred = trackUseNode(use, _, bound, "") and + invoke = getAnInvocationWithSpread(pred, spreadPos) and + spreadArray = invoke.getArgument(spreadPos).(SpreadElement).getOperand().flow() and + i = bound + spreadPos + ) + } + /** * Holds if `rhs` is the right-hand side of a definition of node `nd`. */ @@ -1579,6 +1629,9 @@ module API { /** Gets the edge label for the receiver. */ LabelReceiver receiver() { any() } + /** Gets the edge label for a spread argument passed at index `i`. */ + LabelSpreadArgument spreadArgument(int i) { result.getIndex() = i } + /** Gets the `return` edge label. */ LabelReturn return() { any() } @@ -1628,6 +1681,7 @@ module API { } or MkLabelReceiver() or MkLabelReturn() or + MkLabelSpreadArgument(int index) { index = [0 .. 10] } or MkLabelDecoratedClass() or MkLabelDecoratedMember() or MkLabelDecoratedParameter() or @@ -1743,6 +1797,21 @@ module API { override string toString() { result = "getReceiver()" } } + /** A label representing an array passed as a spread argument at a given index. */ + class LabelSpreadArgument extends ApiLabel, MkLabelSpreadArgument { + private int index; + + LabelSpreadArgument() { this = MkLabelSpreadArgument(index) } + + /** Gets the argument index at which the spread argument appears. */ + int getIndex() { result = index } + + override string toString() { + // Note: This refers to the internal edge that has no corresponding method on API::Node + result = "getSpreadArgument(" + index + ")" + } + } + /** A label for a function that is a wrapper around another function. */ class LabelForwardingFunction extends ApiLabel, MkLabelForwardingFunction { override string toString() { result = "getForwardingFunction()" } diff --git a/javascript/ql/lib/semmle/javascript/dataflow/Sources.qll b/javascript/ql/lib/semmle/javascript/dataflow/Sources.qll index de103ef1c903..06729815e9a4 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/Sources.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/Sources.qll @@ -254,6 +254,12 @@ private module Cached { cached predicate invocation(DataFlow::SourceNode func, DataFlow::InvokeNode invoke) { hasLocalSource(invoke.getCalleeNode(), func) + or + exists(ClassDefinition cls, SuperCall call | + hasLocalSource(cls.getSuperClass().flow(), func) and + call.getBinder() = cls.getConstructor().getBody() and + invoke = call.flow() + ) } /** diff --git a/javascript/ql/test/ApiGraphs/spread/tst.js b/javascript/ql/test/ApiGraphs/spread/tst.js index cece4692043e..33fcb81662cb 100644 --- a/javascript/ql/test/ApiGraphs/spread/tst.js +++ b/javascript/ql/test/ApiGraphs/spread/tst.js @@ -9,3 +9,21 @@ function f() { lib.m1({ ...f() }) + +function getArgs() { + return [ + 'x', /* def=moduleImport("something").getMember("exports").getMember("m2").getSpreadArgument(0).getArrayElement() */ + 'y', /* def=moduleImport("something").getMember("exports").getMember("m2").getSpreadArgument(0).getArrayElement() */ + ] +} + +lib.m2(...getArgs()); + +function f3() { + return [ + 'x', /* def=moduleImport("something").getMember("exports").getMember("m3").getSpreadArgument(1).getArrayElement() */ + 'y', /* def=moduleImport("something").getMember("exports").getMember("m3").getSpreadArgument(1).getArrayElement() */ + ] +} + +lib.m3.bind(undefined, 1)(...f3()); 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 6b33506d502f..78b02c5f7db4 100644 --- a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected +++ b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected @@ -1,5 +1,6 @@ #select | apollo.serverSide.ts:8:39:8:64 | get(fil ... => {}) | apollo.serverSide.ts:7:36:7:44 | { files } | apollo.serverSide.ts:8:43:8:50 | file.url | The $@ of this request depends on a $@. | apollo.serverSide.ts:8:43:8:50 | file.url | URL | apollo.serverSide.ts:7:36:7:44 | { files } | user-provided value | +| apollo.serverSide.ts:18:37:18:62 | get(fil ... => {}) | apollo.serverSide.ts:17:34:17:42 | { files } | apollo.serverSide.ts:18:41:18:48 | file.url | The $@ of this request depends on a $@. | apollo.serverSide.ts:18:41:18:48 | file.url | URL | apollo.serverSide.ts:17:34:17:42 | { files } | user-provided value | | axiosInterceptors.serverSide.js:11:26:11:40 | userProvidedUrl | axiosInterceptors.serverSide.js:19:21:19:28 | req.body | axiosInterceptors.serverSide.js:11:26:11:40 | userProvidedUrl | The $@ of this request depends on a $@. | axiosInterceptors.serverSide.js:11:26:11:40 | userProvidedUrl | endpoint | axiosInterceptors.serverSide.js:19:21:19:28 | req.body | user-provided value | | serverSide.js:18:5:18:20 | request(tainted) | serverSide.js:14:29:14:35 | req.url | serverSide.js:18:13:18:19 | tainted | The $@ of this request depends on a $@. | serverSide.js:18:13:18:19 | tainted | URL | serverSide.js:14:29:14:35 | req.url | user-provided value | | serverSide.js:20:5:20:24 | request.get(tainted) | serverSide.js:14:29:14:35 | req.url | serverSide.js:20:17:20:23 | tainted | The $@ of this request depends on a $@. | serverSide.js:20:17:20:23 | tainted | URL | serverSide.js:14:29:14:35 | req.url | user-provided value | @@ -31,6 +32,11 @@ edges | apollo.serverSide.ts:8:13:8:17 | files | apollo.serverSide.ts:8:28:8:31 | file | provenance | | | apollo.serverSide.ts:8:28:8:31 | file | apollo.serverSide.ts:8:43:8:46 | file | provenance | | | apollo.serverSide.ts:8:43:8:46 | file | apollo.serverSide.ts:8:43:8:50 | file.url | provenance | | +| apollo.serverSide.ts:17:34:17:42 | files | apollo.serverSide.ts:18:11:18:15 | files | provenance | | +| apollo.serverSide.ts:17:34:17:42 | { files } | apollo.serverSide.ts:17:34:17:42 | files | provenance | | +| apollo.serverSide.ts:18:11:18:15 | files | apollo.serverSide.ts:18:26:18:29 | file | provenance | | +| apollo.serverSide.ts:18:26:18:29 | file | apollo.serverSide.ts:18:41:18:44 | file | provenance | | +| apollo.serverSide.ts:18:41:18:44 | file | apollo.serverSide.ts:18:41:18:48 | file.url | provenance | | | axiosInterceptors.serverSide.js:19:11:19:17 | { url } | axiosInterceptors.serverSide.js:19:11:19:28 | url | provenance | | | axiosInterceptors.serverSide.js:19:11:19:28 | url | axiosInterceptors.serverSide.js:20:23:20:25 | url | provenance | | | axiosInterceptors.serverSide.js:19:21:19:28 | req.body | axiosInterceptors.serverSide.js:19:11:19:17 | { url } | provenance | | @@ -91,6 +97,12 @@ nodes | apollo.serverSide.ts:8:28:8:31 | file | semmle.label | file | | apollo.serverSide.ts:8:43:8:46 | file | semmle.label | file | | apollo.serverSide.ts:8:43:8:50 | file.url | semmle.label | file.url | +| apollo.serverSide.ts:17:34:17:42 | files | semmle.label | files | +| apollo.serverSide.ts:17:34:17:42 | { files } | semmle.label | { files } | +| apollo.serverSide.ts:18:11:18:15 | files | semmle.label | files | +| apollo.serverSide.ts:18:26:18:29 | file | semmle.label | file | +| apollo.serverSide.ts:18:41:18:44 | file | semmle.label | file | +| apollo.serverSide.ts:18:41:18:48 | file.url | semmle.label | file.url | | axiosInterceptors.serverSide.js:11:26:11:40 | userProvidedUrl | semmle.label | userProvidedUrl | | axiosInterceptors.serverSide.js:19:11:19:17 | { url } | semmle.label | { url } | | axiosInterceptors.serverSide.js:19:11:19:28 | url | semmle.label | url | diff --git a/javascript/ql/test/query-tests/Security/CWE-918/apollo.serverSide.ts b/javascript/ql/test/query-tests/Security/CWE-918/apollo.serverSide.ts index 1d6aabd87fa5..0f1c4afa554c 100644 --- a/javascript/ql/test/query-tests/Security/CWE-918/apollo.serverSide.ts +++ b/javascript/ql/test/query-tests/Security/CWE-918/apollo.serverSide.ts @@ -14,8 +14,8 @@ function createApolloServer(typeDefs) { const resolvers2 = { Mutation: { - downloadFiles: async (_, { files }) => { // $ MISSING: Source[js/request-forgery] - files.forEach((file) => { get(file.url, (res) => {}); }); // $ MISSING: Alert[js/request-forgery] Sink[js/request-forgery] + downloadFiles: async (_, { files }) => { // $ Source[js/request-forgery] + files.forEach((file) => { get(file.url, (res) => {}); }); // $ Alert[js/request-forgery] Sink[js/request-forgery] return true; }, },