From 2e3df74dcef23a342e16d30dc10958452fcafa7f Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 14 Sep 2020 15:58:29 +0200 Subject: [PATCH 1/8] add importScripts as a sink for js/client-side-unvalidated-url-redirection --- .../dataflow/ClientSideUrlRedirectCustomizations.qll | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll index 358de5e8900..64c76a84b82 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll @@ -132,6 +132,15 @@ module ClientSideUrlRedirect { } } + /** + * An argument to `importScripts(..)` - which is used inside `WebWorker`s to import new scripts - viewed as a `ScriptUrlSink`. + */ + class ImportScriptsSink extends ScriptUrlSink { + ImportScriptsSink() { + this = DataFlow::globalVarRef("importScripts").getACall().getAnArgument() + } + } + /** * A script or iframe `src` attribute, viewed as a `ScriptUrlSink`. */ From 6e84ac8e6c15ee508fb95c0e6930d63ba04da51c Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 14 Sep 2020 16:01:54 +0200 Subject: [PATCH 2/8] add test for importScripts --- .../query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js index 72c9c0b6e72..3fe1985d433 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js +++ b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js @@ -43,4 +43,6 @@ function foo() { var el = document.createElement("script"); el.src = payload; document.body.appendChild(el); // NOT OK + + importScripts(payload); // NOT OK } From 283be1920112f66216d0ee51e1e38dc8d9c84d6f Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 14 Sep 2020 16:02:03 +0200 Subject: [PATCH 3/8] add change-note for importScripts --- change-notes/1.26/analysis-javascript.md | 1 + 1 file changed, 1 insertion(+) diff --git a/change-notes/1.26/analysis-javascript.md b/change-notes/1.26/analysis-javascript.md index 06c8b5dc5e5..7d8b7a353d9 100644 --- a/change-notes/1.26/analysis-javascript.md +++ b/change-notes/1.26/analysis-javascript.md @@ -32,6 +32,7 @@ | Unused loop iteration variable (`js/unused-loop-variable`) | Fewer results | This query no longer flags variables in a destructuring array assignment that are not the last variable in the destructed array. | | Unsafe shell command constructed from library input (`js/shell-command-constructed-from-input`) | More results | This query now recognizes more commands where colon, dash, and underscore are used. | | Unsafe jQuery plugin (`js/unsafe-jquery-plugin`) | More results | This query now detects more unsafe uses of nested option properties. | +| Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | More results | This query now recognizes some unsafes uses of `importScripts()` inside WebWorkers. | ## Changes to libraries From cb7de2714aec1ad1326743018b06e27b87e76880 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 14 Sep 2020 16:46:47 +0200 Subject: [PATCH 4/8] add `onmessage` handlers registered using global property as `PostMessageEventHandler` --- .../ql/src/semmle/javascript/security/dataflow/DOM.qll | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/DOM.qll b/javascript/ql/src/semmle/javascript/security/dataflow/DOM.qll index f9882537b4e..57d5612f35e 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/DOM.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/DOM.qll @@ -199,6 +199,11 @@ class PostMessageEventHandler extends Function { addEventListener.getArgument(0).mayHaveStringValue("message") and addEventListener.getArgument(1).getABoundFunctionValue(paramIndex).getFunction() = this ) + or + exists(DataFlow::Node rhs | + rhs = DataFlow::globalObjectRef().getAPropertyWrite("onmessage").getRhs() and + rhs.getABoundFunctionValue(paramIndex).getFunction() = this + ) } /** From f4f96ce04dc647754d3155a9b68963f6421e4763 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 14 Sep 2020 16:46:58 +0200 Subject: [PATCH 5/8] use new source in client-side-url-redirect test --- .../Security/CWE-601/ClientSideUrlRedirect/tst13.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js index 3fe1985d433..532f063beb8 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js +++ b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js @@ -43,6 +43,10 @@ function foo() { var el = document.createElement("script"); el.src = payload; document.body.appendChild(el); // NOT OK - - importScripts(payload); // NOT OK } + +(function () { + self.onmessage = function (e) { + importScripts(e); // NOT OK + } +})(); From 03a3c4f4b2fa1bffe0d2876f790264948c495155 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 14 Sep 2020 16:47:12 +0200 Subject: [PATCH 6/8] update expected output --- .../ClientSideUrlRedirect/ClientSideUrlRedirect.expected | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected index 506dcf1ee0a..a64597858ad 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected +++ b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected @@ -113,6 +113,10 @@ nodes | tst13.js:40:15:40:21 | payload | | tst13.js:44:14:44:20 | payload | | tst13.js:44:14:44:20 | payload | +| tst13.js:49:32:49:32 | e | +| tst13.js:49:32:49:32 | e | +| tst13.js:50:23:50:23 | e | +| tst13.js:50:23:50:23 | e | | tst.js:2:19:2:69 | /.*redi ... n.href) | | tst.js:2:19:2:72 | /.*redi ... ref)[1] | | tst.js:2:19:2:72 | /.*redi ... ref)[1] | @@ -234,6 +238,10 @@ edges | tst13.js:2:19:2:35 | document.location | tst13.js:2:19:2:42 | documen ... .search | | tst13.js:2:19:2:42 | documen ... .search | tst13.js:2:19:2:52 | documen ... bstr(1) | | tst13.js:2:19:2:52 | documen ... bstr(1) | tst13.js:2:9:2:52 | payload | +| tst13.js:49:32:49:32 | e | tst13.js:50:23:50:23 | e | +| tst13.js:49:32:49:32 | e | tst13.js:50:23:50:23 | e | +| tst13.js:49:32:49:32 | e | tst13.js:50:23:50:23 | e | +| tst13.js:49:32:49:32 | e | tst13.js:50:23:50:23 | e | | tst.js:2:19:2:69 | /.*redi ... n.href) | tst.js:2:19:2:72 | /.*redi ... ref)[1] | | tst.js:2:19:2:69 | /.*redi ... n.href) | tst.js:2:19:2:72 | /.*redi ... ref)[1] | | tst.js:2:47:2:63 | document.location | tst.js:2:47:2:68 | documen ... on.href | @@ -276,5 +284,6 @@ edges | tst13.js:36:21:36:27 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:36:21:36:27 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | | tst13.js:40:15:40:21 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:40:15:40:21 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | | tst13.js:44:14:44:20 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:44:14:44:20 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | +| tst13.js:50:23:50:23 | e | tst13.js:49:32:49:32 | e | tst13.js:50:23:50:23 | e | Untrusted URL redirection due to $@. | tst13.js:49:32:49:32 | e | user-provided value | | tst.js:2:19:2:72 | /.*redi ... ref)[1] | tst.js:2:47:2:63 | document.location | tst.js:2:19:2:72 | /.*redi ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:2:47:2:63 | document.location | user-provided value | | tst.js:6:20:6:59 | indirec ... ref)[1] | tst.js:6:34:6:50 | document.location | tst.js:6:20:6:59 | indirec ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:6:34:6:50 | document.location | user-provided value | From cc5109d693ace064d63bf20e0bb4062941a8288f Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 15 Sep 2020 12:14:51 +0200 Subject: [PATCH 7/8] Update change-notes/1.26/analysis-javascript.md Co-authored-by: Esben Sparre Andreasen --- change-notes/1.26/analysis-javascript.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/change-notes/1.26/analysis-javascript.md b/change-notes/1.26/analysis-javascript.md index 7d8b7a353d9..b2ebe4b223b 100644 --- a/change-notes/1.26/analysis-javascript.md +++ b/change-notes/1.26/analysis-javascript.md @@ -32,7 +32,7 @@ | Unused loop iteration variable (`js/unused-loop-variable`) | Fewer results | This query no longer flags variables in a destructuring array assignment that are not the last variable in the destructed array. | | Unsafe shell command constructed from library input (`js/shell-command-constructed-from-input`) | More results | This query now recognizes more commands where colon, dash, and underscore are used. | | Unsafe jQuery plugin (`js/unsafe-jquery-plugin`) | More results | This query now detects more unsafe uses of nested option properties. | -| Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | More results | This query now recognizes some unsafes uses of `importScripts()` inside WebWorkers. | +| Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | More results | This query now recognizes some unsafe uses of `importScripts()` inside WebWorkers. | ## Changes to libraries From fa255f35344bee18aa3460d129deb3fa189cfeb4 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 15 Sep 2020 12:23:48 +0200 Subject: [PATCH 8/8] add test for self.importScripts(..) --- .../ClientSideUrlRedirect/ClientSideUrlRedirect.expected | 9 +++++++++ .../Security/CWE-601/ClientSideUrlRedirect/tst13.js | 3 +++ 2 files changed, 12 insertions(+) diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected index a64597858ad..b7dca317a88 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected +++ b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected @@ -117,6 +117,10 @@ nodes | tst13.js:49:32:49:32 | e | | tst13.js:50:23:50:23 | e | | tst13.js:50:23:50:23 | e | +| tst13.js:52:34:52:34 | e | +| tst13.js:52:34:52:34 | e | +| tst13.js:53:28:53:28 | e | +| tst13.js:53:28:53:28 | e | | tst.js:2:19:2:69 | /.*redi ... n.href) | | tst.js:2:19:2:72 | /.*redi ... ref)[1] | | tst.js:2:19:2:72 | /.*redi ... ref)[1] | @@ -242,6 +246,10 @@ edges | tst13.js:49:32:49:32 | e | tst13.js:50:23:50:23 | e | | tst13.js:49:32:49:32 | e | tst13.js:50:23:50:23 | e | | tst13.js:49:32:49:32 | e | tst13.js:50:23:50:23 | e | +| tst13.js:52:34:52:34 | e | tst13.js:53:28:53:28 | e | +| tst13.js:52:34:52:34 | e | tst13.js:53:28:53:28 | e | +| tst13.js:52:34:52:34 | e | tst13.js:53:28:53:28 | e | +| tst13.js:52:34:52:34 | e | tst13.js:53:28:53:28 | e | | tst.js:2:19:2:69 | /.*redi ... n.href) | tst.js:2:19:2:72 | /.*redi ... ref)[1] | | tst.js:2:19:2:69 | /.*redi ... n.href) | tst.js:2:19:2:72 | /.*redi ... ref)[1] | | tst.js:2:47:2:63 | document.location | tst.js:2:47:2:68 | documen ... on.href | @@ -285,5 +293,6 @@ edges | tst13.js:40:15:40:21 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:40:15:40:21 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | | tst13.js:44:14:44:20 | payload | tst13.js:2:19:2:35 | document.location | tst13.js:44:14:44:20 | payload | Untrusted URL redirection due to $@. | tst13.js:2:19:2:35 | document.location | user-provided value | | tst13.js:50:23:50:23 | e | tst13.js:49:32:49:32 | e | tst13.js:50:23:50:23 | e | Untrusted URL redirection due to $@. | tst13.js:49:32:49:32 | e | user-provided value | +| tst13.js:53:28:53:28 | e | tst13.js:52:34:52:34 | e | tst13.js:53:28:53:28 | e | Untrusted URL redirection due to $@. | tst13.js:52:34:52:34 | e | user-provided value | | tst.js:2:19:2:72 | /.*redi ... ref)[1] | tst.js:2:47:2:63 | document.location | tst.js:2:19:2:72 | /.*redi ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:2:47:2:63 | document.location | user-provided value | | tst.js:6:20:6:59 | indirec ... ref)[1] | tst.js:6:34:6:50 | document.location | tst.js:6:20:6:59 | indirec ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:6:34:6:50 | document.location | user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js index 532f063beb8..7a71cf06c08 100644 --- a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js +++ b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/tst13.js @@ -49,4 +49,7 @@ function foo() { self.onmessage = function (e) { importScripts(e); // NOT OK } + window.onmessage = function (e) { + self.importScripts(e); // NOT OK + } })();