Skip to content

Commit a36e393

Browse files
asgerfasgerf
authored
Merge pull request github#16739 from RasmusWL/js-array-steps
JS: Allow many Array steps to be used in type-tracking
2 parents 6dbdc9e + 596102d commit a36e393

File tree

8 files changed

+1048
-865
lines changed

8 files changed

+1048
-865
lines changed

‎javascript/ql/lib/change-notes/2024-06-14-type-tracking-array-steps.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Enabled type-tracking to follow content through array methods
5+
* Improved modeling of `Array.prototype.splice` for when it is called with more than two arguments

‎javascript/ql/lib/semmle/javascript/Arrays.qll

Lines changed: 37 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,12 @@ module ArrayTaintTracking {
7777
succ = call.getReceiver().getALocalSource() and
7878
call.getCalleeName() = ["push", "unshift"]
7979
or
80-
// `array.splice(i, del, e)`: if `e` is tainted, then so is `array`.
81-
pred = call.getArgument(2) and
80+
// `array.splice(i, del, e1, e2, ...)`: if any item is tainted, then so is `array`.
81+
pred = call.getArgument(any(int i | i >= 2)) and
82+
succ.(DataFlow::SourceNode).getAMethodCall("splice") = call
83+
or
84+
// `array.splice(i, del, ...e)`: if `e` is tainted, then so is `array`.
85+
pred = call.getASpreadArgument() and
8286
succ.(DataFlow::SourceNode).getAMethodCall("splice") = call
8387
or
8488
// `e = array.pop()`, `e = array.shift()`, or similar: if `array` is tainted, then so is `e`.
@@ -115,9 +119,9 @@ private module ArrayDataFlow {
115119
* A step modeling the creation of an Array using the `Array.from(x)` method.
116120
* The step copies the elements of the argument (set, array, or iterator elements) into the resulting array.
117121
*/
118-
private class ArrayFrom extends DataFlow::SharedFlowStep {
122+
private class ArrayFrom extends PreCallGraphStep {
119123
override predicate loadStoreStep(
120-
DataFlow::Node pred, DataFlow::Node succ, string fromProp, string toProp
124+
DataFlow::Node pred, DataFlow::SourceNode succ, string fromProp, string toProp
121125
) {
122126
exists(DataFlow::CallNode call |
123127
call = arrayFromCall() and
@@ -135,9 +139,9 @@ private module ArrayDataFlow {
135139
*
136140
* Such a step can occur both with the `push` and `unshift` methods, or when creating a new array.
137141
*/
138-
private class ArrayCopySpread extends DataFlow::SharedFlowStep {
142+
private class ArrayCopySpread extends PreCallGraphStep {
139143
override predicate loadStoreStep(
140-
DataFlow::Node pred, DataFlow::Node succ, string fromProp, string toProp
144+
DataFlow::Node pred, DataFlow::SourceNode succ, string fromProp, string toProp
141145
) {
142146
fromProp = arrayLikeElement() and
143147
toProp = arrayElement() and
@@ -156,7 +160,7 @@ private module ArrayDataFlow {
156160
/**
157161
* A step for storing an element on an array using `arr.push(e)` or `arr.unshift(e)`.
158162
*/
159-
private class ArrayAppendStep extends DataFlow::SharedFlowStep {
163+
private class ArrayAppendStep extends PreCallGraphStep {
160164
override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
161165
prop = arrayElement() and
162166
exists(DataFlow::MethodCallNode call |
@@ -187,7 +191,7 @@ private module ArrayDataFlow {
187191
* A step for reading/writing an element from an array inside a for-loop.
188192
* E.g. a read from `foo[i]` to `bar` in `for(var i = 0; i < arr.length; i++) {bar = foo[i]}`.
189193
*/
190-
private class ArrayIndexingStep extends DataFlow::SharedFlowStep {
194+
private class ArrayIndexingStep extends PreCallGraphStep {
191195
override predicate loadStep(DataFlow::Node obj, DataFlow::Node element, string prop) {
192196
exists(ArrayIndexingAccess access |
193197
prop = arrayElement() and
@@ -209,7 +213,7 @@ private module ArrayDataFlow {
209213
* A step for retrieving an element from an array using `.pop()`, `.shift()`, or `.at()`.
210214
* E.g. `array.pop()`.
211215
*/
212-
private class ArrayPopStep extends DataFlow::SharedFlowStep {
216+
private class ArrayPopStep extends PreCallGraphStep {
213217
override predicate loadStep(DataFlow::Node obj, DataFlow::Node element, string prop) {
214218
exists(DataFlow::MethodCallNode call |
215219
call.getMethodName() = ["pop", "shift", "at"] and
@@ -274,25 +278,38 @@ private module ArrayDataFlow {
274278

275279
/**
276280
* A step modeling that `splice` can insert elements into an array.
277-
* For example in `array.splice(i, del, e)`: if `e` is tainted, then so is `array
281+
* For example in `array.splice(i, del, e1, e2, ...)`: if any item is tainted, then so is `array`
278282
*/
279-
private class ArraySpliceStep extends DataFlow::SharedFlowStep {
283+
private class ArraySpliceStep extends PreCallGraphStep {
280284
override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
281285
exists(DataFlow::MethodCallNode call |
282286
call.getMethodName() = "splice" and
283287
prop = arrayElement() and
284-
element = call.getArgument(2) and
288+
element = call.getArgument(any(int i | i >= 2)) and
285289
call = obj.getAMethodCall()
286290
)
287291
}
292+
293+
override predicate loadStoreStep(
294+
DataFlow::Node pred, DataFlow::SourceNode succ, string fromProp, string toProp
295+
) {
296+
fromProp = arrayLikeElement() and
297+
toProp = arrayElement() and
298+
// `array.splice(i, del, ...arr)` variant
299+
exists(DataFlow::MethodCallNode mcn |
300+
mcn.getMethodName() = "splice" and
301+
pred = mcn.getASpreadArgument() and
302+
succ = mcn.getReceiver().getALocalSource()
303+
)
304+
}
288305
}
289306

290307
/**
291308
* A step for modeling `concat`.
292309
* For example in `e = arr1.concat(arr2, arr3)`: if any of the `arr` is tainted, then so is `e`.
293310
*/
294-
private class ArrayConcatStep extends DataFlow::SharedFlowStep {
295-
override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
311+
private class ArrayConcatStep extends PreCallGraphStep {
312+
override predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
296313
exists(DataFlow::MethodCallNode call |
297314
call.getMethodName() = "concat" and
298315
prop = arrayElement() and
@@ -305,8 +322,8 @@ private module ArrayDataFlow {
305322
/**
306323
* A step for modeling that elements from an array `arr` also appear in the result from calling `slice`/`splice`/`filter`.
307324
*/
308-
private class ArraySliceStep extends DataFlow::SharedFlowStep {
309-
override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
325+
private class ArraySliceStep extends PreCallGraphStep {
326+
override predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
310327
exists(DataFlow::MethodCallNode call |
311328
call.getMethodName() = ["slice", "splice", "filter"] and
312329
prop = arrayElement() and
@@ -319,7 +336,7 @@ private module ArrayDataFlow {
319336
/**
320337
* A step modeling that elements from an array `arr` are received by calling `find`.
321338
*/
322-
private class ArrayFindStep extends DataFlow::SharedFlowStep {
339+
private class ArrayFindStep extends PreCallGraphStep {
323340
override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
324341
exists(DataFlow::CallNode call |
325342
call = arrayFindCall(pred) and
@@ -382,7 +399,7 @@ private module ArrayLibraries {
382399
* E.g. `array-union` that creates a union of multiple arrays, or `array-uniq` that creates an array with unique elements.
383400
*/
384401
DataFlow::CallNode arrayCopyCall(DataFlow::Node array) {
385-
result = API::moduleImport(["array-union", "array-uniq", "uniq"]).getACall() and
402+
result = DataFlow::moduleImport(["array-union", "array-uniq", "uniq"]).getACall() and
386403
array = result.getAnArgument()
387404
}
388405

@@ -401,8 +418,8 @@ private module ArrayLibraries {
401418
/**
402419
* A loadStoreStep for a library that copies the elements of an array into another array.
403420
*/
404-
private class ArrayCopyLoadStore extends DataFlow::SharedFlowStep {
405-
override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
421+
private class ArrayCopyLoadStore extends PreCallGraphStep {
422+
override predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
406423
exists(DataFlow::CallNode call |
407424
call = arrayCopyCall(pred) and
408425
succ = call and

‎javascript/ql/test/library-tests/Arrays/DataFlow.expected

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,20 +3,22 @@
33
| arrays.js:2:16:2:23 | "source" | arrays.js:15:27:15:27 | e |
44
| arrays.js:2:16:2:23 | "source" | arrays.js:16:23:16:23 | e |
55
| arrays.js:2:16:2:23 | "source" | arrays.js:20:8:20:16 | arr.pop() |
6-
| arrays.js:2:16:2:23 | "source" | arrays.js:52:10:52:10 | x |
7-
| arrays.js:2:16:2:23 | "source" | arrays.js:56:10:56:10 | x |
8-
| arrays.js:2:16:2:23 | "source" | arrays.js:60:10:60:10 | x |
9-
| arrays.js:2:16:2:23 | "source" | arrays.js:66:10:66:10 | x |
10-
| arrays.js:2:16:2:23 | "source" | arrays.js:71:10:71:10 | x |
11-
| arrays.js:2:16:2:23 | "source" | arrays.js:74:8:74:29 | arr.fin ... llback) |
12-
| arrays.js:2:16:2:23 | "source" | arrays.js:77:8:77:35 | arrayFi ... llback) |
13-
| arrays.js:2:16:2:23 | "source" | arrays.js:81:10:81:10 | x |
14-
| arrays.js:2:16:2:23 | "source" | arrays.js:84:8:84:17 | arr.at(-1) |
6+
| arrays.js:2:16:2:23 | "source" | arrays.js:39:8:39:24 | arr4_spread.pop() |
7+
| arrays.js:2:16:2:23 | "source" | arrays.js:61:10:61:10 | x |
8+
| arrays.js:2:16:2:23 | "source" | arrays.js:65:10:65:10 | x |
9+
| arrays.js:2:16:2:23 | "source" | arrays.js:69:10:69:10 | x |
10+
| arrays.js:2:16:2:23 | "source" | arrays.js:75:10:75:10 | x |
11+
| arrays.js:2:16:2:23 | "source" | arrays.js:80:10:80:10 | x |
12+
| arrays.js:2:16:2:23 | "source" | arrays.js:83:8:83:29 | arr.fin ... llback) |
13+
| arrays.js:2:16:2:23 | "source" | arrays.js:86:8:86:35 | arrayFi ... llback) |
14+
| arrays.js:2:16:2:23 | "source" | arrays.js:90:10:90:10 | x |
15+
| arrays.js:2:16:2:23 | "source" | arrays.js:93:8:93:17 | arr.at(-1) |
1516
| arrays.js:18:22:18:29 | "source" | arrays.js:18:50:18:50 | e |
1617
| arrays.js:22:15:22:22 | "source" | arrays.js:23:8:23:17 | arr2.pop() |
1718
| arrays.js:25:15:25:22 | "source" | arrays.js:26:8:26:17 | arr3.pop() |
1819
| arrays.js:29:21:29:28 | "source" | arrays.js:30:8:30:17 | arr4.pop() |
19-
| arrays.js:29:21:29:28 | "source" | arrays.js:33:8:33:17 | arr5.pop() |
20-
| arrays.js:29:21:29:28 | "source" | arrays.js:35:8:35:26 | arr5.slice(2).pop() |
21-
| arrays.js:29:21:29:28 | "source" | arrays.js:41:8:41:17 | arr6.pop() |
22-
| arrays.js:44:4:44:11 | "source" | arrays.js:45:10:45:18 | ary.pop() |
20+
| arrays.js:29:21:29:28 | "source" | arrays.js:42:8:42:17 | arr5.pop() |
21+
| arrays.js:29:21:29:28 | "source" | arrays.js:44:8:44:26 | arr5.slice(2).pop() |
22+
| arrays.js:29:21:29:28 | "source" | arrays.js:50:8:50:17 | arr6.pop() |
23+
| arrays.js:33:37:33:44 | "source" | arrays.js:35:8:35:25 | arr4_variant.pop() |
24+
| arrays.js:53:4:53:11 | "source" | arrays.js:54:10:54:18 | ary.pop() |

‎javascript/ql/test/library-tests/Arrays/TaintFlow.expected

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,24 +3,26 @@
33
| arrays.js:2:16:2:23 | "source" | arrays.js:15:27:15:27 | e |
44
| arrays.js:2:16:2:23 | "source" | arrays.js:16:23:16:23 | e |
55
| arrays.js:2:16:2:23 | "source" | arrays.js:20:8:20:16 | arr.pop() |
6-
| arrays.js:2:16:2:23 | "source" | arrays.js:49:8:49:13 | arr[0] |
7-
| arrays.js:2:16:2:23 | "source" | arrays.js:52:10:52:10 | x |
8-
| arrays.js:2:16:2:23 | "source" | arrays.js:56:10:56:10 | x |
9-
| arrays.js:2:16:2:23 | "source" | arrays.js:60:10:60:10 | x |
10-
| arrays.js:2:16:2:23 | "source" | arrays.js:66:10:66:10 | x |
11-
| arrays.js:2:16:2:23 | "source" | arrays.js:71:10:71:10 | x |
12-
| arrays.js:2:16:2:23 | "source" | arrays.js:74:8:74:29 | arr.fin ... llback) |
13-
| arrays.js:2:16:2:23 | "source" | arrays.js:77:8:77:35 | arrayFi ... llback) |
14-
| arrays.js:2:16:2:23 | "source" | arrays.js:81:10:81:10 | x |
15-
| arrays.js:2:16:2:23 | "source" | arrays.js:84:8:84:17 | arr.at(-1) |
6+
| arrays.js:2:16:2:23 | "source" | arrays.js:39:8:39:24 | arr4_spread.pop() |
7+
| arrays.js:2:16:2:23 | "source" | arrays.js:58:8:58:13 | arr[0] |
8+
| arrays.js:2:16:2:23 | "source" | arrays.js:61:10:61:10 | x |
9+
| arrays.js:2:16:2:23 | "source" | arrays.js:65:10:65:10 | x |
10+
| arrays.js:2:16:2:23 | "source" | arrays.js:69:10:69:10 | x |
11+
| arrays.js:2:16:2:23 | "source" | arrays.js:75:10:75:10 | x |
12+
| arrays.js:2:16:2:23 | "source" | arrays.js:80:10:80:10 | x |
13+
| arrays.js:2:16:2:23 | "source" | arrays.js:83:8:83:29 | arr.fin ... llback) |
14+
| arrays.js:2:16:2:23 | "source" | arrays.js:86:8:86:35 | arrayFi ... llback) |
15+
| arrays.js:2:16:2:23 | "source" | arrays.js:90:10:90:10 | x |
16+
| arrays.js:2:16:2:23 | "source" | arrays.js:93:8:93:17 | arr.at(-1) |
1617
| arrays.js:18:22:18:29 | "source" | arrays.js:18:50:18:50 | e |
1718
| arrays.js:22:15:22:22 | "source" | arrays.js:23:8:23:17 | arr2.pop() |
1819
| arrays.js:25:15:25:22 | "source" | arrays.js:26:8:26:17 | arr3.pop() |
1920
| arrays.js:29:21:29:28 | "source" | arrays.js:30:8:30:17 | arr4.pop() |
20-
| arrays.js:29:21:29:28 | "source" | arrays.js:33:8:33:17 | arr5.pop() |
21-
| arrays.js:29:21:29:28 | "source" | arrays.js:35:8:35:26 | arr5.slice(2).pop() |
22-
| arrays.js:29:21:29:28 | "source" | arrays.js:41:8:41:17 | arr6.pop() |
23-
| arrays.js:44:4:44:11 | "source" | arrays.js:45:10:45:18 | ary.pop() |
24-
| arrays.js:44:4:44:11 | "source" | arrays.js:46:10:46:12 | ary |
25-
| arrays.js:86:9:86:16 | "source" | arrays.js:86:8:86:34 | ["sourc ... ) => x) |
26-
| arrays.js:87:9:87:16 | "source" | arrays.js:87:8:87:36 | ["sourc ... => !!x) |
21+
| arrays.js:29:21:29:28 | "source" | arrays.js:42:8:42:17 | arr5.pop() |
22+
| arrays.js:29:21:29:28 | "source" | arrays.js:44:8:44:26 | arr5.slice(2).pop() |
23+
| arrays.js:29:21:29:28 | "source" | arrays.js:50:8:50:17 | arr6.pop() |
24+
| arrays.js:33:37:33:44 | "source" | arrays.js:35:8:35:25 | arr4_variant.pop() |
25+
| arrays.js:53:4:53:11 | "source" | arrays.js:54:10:54:18 | ary.pop() |
26+
| arrays.js:53:4:53:11 | "source" | arrays.js:55:10:55:12 | ary |
27+
| arrays.js:95:9:95:16 | "source" | arrays.js:95:8:95:34 | ["sourc ... ) => x) |
28+
| arrays.js:96:9:96:16 | "source" | arrays.js:96:8:96:36 | ["sourc ... => !!x) |

‎javascript/ql/test/library-tests/Arrays/arrays.js

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,15 @@
2929
arr4.splice(0, 0, "source");
3030
sink(arr4.pop()); // NOT OK
3131

32+
var arr4_variant = [];
33+
arr4_variant.splice(0, 0, "safe", "source");
34+
arr4_variant.pop();
35+
sink(arr4_variant.pop()); // NOT OK
36+
37+
var arr4_spread = [];
38+
arr4_spread.splice(0, 0, ...arr);
39+
sink(arr4_spread.pop()); // NOT OK
40+
3241
var arr5 = [].concat(arr4);
3342
sink(arr5.pop()); // NOT OK
3443

@@ -46,7 +55,7 @@
4655
sink(ary); // OK - its the array itself, not an element.
4756
});
4857

49-
sink(arr[0]); // OK - tuple like usage.
58+
sink(arr[0]); // OK - tuple like usage.
5059

5160
for (const x of arr) {
5261
sink(x); // NOT OK
@@ -59,7 +68,7 @@
5968
for (const x of [...arr]) {
6069
sink(x); // NOT OK
6170
}
62-
71+
6372
var arr7 = [];
6473
arr7.push(...arr);
6574
for (const x of arr7) {

0 commit comments

Comments
 (0)