From 3786ad4277c7b63a9901a0dd4ae211383dd48f12 Mon Sep 17 00:00:00 2001 From: Napalys Date: Mon, 18 Nov 2024 12:44:49 +0100 Subject: [PATCH 1/8] JS: Add: test case for Map.groupBy --- javascript/ql/test/library-tests/TaintTracking/tst.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/javascript/ql/test/library-tests/TaintTracking/tst.js b/javascript/ql/test/library-tests/TaintTracking/tst.js index a5142e29685f..6ea2e755df4b 100644 --- a/javascript/ql/test/library-tests/TaintTracking/tst.js +++ b/javascript/ql/test/library-tests/TaintTracking/tst.js @@ -68,4 +68,7 @@ function test() { sink(x.toReversed()) // NOT OK const xReversed = x.toReversed(); sink(xReversed) // NOT OK + + sink(Map.groupBy(x, z => z)); // NOT OK -- This should be marked via taint step, but it is not. + sink(Custom.groupBy(x, z => z)); // OK } From c02ad65fdc27142b10d21b26915c9425422957ff Mon Sep 17 00:00:00 2001 From: Napalys Date: Mon, 18 Nov 2024 12:50:06 +0100 Subject: [PATCH 2/8] JS: Add: taint step for Map.groupBy function --- javascript/ql/lib/semmle/javascript/Collections.qll | 13 +++++++++++++ .../TaintTracking/BasicTaintTracking.expected | 1 + .../ql/test/library-tests/TaintTracking/tst.js | 2 +- 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/Collections.qll b/javascript/ql/lib/semmle/javascript/Collections.qll index a0e251554ff7..ff64a5568d16 100644 --- a/javascript/ql/lib/semmle/javascript/Collections.qll +++ b/javascript/ql/lib/semmle/javascript/Collections.qll @@ -151,4 +151,17 @@ private module CollectionDataFlow { ) } } + + /** + * A step for a call to `groupBy` on an iterable object. + */ + private class GroupByTaintStep extends TaintTracking::SharedTaintStep { + override predicate heapStep(DataFlow::Node pred, DataFlow::Node succ) { + exists(DataFlow::MethodCallNode call | + call = DataFlow::globalVarRef("Map").getAMemberCall("groupBy") and + pred = call.getArgument(0) and + succ = call + ) + } + } } diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected index 884657842058..248a35b907f8 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected @@ -244,6 +244,7 @@ typeInferenceMismatch | tst.js:2:13:2:20 | source() | tst.js:66:10:66:16 | xSorted | | tst.js:2:13:2:20 | source() | tst.js:68:10:68:23 | x.toReversed() | | tst.js:2:13:2:20 | source() | tst.js:70:10:70:18 | xReversed | +| tst.js:2:13:2:20 | source() | tst.js:72:10:72:31 | Map.gro ... z => z) | | xml.js:5:18:5:25 | source() | xml.js:8:14:8:17 | text | | xml.js:12:17:12:24 | source() | xml.js:13:14:13:19 | result | | xml.js:23:18:23:25 | source() | xml.js:20:14:20:17 | attr | diff --git a/javascript/ql/test/library-tests/TaintTracking/tst.js b/javascript/ql/test/library-tests/TaintTracking/tst.js index 6ea2e755df4b..c69895625cad 100644 --- a/javascript/ql/test/library-tests/TaintTracking/tst.js +++ b/javascript/ql/test/library-tests/TaintTracking/tst.js @@ -69,6 +69,6 @@ function test() { const xReversed = x.toReversed(); sink(xReversed) // NOT OK - sink(Map.groupBy(x, z => z)); // NOT OK -- This should be marked via taint step, but it is not. + sink(Map.groupBy(x, z => z)); // NOT OK sink(Custom.groupBy(x, z => z)); // OK } From 8ae05d8be474848cf325180e8507aac10ed63ee6 Mon Sep 17 00:00:00 2001 From: Napalys Date: Mon, 18 Nov 2024 12:52:49 +0100 Subject: [PATCH 3/8] JS: Add: test case for Object.groupBy --- javascript/ql/test/library-tests/TaintTracking/tst.js | 1 + 1 file changed, 1 insertion(+) diff --git a/javascript/ql/test/library-tests/TaintTracking/tst.js b/javascript/ql/test/library-tests/TaintTracking/tst.js index c69895625cad..d268c02e687b 100644 --- a/javascript/ql/test/library-tests/TaintTracking/tst.js +++ b/javascript/ql/test/library-tests/TaintTracking/tst.js @@ -71,4 +71,5 @@ function test() { sink(Map.groupBy(x, z => z)); // NOT OK sink(Custom.groupBy(x, z => z)); // OK + sink(Object.groupBy(x, z => z)); // NOT OK -- This should be marked via taint step, but it is not. } From 213ce225e0d3a3a426c20d91f48abfed94328f4b Mon Sep 17 00:00:00 2001 From: Napalys Date: Mon, 18 Nov 2024 12:58:07 +0100 Subject: [PATCH 4/8] JS: Add: taint step for Object.groupBy function, fixed test cases from 8ae05d8be474848cf325180e8507aac10ed63ee6 --- javascript/ql/lib/semmle/javascript/Collections.qll | 2 +- .../library-tests/TaintTracking/BasicTaintTracking.expected | 1 + javascript/ql/test/library-tests/TaintTracking/tst.js | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/Collections.qll b/javascript/ql/lib/semmle/javascript/Collections.qll index ff64a5568d16..c7348e603f4f 100644 --- a/javascript/ql/lib/semmle/javascript/Collections.qll +++ b/javascript/ql/lib/semmle/javascript/Collections.qll @@ -158,7 +158,7 @@ private module CollectionDataFlow { private class GroupByTaintStep extends TaintTracking::SharedTaintStep { override predicate heapStep(DataFlow::Node pred, DataFlow::Node succ) { exists(DataFlow::MethodCallNode call | - call = DataFlow::globalVarRef("Map").getAMemberCall("groupBy") and + call = DataFlow::globalVarRef(["Map", "Object"]).getAMemberCall("groupBy") and pred = call.getArgument(0) and succ = call ) diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected index 248a35b907f8..79be56cbb7e2 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected @@ -245,6 +245,7 @@ typeInferenceMismatch | tst.js:2:13:2:20 | source() | tst.js:68:10:68:23 | x.toReversed() | | tst.js:2:13:2:20 | source() | tst.js:70:10:70:18 | xReversed | | tst.js:2:13:2:20 | source() | tst.js:72:10:72:31 | Map.gro ... z => z) | +| tst.js:2:13:2:20 | source() | tst.js:74:10:74:34 | Object. ... z => z) | | xml.js:5:18:5:25 | source() | xml.js:8:14:8:17 | text | | xml.js:12:17:12:24 | source() | xml.js:13:14:13:19 | result | | xml.js:23:18:23:25 | source() | xml.js:20:14:20:17 | attr | diff --git a/javascript/ql/test/library-tests/TaintTracking/tst.js b/javascript/ql/test/library-tests/TaintTracking/tst.js index d268c02e687b..0fab561954de 100644 --- a/javascript/ql/test/library-tests/TaintTracking/tst.js +++ b/javascript/ql/test/library-tests/TaintTracking/tst.js @@ -71,5 +71,5 @@ function test() { sink(Map.groupBy(x, z => z)); // NOT OK sink(Custom.groupBy(x, z => z)); // OK - sink(Object.groupBy(x, z => z)); // NOT OK -- This should be marked via taint step, but it is not. + sink(Object.groupBy(x, z => z)); // NOT OK } From 6344f83e4b7740dbe3e406f877064388b4dfdc2a Mon Sep 17 00:00:00 2001 From: Napalys Date: Wed, 20 Nov 2024 13:22:53 +0100 Subject: [PATCH 5/8] JS: Add: tests for taint tracking in groupBy functions --- .../TaintTracking/BasicTaintTracking.expected | 4 ++++ .../test/library-tests/TaintTracking/tst.js | 24 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected index 79be56cbb7e2..ecbab8685136 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected @@ -246,6 +246,10 @@ typeInferenceMismatch | tst.js:2:13:2:20 | source() | tst.js:70:10:70:18 | xReversed | | tst.js:2:13:2:20 | source() | tst.js:72:10:72:31 | Map.gro ... z => z) | | tst.js:2:13:2:20 | source() | tst.js:74:10:74:34 | Object. ... z => z) | +| tst.js:2:13:2:20 | source() | tst.js:79:14:79:20 | grouped | +| tst.js:75:22:75:29 | source() | tst.js:75:10:75:52 | Map.gro ... (item)) | +| tst.js:82:23:82:30 | source() | tst.js:84:14:84:20 | grouped | +| tst.js:87:22:87:29 | source() | tst.js:90:14:90:25 | taintedValue | | xml.js:5:18:5:25 | source() | xml.js:8:14:8:17 | text | | xml.js:12:17:12:24 | source() | xml.js:13:14:13:19 | result | | xml.js:23:18:23:25 | source() | xml.js:20:14:20:17 | attr | diff --git a/javascript/ql/test/library-tests/TaintTracking/tst.js b/javascript/ql/test/library-tests/TaintTracking/tst.js index 0fab561954de..0c642b3a2c05 100644 --- a/javascript/ql/test/library-tests/TaintTracking/tst.js +++ b/javascript/ql/test/library-tests/TaintTracking/tst.js @@ -72,4 +72,28 @@ function test() { sink(Map.groupBy(x, z => z)); // NOT OK sink(Custom.groupBy(x, z => z)); // OK sink(Object.groupBy(x, z => z)); // NOT OK + sink(Map.groupBy(source(), (item) => sink(item))); // NOT OK + + { + const grouped = Map.groupBy(x, (item) => sink(item)); // NOT OK -- Should be tainted, but it is not + sink(grouped); // NOT OK + } + { + const list = [source()]; + const grouped = Map.groupBy(list, (item) => sink(item)); // NOT OK -- Should be tainted, but it is not + sink(grouped); // NOT OK + } + { + const data = source(); + const result = Object.groupBy(data, item => item); + const taintedValue = result[notDefined()]; + sink(taintedValue); // NOT OK + } + { + const data = source(); + const map = Map.groupBy(data, item => item); + const taintedValue = map.get(true); + sink(taintedValue); // NOT OK -- Should be tainted, but it is not + sink(map.get(true)); // NOT OK -- Should be tainted, but it is not + } } From 58faa2d71edf98ce13009a048a417e1032bd390b Mon Sep 17 00:00:00 2001 From: Napalys Date: Wed, 20 Nov 2024 13:34:11 +0100 Subject: [PATCH 6/8] JS: Add: dataflow step for static method of groupBy from Map. --- .../ql/lib/semmle/javascript/Collections.qll | 17 ++++++++++++++++- .../TaintTracking/BasicTaintTracking.expected | 5 +++++ .../TaintTracking/DataFlowTracking.expected | 2 ++ .../ql/test/library-tests/TaintTracking/tst.js | 8 ++++---- 4 files changed, 27 insertions(+), 5 deletions(-) diff --git a/javascript/ql/lib/semmle/javascript/Collections.qll b/javascript/ql/lib/semmle/javascript/Collections.qll index c7348e603f4f..df7ad317014d 100644 --- a/javascript/ql/lib/semmle/javascript/Collections.qll +++ b/javascript/ql/lib/semmle/javascript/Collections.qll @@ -160,7 +160,22 @@ private module CollectionDataFlow { exists(DataFlow::MethodCallNode call | call = DataFlow::globalVarRef(["Map", "Object"]).getAMemberCall("groupBy") and pred = call.getArgument(0) and - succ = call + (succ = call.getCallback(1).getParameter(0) or succ = call.getALocalUse()) + ) + } + } + + /** + * A step for handling data flow and taint tracking for the groupBy method on iterable objects. + * Ensures propagation of taint and data flow through the groupBy operation. + */ + private class GroupByDataFlowStep extends PreCallGraphStep { + override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { + exists(DataFlow::MethodCallNode call | + call = DataFlow::globalVarRef("Map").getAMemberCall("groupBy") and + pred = call.getArgument(0) and + succ = call and + prop = mapValueUnknownKey() ) } } diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected index ecbab8685136..005327d188e5 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected @@ -246,10 +246,15 @@ typeInferenceMismatch | tst.js:2:13:2:20 | source() | tst.js:70:10:70:18 | xReversed | | tst.js:2:13:2:20 | source() | tst.js:72:10:72:31 | Map.gro ... z => z) | | tst.js:2:13:2:20 | source() | tst.js:74:10:74:34 | Object. ... z => z) | +| tst.js:2:13:2:20 | source() | tst.js:78:55:78:58 | item | | tst.js:2:13:2:20 | source() | tst.js:79:14:79:20 | grouped | | tst.js:75:22:75:29 | source() | tst.js:75:10:75:52 | Map.gro ... (item)) | +| tst.js:75:22:75:29 | source() | tst.js:75:47:75:50 | item | +| tst.js:82:23:82:30 | source() | tst.js:83:58:83:61 | item | | tst.js:82:23:82:30 | source() | tst.js:84:14:84:20 | grouped | | tst.js:87:22:87:29 | source() | tst.js:90:14:90:25 | taintedValue | +| tst.js:93:22:93:29 | source() | tst.js:96:14:96:25 | taintedValue | +| tst.js:93:22:93:29 | source() | tst.js:97:14:97:26 | map.get(true) | | xml.js:5:18:5:25 | source() | xml.js:8:14:8:17 | text | | xml.js:12:17:12:24 | source() | xml.js:13:14:13:19 | result | | xml.js:23:18:23:25 | source() | xml.js:20:14:20:17 | attr | diff --git a/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected b/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected index 33a27661ecd1..3b89229b2d7f 100644 --- a/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected @@ -112,3 +112,5 @@ | thisAssignments.js:7:19:7:26 | source() | thisAssignments.js:8:10:8:20 | this.field2 | | tst.js:2:13:2:20 | source() | tst.js:4:10:4:10 | x | | tst.js:2:13:2:20 | source() | tst.js:54:14:54:19 | unsafe | +| tst.js:93:22:93:29 | source() | tst.js:96:14:96:25 | taintedValue | +| tst.js:93:22:93:29 | source() | tst.js:97:14:97:26 | map.get(true) | diff --git a/javascript/ql/test/library-tests/TaintTracking/tst.js b/javascript/ql/test/library-tests/TaintTracking/tst.js index 0c642b3a2c05..a0b596802b76 100644 --- a/javascript/ql/test/library-tests/TaintTracking/tst.js +++ b/javascript/ql/test/library-tests/TaintTracking/tst.js @@ -75,12 +75,12 @@ function test() { sink(Map.groupBy(source(), (item) => sink(item))); // NOT OK { - const grouped = Map.groupBy(x, (item) => sink(item)); // NOT OK -- Should be tainted, but it is not + const grouped = Map.groupBy(x, (item) => sink(item)); // NOT OK sink(grouped); // NOT OK } { const list = [source()]; - const grouped = Map.groupBy(list, (item) => sink(item)); // NOT OK -- Should be tainted, but it is not + const grouped = Map.groupBy(list, (item) => sink(item)); // NOT OK sink(grouped); // NOT OK } { @@ -93,7 +93,7 @@ function test() { const data = source(); const map = Map.groupBy(data, item => item); const taintedValue = map.get(true); - sink(taintedValue); // NOT OK -- Should be tainted, but it is not - sink(map.get(true)); // NOT OK -- Should be tainted, but it is not + sink(taintedValue); // NOT OK + sink(map.get(true)); // NOT OK } } From cdf43f7118e2e535fdc751aa1870e663a6da499d Mon Sep 17 00:00:00 2001 From: Napalys Date: Wed, 20 Nov 2024 14:06:44 +0100 Subject: [PATCH 7/8] Added change notes --- .../ql/lib/change-notes/2024-11-20-ES2024-group-functions.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 javascript/ql/lib/change-notes/2024-11-20-ES2024-group-functions.md diff --git a/javascript/ql/lib/change-notes/2024-11-20-ES2024-group-functions.md b/javascript/ql/lib/change-notes/2024-11-20-ES2024-group-functions.md new file mode 100644 index 000000000000..8511727f8e77 --- /dev/null +++ b/javascript/ql/lib/change-notes/2024-11-20-ES2024-group-functions.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added taint-steps for `Map.groupBy` and `Object.groupBy`. From 7ee0a7b3982e47a7aefdeeb64fd7c24caa43a9d4 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Thu, 21 Nov 2024 14:02:42 +0100 Subject: [PATCH 8/8] Update javascript/ql/lib/semmle/javascript/Collections.qll Co-authored-by: Erik Krogh Kristensen --- javascript/ql/lib/semmle/javascript/Collections.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/Collections.qll b/javascript/ql/lib/semmle/javascript/Collections.qll index df7ad317014d..028c3abe4b3b 100644 --- a/javascript/ql/lib/semmle/javascript/Collections.qll +++ b/javascript/ql/lib/semmle/javascript/Collections.qll @@ -160,7 +160,7 @@ private module CollectionDataFlow { exists(DataFlow::MethodCallNode call | call = DataFlow::globalVarRef(["Map", "Object"]).getAMemberCall("groupBy") and pred = call.getArgument(0) and - (succ = call.getCallback(1).getParameter(0) or succ = call.getALocalUse()) + (succ = call.getCallback(1).getParameter(0) or succ = call) ) } }