Skip to content

Commit 887942e

Browse files
authored
Merge pull request #19108 from asgerf/js/api-graph-spread-rest
JS: Handle spread/rest in API graphs
2 parents aacdc70 + 4746cfd commit 887942e

File tree

5 files changed

+107
-2
lines changed

5 files changed

+107
-2
lines changed

Diff for: javascript/ql/lib/semmle/javascript/ApiGraphs.qll

+69
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,11 @@ module API {
318318
Node getParameter(int i) {
319319
Stages::ApiStage::ref() and
320320
result = this.getASuccessor(Label::parameter(i))
321+
or
322+
exists(int spreadIndex, string arrayProp |
323+
result = this.getASuccessor(Label::spreadArgument(spreadIndex)).getMember(arrayProp) and
324+
i = spreadIndex + arrayProp.toInt()
325+
)
321326
}
322327

323328
/**
@@ -860,6 +865,23 @@ module API {
860865
.getStaticMember(name, DataFlow::MemberKind::getter())
861866
.getAReturn()
862867
)
868+
or
869+
// Handle rest parameters escaping into external code. For example:
870+
//
871+
// function foo(...rest) {
872+
// externalFunc(rest);
873+
// }
874+
//
875+
// Here, 'rest' reaches a def-node at the call to externalFunc, so we need to ensure
876+
// the arguments passed to 'foo' are stored in the 'rest' array.
877+
exists(Function fun, DataFlow::InvokeNode invoke, int argIndex, Parameter rest |
878+
fun.getRestParameter() = rest and
879+
rest.flow() = pred and
880+
invoke.getACallee() = fun and
881+
invoke.getArgument(argIndex) = rhs and
882+
argIndex >= rest.getIndex() and
883+
lbl = Label::member((argIndex - rest.getIndex()).toString())
884+
)
863885
)
864886
or
865887
exists(DataFlow::ClassNode cls, string name |
@@ -888,6 +910,11 @@ module API {
888910
i = -1 and lbl = Label::receiver()
889911
)
890912
or
913+
exists(int i |
914+
spreadArgumentPassing(base, i, rhs) and
915+
lbl = Label::spreadArgument(i)
916+
)
917+
or
891918
exists(DataFlow::SourceNode src, DataFlow::PropWrite pw |
892919
use(base, src) and pw = trackUseNode(src).getAPropertyWrite() and rhs = pw.getRhs()
893920
|
@@ -931,6 +958,29 @@ module API {
931958
)
932959
}
933960

961+
pragma[nomagic]
962+
private int firstSpreadIndex(InvokeExpr expr) {
963+
result = min(int i | expr.getArgument(i) instanceof SpreadElement)
964+
}
965+
966+
pragma[nomagic]
967+
private InvokeExpr getAnInvocationWithSpread(DataFlow::SourceNode node, int i) {
968+
result = node.getAnInvocation().asExpr() and
969+
i = firstSpreadIndex(result)
970+
}
971+
972+
private predicate spreadArgumentPassing(TApiNode base, int i, DataFlow::Node spreadArray) {
973+
exists(
974+
DataFlow::Node use, DataFlow::SourceNode pred, int bound, InvokeExpr invoke, int spreadPos
975+
|
976+
use(base, use) and
977+
pred = trackUseNode(use, _, bound, "") and
978+
invoke = getAnInvocationWithSpread(pred, spreadPos) and
979+
spreadArray = invoke.getArgument(spreadPos).(SpreadElement).getOperand().flow() and
980+
i = bound + spreadPos
981+
)
982+
}
983+
934984
/**
935985
* Holds if `rhs` is the right-hand side of a definition of node `nd`.
936986
*/
@@ -1579,6 +1629,9 @@ module API {
15791629
/** Gets the edge label for the receiver. */
15801630
LabelReceiver receiver() { any() }
15811631

1632+
/** Gets the edge label for a spread argument passed at index `i`. */
1633+
LabelSpreadArgument spreadArgument(int i) { result.getIndex() = i }
1634+
15821635
/** Gets the `return` edge label. */
15831636
LabelReturn return() { any() }
15841637

@@ -1628,6 +1681,7 @@ module API {
16281681
} or
16291682
MkLabelReceiver() or
16301683
MkLabelReturn() or
1684+
MkLabelSpreadArgument(int index) { index = [0 .. 10] } or
16311685
MkLabelDecoratedClass() or
16321686
MkLabelDecoratedMember() or
16331687
MkLabelDecoratedParameter() or
@@ -1743,6 +1797,21 @@ module API {
17431797
override string toString() { result = "getReceiver()" }
17441798
}
17451799

1800+
/** A label representing an array passed as a spread argument at a given index. */
1801+
class LabelSpreadArgument extends ApiLabel, MkLabelSpreadArgument {
1802+
private int index;
1803+
1804+
LabelSpreadArgument() { this = MkLabelSpreadArgument(index) }
1805+
1806+
/** Gets the argument index at which the spread argument appears. */
1807+
int getIndex() { result = index }
1808+
1809+
override string toString() {
1810+
// Note: This refers to the internal edge that has no corresponding method on API::Node
1811+
result = "getSpreadArgument(" + index + ")"
1812+
}
1813+
}
1814+
17461815
/** A label for a function that is a wrapper around another function. */
17471816
class LabelForwardingFunction extends ApiLabel, MkLabelForwardingFunction {
17481817
override string toString() { result = "getForwardingFunction()" }

Diff for: javascript/ql/lib/semmle/javascript/dataflow/Sources.qll

+6
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,12 @@ private module Cached {
254254
cached
255255
predicate invocation(DataFlow::SourceNode func, DataFlow::InvokeNode invoke) {
256256
hasLocalSource(invoke.getCalleeNode(), func)
257+
or
258+
exists(ClassDefinition cls, SuperCall call |
259+
hasLocalSource(cls.getSuperClass().flow(), func) and
260+
call.getBinder() = cls.getConstructor().getBody() and
261+
invoke = call.flow()
262+
)
257263
}
258264

259265
/**

Diff for: javascript/ql/test/ApiGraphs/spread/tst.js

+18
Original file line numberDiff line numberDiff line change
@@ -9,3 +9,21 @@ function f() {
99
lib.m1({
1010
...f()
1111
})
12+
13+
function getArgs() {
14+
return [
15+
'x', /* def=moduleImport("something").getMember("exports").getMember("m2").getSpreadArgument(0).getArrayElement() */
16+
'y', /* def=moduleImport("something").getMember("exports").getMember("m2").getSpreadArgument(0).getArrayElement() */
17+
]
18+
}
19+
20+
lib.m2(...getArgs());
21+
22+
function f3() {
23+
return [
24+
'x', /* def=moduleImport("something").getMember("exports").getMember("m3").getSpreadArgument(1).getArrayElement() */
25+
'y', /* def=moduleImport("something").getMember("exports").getMember("m3").getSpreadArgument(1).getArrayElement() */
26+
]
27+
}
28+
29+
lib.m3.bind(undefined, 1)(...f3());

Diff for: javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected

+12
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#select
22
| 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 |
3+
| 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 |
34
| 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 |
45
| 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 |
56
| 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
3132
| apollo.serverSide.ts:8:13:8:17 | files | apollo.serverSide.ts:8:28:8:31 | file | provenance | |
3233
| apollo.serverSide.ts:8:28:8:31 | file | apollo.serverSide.ts:8:43:8:46 | file | provenance | |
3334
| apollo.serverSide.ts:8:43:8:46 | file | apollo.serverSide.ts:8:43:8:50 | file.url | provenance | |
35+
| apollo.serverSide.ts:17:34:17:42 | files | apollo.serverSide.ts:18:11:18:15 | files | provenance | |
36+
| apollo.serverSide.ts:17:34:17:42 | { files } | apollo.serverSide.ts:17:34:17:42 | files | provenance | |
37+
| apollo.serverSide.ts:18:11:18:15 | files | apollo.serverSide.ts:18:26:18:29 | file | provenance | |
38+
| apollo.serverSide.ts:18:26:18:29 | file | apollo.serverSide.ts:18:41:18:44 | file | provenance | |
39+
| apollo.serverSide.ts:18:41:18:44 | file | apollo.serverSide.ts:18:41:18:48 | file.url | provenance | |
3440
| axiosInterceptors.serverSide.js:19:11:19:17 | { url } | axiosInterceptors.serverSide.js:19:11:19:28 | url | provenance | |
3541
| axiosInterceptors.serverSide.js:19:11:19:28 | url | axiosInterceptors.serverSide.js:20:23:20:25 | url | provenance | |
3642
| axiosInterceptors.serverSide.js:19:21:19:28 | req.body | axiosInterceptors.serverSide.js:19:11:19:17 | { url } | provenance | |
@@ -91,6 +97,12 @@ nodes
9197
| apollo.serverSide.ts:8:28:8:31 | file | semmle.label | file |
9298
| apollo.serverSide.ts:8:43:8:46 | file | semmle.label | file |
9399
| apollo.serverSide.ts:8:43:8:50 | file.url | semmle.label | file.url |
100+
| apollo.serverSide.ts:17:34:17:42 | files | semmle.label | files |
101+
| apollo.serverSide.ts:17:34:17:42 | { files } | semmle.label | { files } |
102+
| apollo.serverSide.ts:18:11:18:15 | files | semmle.label | files |
103+
| apollo.serverSide.ts:18:26:18:29 | file | semmle.label | file |
104+
| apollo.serverSide.ts:18:41:18:44 | file | semmle.label | file |
105+
| apollo.serverSide.ts:18:41:18:48 | file.url | semmle.label | file.url |
94106
| axiosInterceptors.serverSide.js:11:26:11:40 | userProvidedUrl | semmle.label | userProvidedUrl |
95107
| axiosInterceptors.serverSide.js:19:11:19:17 | { url } | semmle.label | { url } |
96108
| axiosInterceptors.serverSide.js:19:11:19:28 | url | semmle.label | url |

Diff for: javascript/ql/test/query-tests/Security/CWE-918/apollo.serverSide.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ function createApolloServer(typeDefs) {
1414

1515
const resolvers2 = {
1616
Mutation: {
17-
downloadFiles: async (_, { files }) => { // $ MISSING: Source[js/request-forgery]
18-
files.forEach((file) => { get(file.url, (res) => {}); }); // $ MISSING: Alert[js/request-forgery] Sink[js/request-forgery]
17+
downloadFiles: async (_, { files }) => { // $ Source[js/request-forgery]
18+
files.forEach((file) => { get(file.url, (res) => {}); }); // $ Alert[js/request-forgery] Sink[js/request-forgery]
1919
return true;
2020
},
2121
},

0 commit comments

Comments
 (0)