From c75d785684d0d25424d0501fb6726b291a20408a Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Tue, 30 Oct 2018 10:59:25 -0400 Subject: [PATCH] JavaScript: Fix modelling of `_.partial`. Like `Function.prototype.bind` (but unlike `ramda.partial`) it takes the curried arguments as rest arguments, not as an array; cf. https://lodash.com/docs/4.17.10#partial and https://underscorejs.org/#partial. --- change-notes/1.19/analysis-javascript.md | 2 +- .../javascript/dataflow/Configuration.qll | 21 ++++++++++--- .../InterProceduralFlow/DataFlow.expected | 4 +++ .../InterProceduralFlow/GermanFlow.expected | 4 +++ .../TaintTracking.expected | 4 +++ .../InterProceduralFlow/partial.js | 30 +++++++++++++++++++ .../Security/CWE-079/ReflectedXss.expected | 4 +-- .../CWE-079/ReflectedXssPath.expected | 8 ++--- .../ReflectedXssWithCustomSanitizer.expected | 4 +-- ...flectedXssWithCustomSanitizer_old.expected | 4 +-- .../query-tests/Security/CWE-079/partial.js | 4 +-- 11 files changed, 72 insertions(+), 17 deletions(-) create mode 100644 javascript/ql/test/library-tests/InterProceduralFlow/partial.js diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index 635771eea8dd..152fcbdcd87f 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -9,7 +9,7 @@ * Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following features: - file system access, for example through [fs-extra](https://github.com/jprichardson/node-fs-extra) or [globby](https://www.npmjs.com/package/globby) - outbound network access, for example through the [fetch API](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API) - - the [Google Cloud Spanner](https://cloud.google.com/spanner) database + - the [Google Cloud Spanner](https://cloud.google.com/spanner), [lodash](https://lodash.com) and [underscore](https://underscorejs.org/) libraries * The type inference now handles nested imports (that is, imports not appearing at the toplevel). This may yield fewer false-positive results on projects that use this non-standard language feature. diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll index e2c5f325b0d5..625fc8b769bc 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll @@ -418,11 +418,24 @@ private class BindPartialCall extends AdditionalPartialInvokeNode, DataFlow::Met } /** - * A partial call through `_.partial` or a function with a similar interface. + * A partial call through `_.partial`. */ -private class LibraryPartialCall extends AdditionalPartialInvokeNode { - LibraryPartialCall() { - this = LodashUnderscore::member("partial").getACall() or +private class LodashPartialCall extends AdditionalPartialInvokeNode { + LodashPartialCall() { + this = LodashUnderscore::member("partial").getACall() + } + + override predicate isPartialArgument(DataFlow::Node callback, DataFlow::Node argument, int index) { + callback = getArgument(0) and + argument = getArgument(index+1) + } +} + +/** + * A partial call through `ramda.partial`. + */ +private class RamdaPartialCall extends AdditionalPartialInvokeNode { + RamdaPartialCall() { this = DataFlow::moduleMember("ramda", "partial").getACall() } diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected b/javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected index 0bd663e2038a..257378e06657 100644 --- a/javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected +++ b/javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected @@ -16,6 +16,10 @@ | nodeJsLib.js:2:15:2:23 | "tainted" | esClient.js:7:13:7:18 | nj.foo | | nodeJsLib.js:2:15:2:23 | "tainted" | esClient.js:10:13:10:17 | njFoo | | nodeJsLib.js:2:15:2:23 | "tainted" | nodeJsClient.js:4:13:4:18 | nj.foo | +| partial.js:5:15:5:24 | "tainted1" | partial.js:9:15:9:15 | x | +| partial.js:5:15:5:24 | "tainted1" | partial.js:15:15:15:15 | x | +| partial.js:5:15:5:24 | "tainted1" | partial.js:21:15:21:15 | x | +| partial.js:5:15:5:24 | "tainted1" | partial.js:27:15:27:15 | x | | promises.js:2:16:2:24 | "tainted" | promises.js:7:16:7:18 | val | | promises.js:11:22:11:31 | "resolved" | promises.js:19:20:19:20 | v | | promises.js:11:22:11:31 | "resolved" | promises.js:27:16:27:16 | v | diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected b/javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected index 21ec35e6dd1b..32828bae083b 100644 --- a/javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected +++ b/javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected @@ -17,6 +17,10 @@ | nodeJsLib.js:2:15:2:23 | "tainted" | esClient.js:7:13:7:18 | nj.foo | | nodeJsLib.js:2:15:2:23 | "tainted" | esClient.js:10:13:10:17 | njFoo | | nodeJsLib.js:2:15:2:23 | "tainted" | nodeJsClient.js:4:13:4:18 | nj.foo | +| partial.js:5:15:5:24 | "tainted1" | partial.js:9:15:9:15 | x | +| partial.js:5:15:5:24 | "tainted1" | partial.js:15:15:15:15 | x | +| partial.js:5:15:5:24 | "tainted1" | partial.js:21:15:21:15 | x | +| partial.js:5:15:5:24 | "tainted1" | partial.js:27:15:27:15 | x | | promises.js:2:16:2:24 | "tainted" | promises.js:7:16:7:18 | val | | promises.js:11:22:11:31 | "resolved" | promises.js:19:20:19:20 | v | | promises.js:11:22:11:31 | "resolved" | promises.js:27:16:27:16 | v | diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected b/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected index 9901c666086c..6dfda67a76b8 100644 --- a/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected +++ b/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected @@ -19,6 +19,10 @@ | nodeJsLib.js:2:15:2:23 | "tainted" | esClient.js:7:13:7:18 | nj.foo | | nodeJsLib.js:2:15:2:23 | "tainted" | esClient.js:10:13:10:17 | njFoo | | nodeJsLib.js:2:15:2:23 | "tainted" | nodeJsClient.js:4:13:4:18 | nj.foo | +| partial.js:5:15:5:24 | "tainted1" | partial.js:9:15:9:15 | x | +| partial.js:5:15:5:24 | "tainted1" | partial.js:15:15:15:15 | x | +| partial.js:5:15:5:24 | "tainted1" | partial.js:21:15:21:15 | x | +| partial.js:5:15:5:24 | "tainted1" | partial.js:27:15:27:15 | x | | promises.js:2:16:2:24 | "tainted" | promises.js:7:16:7:18 | val | | promises.js:2:16:2:24 | "tainted" | promises.js:38:32:38:32 | v | | promises.js:11:22:11:31 | "resolved" | promises.js:19:20:19:20 | v | diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/partial.js b/javascript/ql/test/library-tests/InterProceduralFlow/partial.js new file mode 100644 index 000000000000..bd88efc7410c --- /dev/null +++ b/javascript/ql/test/library-tests/InterProceduralFlow/partial.js @@ -0,0 +1,30 @@ +let underscore = require('underscore'); +let lodash = require('lodash'); +let R = require('ramda'); + +let source1 = "tainted1"; +let source2 = "tainted2"; + +function f1(x, y) { + let sink1 = x; + let sink2 = y; +} +f1.bind(null, source1)(source2); + +function f2(x, y) { + let sink1 = x; + let sink2 = y; +} +underscore.partial(f2, source1)(source2); + +function f3(x, y) { + let sink1 = x; + let sink2 = y; +} +lodash.partial(f3, source1)(source2); + +function f4(x, y) { + let sink1 = x; + let sink2 = y; +} +R.partial(f4, [source1])(source2); diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected index 51c73fd82833..be0c937c38f3 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected @@ -3,8 +3,8 @@ | formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value | | formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value | | partial.js:10:14:10:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:13:42:13:48 | req.url | user-provided value | -| partial.js:19:14:19:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:22:52:22:58 | req.url | user-provided value | -| partial.js:28:14:28:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:31:48:31:54 | req.url | user-provided value | +| partial.js:19:14:19:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:22:51:22:57 | req.url | user-provided value | +| partial.js:28:14:28:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:31:47:31:53 | req.url | user-provided value | | partial.js:37:14:37:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:40:43:40:49 | req.url | user-provided value | | promises.js:6:25:6:25 | x | Cross-site scripting vulnerability due to $@. | promises.js:5:44:5:57 | req.query.data | user-provided value | | tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssPath.expected b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssPath.expected index 0106b1ca4c8f..28682d23433a 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssPath.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssPath.expected @@ -16,10 +16,10 @@ edges | partial.js:13:42:13:48 | req.url | partial.js:9:25:9:25 | x | | partial.js:18:25:18:25 | x | partial.js:19:14:19:14 | x | | partial.js:19:14:19:14 | x | partial.js:19:14:19:18 | x + y | -| partial.js:22:52:22:58 | req.url | partial.js:18:25:18:25 | x | +| partial.js:22:51:22:57 | req.url | partial.js:18:25:18:25 | x | | partial.js:27:25:27:25 | x | partial.js:28:14:28:14 | x | | partial.js:28:14:28:14 | x | partial.js:28:14:28:18 | x + y | -| partial.js:31:48:31:54 | req.url | partial.js:27:25:27:25 | x | +| partial.js:31:47:31:53 | req.url | partial.js:27:25:27:25 | x | | partial.js:36:25:36:25 | x | partial.js:37:14:37:14 | x | | partial.js:37:14:37:14 | x | partial.js:37:14:37:18 | x + y | | partial.js:40:43:40:49 | req.url | partial.js:36:25:36:25 | x | @@ -38,8 +38,8 @@ edges | formatting.js:6:14:6:47 | util.fo ... , evil) | formatting.js:4:16:4:29 | req.query.evil | formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value | | formatting.js:7:14:7:53 | require ... , evil) | formatting.js:4:16:4:29 | req.query.evil | formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value | | partial.js:10:14:10:18 | x + y | partial.js:13:42:13:48 | req.url | partial.js:10:14:10:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:13:42:13:48 | req.url | user-provided value | -| partial.js:19:14:19:18 | x + y | partial.js:22:52:22:58 | req.url | partial.js:19:14:19:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:22:52:22:58 | req.url | user-provided value | -| partial.js:28:14:28:18 | x + y | partial.js:31:48:31:54 | req.url | partial.js:28:14:28:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:31:48:31:54 | req.url | user-provided value | +| partial.js:19:14:19:18 | x + y | partial.js:22:51:22:57 | req.url | partial.js:19:14:19:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:22:51:22:57 | req.url | user-provided value | +| partial.js:28:14:28:18 | x + y | partial.js:31:47:31:53 | req.url | partial.js:28:14:28:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:31:47:31:53 | req.url | user-provided value | | partial.js:37:14:37:18 | x + y | partial.js:40:43:40:49 | req.url | partial.js:37:14:37:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:40:43:40:49 | req.url | user-provided value | | promises.js:6:25:6:25 | x | promises.js:5:44:5:57 | req.query.data | promises.js:6:25:6:25 | x | Cross-site scripting vulnerability due to $@. | promises.js:5:44:5:57 | req.query.data | user-provided value | | promises.js:6:25:6:25 | x | promises.js:5:44:5:57 | req.query.data | promises.js:6:25:6:25 | x | Cross-site scripting vulnerability due to $@. | promises.js:5:44:5:57 | req.query.data | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer.expected b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer.expected index c6cacfa5b01c..ef68af087bed 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer.expected @@ -2,8 +2,8 @@ | formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value | | formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value | | partial.js:10:14:10:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:13:42:13:48 | req.url | user-provided value | -| partial.js:19:14:19:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:22:52:22:58 | req.url | user-provided value | -| partial.js:28:14:28:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:31:48:31:54 | req.url | user-provided value | +| partial.js:19:14:19:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:22:51:22:57 | req.url | user-provided value | +| partial.js:28:14:28:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:31:47:31:53 | req.url | user-provided value | | partial.js:37:14:37:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:40:43:40:49 | req.url | user-provided value | | promises.js:6:25:6:25 | x | Cross-site scripting vulnerability due to $@. | promises.js:5:44:5:57 | req.query.data | user-provided value | | tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer_old.expected b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer_old.expected index 0c3fc03a2cd5..bfbe2040225b 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer_old.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXssWithCustomSanitizer_old.expected @@ -6,8 +6,8 @@ WARNING: Predicate flowsFrom has been deprecated and may be removed in future (R | formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value | | formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value | | partial.js:10:14:10:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:13:42:13:48 | req.url | user-provided value | -| partial.js:19:14:19:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:22:52:22:58 | req.url | user-provided value | -| partial.js:28:14:28:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:31:48:31:54 | req.url | user-provided value | +| partial.js:19:14:19:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:22:51:22:57 | req.url | user-provided value | +| partial.js:28:14:28:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:31:47:31:53 | req.url | user-provided value | | partial.js:37:14:37:18 | x + y | Cross-site scripting vulnerability due to $@. | partial.js:40:43:40:49 | req.url | user-provided value | | promises.js:6:25:6:25 | x | Cross-site scripting vulnerability due to $@. | promises.js:5:44:5:57 | req.query.data | user-provided value | | tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/partial.js b/javascript/ql/test/query-tests/Security/CWE-079/partial.js index f2ec06235aa5..17a36fa67121 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/partial.js +++ b/javascript/ql/test/query-tests/Security/CWE-079/partial.js @@ -19,7 +19,7 @@ app.get("/underscore", (req, res) => { res.send(x + y); // NOT OK } - let callback = underscore.partial(sendResponse, [req.url]); + let callback = underscore.partial(sendResponse, req.url); [1, 2, 3].forEach(callback); }); @@ -28,7 +28,7 @@ app.get("/lodash", (req, res) => { res.send(x + y); // NOT OK } - let callback = lodash.partial(sendResponse, [req.url]); + let callback = lodash.partial(sendResponse, req.url); [1, 2, 3].forEach(callback); });