hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-24 16:25:15 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #16739 from RasmusWL/js-array-steps

Browse Source 
JS: Allow many Array steps to be used in type-tracking
This commit is contained in:
Asger F
2024-06-20 11:39:46 +02:00
committed by GitHub
parent 6dbdc9e17f 596102d3fb
commit a36e39359f
8 changed files with 1048 additions and 865 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
javascript/ql/lib/change-notes/2024-06-14-type-tracking-array-steps.md Normal file
View File

@@ -0,0 +1,5 @@
---
category: minorAnalysis
---
* Enabled type-tracking to follow content through array methods
* Improved modeling of `Array.prototype.splice` for when it is called with more than two arguments

57
javascript/ql/lib/semmle/javascript/Arrays.qll
View File

@@ -77,8 +77,12 @@ module ArrayTaintTracking {
succ = call.getReceiver().getALocalSource() and
call.getCalleeName() = ["push", "unshift"]
or
// `array.splice(i, del, e)`: if `e` is tainted, then so is `array`.
pred = call.getArgument(2) and
// `array.splice(i, del, e1, e2, ...)`: if any item is tainted, then so is `array`.
pred = call.getArgument(any(int i | i >= 2)) and
succ.(DataFlow::SourceNode).getAMethodCall("splice") = call
or
// `array.splice(i, del, ...e)`: if `e` is tainted, then so is `array`.
pred = call.getASpreadArgument() and
succ.(DataFlow::SourceNode).getAMethodCall("splice") = call
or
// `e = array.pop()`, `e = array.shift()`, or similar: if `array` is tainted, then so is `e`.
@@ -115,9 +119,9 @@ private module ArrayDataFlow {
* A step modeling the creation of an Array using the `Array.from(x)` method.
* The step copies the elements of the argument (set, array, or iterator elements) into the resulting array.
*/
private class ArrayFrom extends DataFlow::SharedFlowStep {
private class ArrayFrom extends PreCallGraphStep {
override predicate loadStoreStep(
DataFlow::Node pred, DataFlow::Node succ, string fromProp, string toProp
DataFlow::Node pred, DataFlow::SourceNode succ, string fromProp, string toProp
) {
exists(DataFlow::CallNode call |
call = arrayFromCall() and
@@ -135,9 +139,9 @@ private module ArrayDataFlow {
*
* Such a step can occur both with the `push` and `unshift` methods, or when creating a new array.
*/
private class ArrayCopySpread extends DataFlow::SharedFlowStep {
private class ArrayCopySpread extends PreCallGraphStep {
override predicate loadStoreStep(
DataFlow::Node pred, DataFlow::Node succ, string fromProp, string toProp
DataFlow::Node pred, DataFlow::SourceNode succ, string fromProp, string toProp
) {
fromProp = arrayLikeElement() and
toProp = arrayElement() and
@@ -156,7 +160,7 @@ private module ArrayDataFlow {
/**
* A step for storing an element on an array using `arr.push(e)` or `arr.unshift(e)`.
*/
private class ArrayAppendStep extends DataFlow::SharedFlowStep {
private class ArrayAppendStep extends PreCallGraphStep {
override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
prop = arrayElement() and
exists(DataFlow::MethodCallNode call |
@@ -187,7 +191,7 @@ private module ArrayDataFlow {
* A step for reading/writing an element from an array inside a for-loop.
* E.g. a read from `foo[i]` to `bar` in `for(var i = 0; i < arr.length; i++) {bar = foo[i]}`.
*/
private class ArrayIndexingStep extends DataFlow::SharedFlowStep {
private class ArrayIndexingStep extends PreCallGraphStep {
override predicate loadStep(DataFlow::Node obj, DataFlow::Node element, string prop) {
exists(ArrayIndexingAccess access |
prop = arrayElement() and
@@ -209,7 +213,7 @@ private module ArrayDataFlow {
* A step for retrieving an element from an array using `.pop()`, `.shift()`, or `.at()`.
* E.g. `array.pop()`.
*/
private class ArrayPopStep extends DataFlow::SharedFlowStep {
private class ArrayPopStep extends PreCallGraphStep {
override predicate loadStep(DataFlow::Node obj, DataFlow::Node element, string prop) {
exists(DataFlow::MethodCallNode call |
call.getMethodName() = ["pop", "shift", "at"] and
@@ -274,25 +278,38 @@ private module ArrayDataFlow {
/**
* A step modeling that `splice` can insert elements into an array.
* For example in `array.splice(i, del, e)`: if `e` is tainted, then so is `array
* For example in `array.splice(i, del, e1, e2, ...)`: if any item is tainted, then so is `array`
*/
private class ArraySpliceStep extends DataFlow::SharedFlowStep {
private class ArraySpliceStep extends PreCallGraphStep {
override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
exists(DataFlow::MethodCallNode call |
call.getMethodName() = "splice" and
prop = arrayElement() and
element = call.getArgument(2) and
element = call.getArgument(any(int i | i >= 2)) and
call = obj.getAMethodCall()
)
}
override predicate loadStoreStep(
DataFlow::Node pred, DataFlow::SourceNode succ, string fromProp, string toProp
) {
fromProp = arrayLikeElement() and
toProp = arrayElement() and
// `array.splice(i, del, ...arr)` variant
exists(DataFlow::MethodCallNode mcn |
mcn.getMethodName() = "splice" and
pred = mcn.getASpreadArgument() and
succ = mcn.getReceiver().getALocalSource()
)
}
}
/**
* A step for modeling `concat`.
* For example in `e = arr1.concat(arr2, arr3)`: if any of the `arr` is tainted, then so is `e`.
*/
private class ArrayConcatStep extends DataFlow::SharedFlowStep {
override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
private class ArrayConcatStep extends PreCallGraphStep {
override predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
exists(DataFlow::MethodCallNode call |
call.getMethodName() = "concat" and
prop = arrayElement() and
@@ -305,8 +322,8 @@ private module ArrayDataFlow {
/**
* A step for modeling that elements from an array `arr` also appear in the result from calling `slice`/`splice`/`filter`.
*/
private class ArraySliceStep extends DataFlow::SharedFlowStep {
override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
private class ArraySliceStep extends PreCallGraphStep {
override predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
exists(DataFlow::MethodCallNode call |
call.getMethodName() = ["slice", "splice", "filter"] and
prop = arrayElement() and
@@ -319,7 +336,7 @@ private module ArrayDataFlow {
/**
* A step modeling that elements from an array `arr` are received by calling `find`.
*/
private class ArrayFindStep extends DataFlow::SharedFlowStep {
private class ArrayFindStep extends PreCallGraphStep {
override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
exists(DataFlow::CallNode call |
call = arrayFindCall(pred) and
@@ -382,7 +399,7 @@ private module ArrayLibraries {
* E.g. `array-union` that creates a union of multiple arrays, or `array-uniq` that creates an array with unique elements.
*/
DataFlow::CallNode arrayCopyCall(DataFlow::Node array) {
result = API::moduleImport(["array-union", "array-uniq", "uniq"]).getACall() and
result = DataFlow::moduleImport(["array-union", "array-uniq", "uniq"]).getACall() and
array = result.getAnArgument()
}
@@ -401,8 +418,8 @@ private module ArrayLibraries {
/**
* A loadStoreStep for a library that copies the elements of an array into another array.
*/
private class ArrayCopyLoadStore extends DataFlow::SharedFlowStep {
override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
private class ArrayCopyLoadStore extends PreCallGraphStep {
override predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
exists(DataFlow::CallNode call |
call = arrayCopyCall(pred) and
succ = call and

28
javascript/ql/test/library-tests/Arrays/DataFlow.expected
View File

@@ -3,20 +3,22 @@
| arrays.js:2:16:2:23 | "source" | arrays.js:15:27:15:27 | e |
| arrays.js:2:16:2:23 | "source" | arrays.js:16:23:16:23 | e |
| arrays.js:2:16:2:23 | "source" | arrays.js:20:8:20:16 | arr.pop() |
| arrays.js:2:16:2:23 | "source" | arrays.js:52:10:52:10 | x |
| arrays.js:2:16:2:23 | "source" | arrays.js:56:10:56:10 | x |
| arrays.js:2:16:2:23 | "source" | arrays.js:60:10:60:10 | x |
| arrays.js:2:16:2:23 | "source" | arrays.js:66:10:66:10 | x |
| arrays.js:2:16:2:23 | "source" | arrays.js:71:10:71:10 | x |
| arrays.js:2:16:2:23 | "source" | arrays.js:74:8:74:29 | arr.fin ... llback) |
| arrays.js:2:16:2:23 | "source" | arrays.js:77:8:77:35 | arrayFi ... llback) |
| arrays.js:2:16:2:23 | "source" | arrays.js:81:10:81:10 | x |
| arrays.js:2:16:2:23 | "source" | arrays.js:84:8:84:17 | arr.at(-1) |
| arrays.js:2:16:2:23 | "source" | arrays.js:39:8:39:24 | arr4_spread.pop() |
| arrays.js:2:16:2:23 | "source" | arrays.js:61:10:61:10 | x |
| arrays.js:2:16:2:23 | "source" | arrays.js:65:10:65:10 | x |
| arrays.js:2:16:2:23 | "source" | arrays.js:69:10:69:10 | x |
| arrays.js:2:16:2:23 | "source" | arrays.js:75:10:75:10 | x |
| arrays.js:2:16:2:23 | "source" | arrays.js:80:10:80:10 | x |
| arrays.js:2:16:2:23 | "source" | arrays.js:83:8:83:29 | arr.fin ... llback) |
| arrays.js:2:16:2:23 | "source" | arrays.js:86:8:86:35 | arrayFi ... llback) |
| arrays.js:2:16:2:23 | "source" | arrays.js:90:10:90:10 | x |
| arrays.js:2:16:2:23 | "source" | arrays.js:93:8:93:17 | arr.at(-1) |
| arrays.js:18:22:18:29 | "source" | arrays.js:18:50:18:50 | e |
| arrays.js:22:15:22:22 | "source" | arrays.js:23:8:23:17 | arr2.pop() |
| arrays.js:25:15:25:22 | "source" | arrays.js:26:8:26:17 | arr3.pop() |
| arrays.js:29:21:29:28 | "source" | arrays.js:30:8:30:17 | arr4.pop() |
| arrays.js:29:21:29:28 | "source" | arrays.js:33:8:33:17 | arr5.pop() |
| arrays.js:29:21:29:28 | "source" | arrays.js:35:8:35:26 | arr5.slice(2).pop() |
| arrays.js:29:21:29:28 | "source" | arrays.js:41:8:41:17 | arr6.pop() |
| arrays.js:44:4:44:11 | "source" | arrays.js:45:10:45:18 | ary.pop() |
| arrays.js:29:21:29:28 | "source" | arrays.js:42:8:42:17 | arr5.pop() |
| arrays.js:29:21:29:28 | "source" | arrays.js:44:8:44:26 | arr5.slice(2).pop() |
| arrays.js:29:21:29:28 | "source" | arrays.js:50:8:50:17 | arr6.pop() |
| arrays.js:33:37:33:44 | "source" | arrays.js:35:8:35:25 | arr4_variant.pop() |
| arrays.js:53:4:53:11 | "source" | arrays.js:54:10:54:18 | ary.pop() |

36
javascript/ql/test/library-tests/Arrays/TaintFlow.expected
View File

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

13
javascript/ql/test/library-tests/Arrays/arrays.js
View File

@@ -29,6 +29,15 @@
arr4.splice(0, 0, "source");
sink(arr4.pop()); // NOT OK
var arr4_variant = [];
arr4_variant.splice(0, 0, "safe", "source");
arr4_variant.pop();
sink(arr4_variant.pop()); // NOT OK
var arr4_spread = [];
arr4_spread.splice(0, 0, ...arr);
sink(arr4_spread.pop()); // NOT OK
var arr5 = [].concat(arr4);
sink(arr5.pop()); // NOT OK
@@ -46,7 +55,7 @@
sink(ary); // OK - its the array itself, not an element.
});
sink(arr[0]); // OK - tuple like usage.
sink(arr[0]); // OK - tuple like usage.
for (const x of arr) {
sink(x); // NOT OK
@@ -59,7 +68,7 @@
for (const x of [...arr]) {
sink(x); // NOT OK
}
var arr7 = [];
arr7.push(...arr);
for (const x of arr7) {

1771
javascript/ql/test/library-tests/Arrays/printAst.expected
View File

File diff suppressed because it is too large Load Diff

1
javascript/ql/test/library-tests/frameworks/Collections/test.expected
View File

@@ -26,6 +26,7 @@ typeTracking
| tst.js:2:16:2:23 | source() | tst.js:37:14:37:14 | e |
| tst.js:2:16:2:23 | source() | tst.js:41:14:41:14 | e |
| tst.js:2:16:2:23 | source() | tst.js:45:14:45:14 | e |
| tst.js:2:16:2:23 | source() | tst.js:49:14:49:14 | e |
| tst.js:2:16:2:23 | source() | tst.js:53:8:53:21 | map.get("key") |
| tst.js:2:16:2:23 | source() | tst.js:59:8:59:22 | map2.get("foo") |
| tst.js:2:16:2:23 | source() | tst.js:64:8:64:26 | map3.get(unknown()) |

2
javascript/ql/test/library-tests/frameworks/Collections/tst.js
View File

@@ -47,7 +47,7 @@
}
for (const e of Array.from(set)) {
sink(e); // NOT OK (not caught by type-tracking, as it doesn't include array steps).
sink(e); // NOT OK
}
sink(map.get("key")); // NOT OK.