From def8d75cb8733b07dcd68d5e856a097e0143d154 Mon Sep 17 00:00:00 2001 From: Napalys Date: Fri, 8 Nov 2024 13:37:34 +0100 Subject: [PATCH 1/6] Added test case for Array.prototype.toSorted, which is currently not flagged as a taint sink. --- javascript/ql/test/library-tests/TaintTracking/tst.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/javascript/ql/test/library-tests/TaintTracking/tst.js b/javascript/ql/test/library-tests/TaintTracking/tst.js index 475e6fdbff0..ed27304c566 100644 --- a/javascript/ql/test/library-tests/TaintTracking/tst.js +++ b/javascript/ql/test/library-tests/TaintTracking/tst.js @@ -59,4 +59,8 @@ function test() { tagged`foo ${"safe"} bar ${x} baz`; sink(x.reverse()); // NOT OK + + sink(x.toSorted()) // NOT OK + const xSorted = x.toSorted(); + sink(xSorted) // NOT OK } From 3f0a54c2e81cf1ff45434b9ec02664f9b8cf8422 Mon Sep 17 00:00:00 2001 From: Napalys Date: Fri, 8 Nov 2024 14:10:55 +0100 Subject: [PATCH 2/6] Added support for Array.prototype.toSorted function --- javascript/ql/lib/semmle/javascript/Arrays.qll | 14 ++++++++++++++ .../TaintTracking/BasicTaintTracking.expected | 2 ++ .../query-tests/NodeJS/DubiousImport/externs.js | 12 ++++++++++++ 3 files changed, 28 insertions(+) diff --git a/javascript/ql/lib/semmle/javascript/Arrays.qll b/javascript/ql/lib/semmle/javascript/Arrays.qll index ebff3d97a98..911af4c17cc 100644 --- a/javascript/ql/lib/semmle/javascript/Arrays.qll +++ b/javascript/ql/lib/semmle/javascript/Arrays.qll @@ -458,4 +458,18 @@ private module ArrayLibraries { ) } } + + /** + * A taint propagating data flow edge arising from array transformation operations + * that return a new array instead of modifying the original array in place. + */ + private class ImmutableArrayTransformStep extends TaintTracking::SharedTaintStep { + override predicate heapStep(DataFlow::Node pred, DataFlow::Node succ) { + exists(DataFlow::MethodCallNode call | + call.getMethodName() = "toSorted" and + pred = call.getReceiver() and + succ = call + ) + } + } } diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected index b1f3e7b26bd..73549543b42 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected @@ -234,6 +234,8 @@ typeInferenceMismatch | tst.js:2:13:2:20 | source() | tst.js:51:10:51:31 | seriali ... ript(x) | | tst.js:2:13:2:20 | source() | tst.js:54:14:54:19 | unsafe | | tst.js:2:13:2:20 | source() | tst.js:61:10:61:20 | x.reverse() | +| tst.js:2:13:2:20 | source() | tst.js:63:10:63:21 | x.toSorted() | +| tst.js:2:13:2:20 | source() | tst.js:65:10:65:16 | xSorted | | 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/query-tests/NodeJS/DubiousImport/externs.js b/javascript/ql/test/query-tests/NodeJS/DubiousImport/externs.js index bd8e36c10e3..b4b40979805 100644 --- a/javascript/ql/test/query-tests/NodeJS/DubiousImport/externs.js +++ b/javascript/ql/test/query-tests/NodeJS/DubiousImport/externs.js @@ -371,6 +371,18 @@ Array.prototype.slice = function(opt_begin, opt_end) {}; */ Array.prototype.sort = function(opt_compareFunction) {}; +/** + * Returns a new array with the elements sorted. + * + * @param {function(T,T):number=} opt_compareFunction Specifies a function that + * defines the sort order. If omitted, the array elements are converted to strings, + * then sorted according to each character's Unicode code point value. + * @this {{length: number}|Array.} + * @template T + * @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toSorted + */ +Array.prototype.toSorted = function(opt_compareFunction) {}; + /** * Changes the content of an array, adding new elements while removing old * elements. From 3215967cbc9e8a585510bef3a88c9eb90f7d9b22 Mon Sep 17 00:00:00 2001 From: Napalys Date: Fri, 8 Nov 2024 15:00:48 +0100 Subject: [PATCH 3/6] Added toReserved test case --- javascript/ql/test/library-tests/TaintTracking/tst.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/javascript/ql/test/library-tests/TaintTracking/tst.js b/javascript/ql/test/library-tests/TaintTracking/tst.js index ed27304c566..3af935687a0 100644 --- a/javascript/ql/test/library-tests/TaintTracking/tst.js +++ b/javascript/ql/test/library-tests/TaintTracking/tst.js @@ -63,4 +63,8 @@ function test() { sink(x.toSorted()) // NOT OK const xSorted = x.toSorted(); sink(xSorted) // NOT OK + + sink(x.toReversed()) // NOT OK + const xReversed = x.toReversed(); + sink(xReversed) // NOT OK } From 7427a24ca1210a453fc2b69c9479db92ce7fd36f Mon Sep 17 00:00:00 2001 From: Napalys Date: Fri, 8 Nov 2024 14:23:10 +0100 Subject: [PATCH 4/6] Added test case for Array.prototype.toReversed, which is currently not flagged as a taint sink. --- javascript/ql/lib/semmle/javascript/Arrays.qll | 2 +- .../TaintTracking/BasicTaintTracking.expected | 2 ++ .../ql/test/query-tests/NodeJS/DubiousImport/externs.js | 9 +++++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/javascript/ql/lib/semmle/javascript/Arrays.qll b/javascript/ql/lib/semmle/javascript/Arrays.qll index 911af4c17cc..d4360072aa2 100644 --- a/javascript/ql/lib/semmle/javascript/Arrays.qll +++ b/javascript/ql/lib/semmle/javascript/Arrays.qll @@ -466,7 +466,7 @@ private module ArrayLibraries { private class ImmutableArrayTransformStep extends TaintTracking::SharedTaintStep { override predicate heapStep(DataFlow::Node pred, DataFlow::Node succ) { exists(DataFlow::MethodCallNode call | - call.getMethodName() = "toSorted" and + call.getMethodName() in ["toSorted", "toReversed"] and pred = call.getReceiver() and succ = call ) diff --git a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected index 73549543b42..ba15575eb9c 100644 --- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected +++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected @@ -236,6 +236,8 @@ typeInferenceMismatch | tst.js:2:13:2:20 | source() | tst.js:61:10:61:20 | x.reverse() | | tst.js:2:13:2:20 | source() | tst.js:63:10:63:21 | x.toSorted() | | tst.js:2:13:2:20 | source() | tst.js:65:10:65:16 | xSorted | +| tst.js:2:13:2:20 | source() | tst.js:67:10:67:23 | x.toReversed() | +| tst.js:2:13:2:20 | source() | tst.js:69:10:69:18 | xReversed | | 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/query-tests/NodeJS/DubiousImport/externs.js b/javascript/ql/test/query-tests/NodeJS/DubiousImport/externs.js index b4b40979805..d82d79c7177 100644 --- a/javascript/ql/test/query-tests/NodeJS/DubiousImport/externs.js +++ b/javascript/ql/test/query-tests/NodeJS/DubiousImport/externs.js @@ -333,6 +333,15 @@ Array.prototype.push = function(var_args) {}; */ Array.prototype.reverse = function() {}; +/** + * Returns a new array with the elements in reversed order. + * + * @this {{length: number}} + * @template T + * @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toReversed + */ +Array.prototype.toReversed = function() {}; + /** * Removes the first element from an array and returns that element. This * method changes the length of the array. From 5f8ff125e93a45e0ad94639da524b89f49d105e8 Mon Sep 17 00:00:00 2001 From: Napalys Date: Tue, 12 Nov 2024 12:21:39 +0100 Subject: [PATCH 5/6] Added change notes --- .../change-notes/2024-11-12-immutable-array-operations.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 javascript/ql/lib/change-notes/2024-11-12-immutable-array-operations.md diff --git a/javascript/ql/lib/change-notes/2024-11-12-immutable-array-operations.md b/javascript/ql/lib/change-notes/2024-11-12-immutable-array-operations.md new file mode 100644 index 00000000000..20c16d88c6e --- /dev/null +++ b/javascript/ql/lib/change-notes/2024-11-12-immutable-array-operations.md @@ -0,0 +1,5 @@ +--- +category: minorAnalysis +--- +* Added taint-steps for `Array.prototype.toReversed`. +* Added taint-steps for `Array.prototype.toSorted`. From ef18a6e56265d7d5d2cf8501caab5cd0c797372a Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Wed, 13 Nov 2024 08:29:18 +0100 Subject: [PATCH 6/6] Remove toReversed and toSorted func prototypes from extern.js. --- .../NodeJS/DubiousImport/externs.js | 21 ------------------- 1 file changed, 21 deletions(-) diff --git a/javascript/ql/test/query-tests/NodeJS/DubiousImport/externs.js b/javascript/ql/test/query-tests/NodeJS/DubiousImport/externs.js index d82d79c7177..bd8e36c10e3 100644 --- a/javascript/ql/test/query-tests/NodeJS/DubiousImport/externs.js +++ b/javascript/ql/test/query-tests/NodeJS/DubiousImport/externs.js @@ -333,15 +333,6 @@ Array.prototype.push = function(var_args) {}; */ Array.prototype.reverse = function() {}; -/** - * Returns a new array with the elements in reversed order. - * - * @this {{length: number}} - * @template T - * @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toReversed - */ -Array.prototype.toReversed = function() {}; - /** * Removes the first element from an array and returns that element. This * method changes the length of the array. @@ -380,18 +371,6 @@ Array.prototype.slice = function(opt_begin, opt_end) {}; */ Array.prototype.sort = function(opt_compareFunction) {}; -/** - * Returns a new array with the elements sorted. - * - * @param {function(T,T):number=} opt_compareFunction Specifies a function that - * defines the sort order. If omitted, the array elements are converted to strings, - * then sorted according to each character's Unicode code point value. - * @this {{length: number}|Array.} - * @template T - * @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toSorted - */ -Array.prototype.toSorted = function(opt_compareFunction) {}; - /** * Changes the content of an array, adding new elements while removing old * elements.