From d6afd438ba0271782e4a8edc5e336e72a3149f6c Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Wed, 12 Feb 2020 16:14:18 +0100 Subject: [PATCH 1/3] add model for chrome-remote-interface as a ClientRequest --- change-notes/1.24/analysis-javascript.md | 1 + .../javascript/frameworks/ClientRequests.qll | 68 +++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/change-notes/1.24/analysis-javascript.md b/change-notes/1.24/analysis-javascript.md index be33d77204f1..706b186ac58b 100644 --- a/change-notes/1.24/analysis-javascript.md +++ b/change-notes/1.24/analysis-javascript.md @@ -25,6 +25,7 @@ - [for-in](https://www.npmjs.com/package/for-in) - [for-own](https://www.npmjs.com/package/for-own) - [send](https://www.npmjs.com/package/send) + - [chrome-remote-interface](https://www.npmjs.com/package/chrome-remote-interface) ## New queries diff --git a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll index cf729947ee2e..5fbf789742b8 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll @@ -549,4 +549,72 @@ module ClientRequest { ) } } + + /** + * Gets a reference to a instance of `chrome-remote-interface`. + * + * An instantiation of `chrome-remote-interface` either accepts a callback or returns a promise. + * + * The `isPromise` parameter reflects whether the reference is a promise containing + * an instance of `chrome-remote-interface`, or an instance of `chrome-remote-interface`. + */ + private DataFlow::SourceNode chromeRemoteInterface(DataFlow::TypeTracker t, boolean isPromise) { + t.start() and + exists(DataFlow::CallNode call | + call = DataFlow::moduleImport("chrome-remote-interface").getAnInvocation() + | + result = call and isPromise = true + or + result = call.getCallback([0 .. 1]).getParameter(0) and isPromise = false + ) + or + exists(DataFlow::TypeTracker t2 | result = chromeRemoteInterface(t2, isPromise).track(t2, t)) + or + // Simple promise tracking. + exists(DataFlow::TypeTracker t2, DataFlow::SourceNode pred | + pred = chromeRemoteInterface(t2, true) and + isPromise = false and + ( + t2 = t and + exists(AwaitExpr await | DataFlow::valueNode(await.getOperand()).getALocalSource() = pred | + result.getEnclosingExpr() = await + ) + or + t2 = t and + exists(DataFlow::MethodCallNode thenCall | + thenCall.getMethodName() = "then" and pred = thenCall.getReceiver().getALocalSource() + | + result = thenCall.getCallback(0).getParameter(0) + ) + ) + ) + } + + /** + * A call to navigate a browser controlled by `chrome-remote-interface` to a specific URL. + */ + class ChromeRemoteInterfaceRequest extends ClientRequest::Range, DataFlow::CallNode { + int optionsArg; + + ChromeRemoteInterfaceRequest() { + exists(DataFlow::SourceNode instance | + instance = chromeRemoteInterface(DataFlow::TypeTracker::end(), false) + | + optionsArg = 0 and + this = instance.getAPropertyRead("Page").getAMemberCall("navigate") + or + optionsArg = 1 and + this = instance.getAMemberCall("send") and + this.getArgument(0).mayHaveStringValue("Page.navigate") + ) + } + + override DataFlow::Node getUrl() { + result = getArgument(optionsArg).getALocalSource().getAPropertyWrite("url").getRhs() + } + + override DataFlow::Node getHost() { none() } + + override DataFlow::Node getADataNode() { none() } + } } From 1ab5ca4e649402ef81ad3b23bb88916a77b495ed Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 13 Feb 2020 14:15:28 +0100 Subject: [PATCH 2/3] typo in docstring Co-Authored-By: Esben Sparre Andreasen --- .../ql/src/semmle/javascript/frameworks/ClientRequests.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll index 5fbf789742b8..d39b26f2563a 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll @@ -551,7 +551,7 @@ module ClientRequest { } /** - * Gets a reference to a instance of `chrome-remote-interface`. + * Gets a reference to an instance of `chrome-remote-interface`. * * An instantiation of `chrome-remote-interface` either accepts a callback or returns a promise. * From 897bb4d80107a71ed221e4f964f86da8ca3959fb Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Thu, 13 Feb 2020 15:12:29 +0100 Subject: [PATCH 3/3] add test for chrome-remote-interface --- .../Security/CWE-918/RequestForgery.expected | 21 +++++++++++++++++++ .../test/query-tests/Security/CWE-918/tst.js | 12 +++++++++++ 2 files changed, 33 insertions(+) diff --git a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected index 5aeef6447cc0..80d9d41e1962 100644 --- a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected +++ b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected @@ -37,6 +37,16 @@ nodes | tst.js:45:13:45:56 | 'http:/ ... tainted | | tst.js:45:13:45:56 | 'http:/ ... tainted | | tst.js:45:50:45:56 | tainted | +| tst.js:58:9:58:52 | tainted | +| tst.js:58:19:58:42 | url.par ... , true) | +| tst.js:58:19:58:48 | url.par ... ).query | +| tst.js:58:19:58:52 | url.par ... ery.url | +| tst.js:58:29:58:35 | req.url | +| tst.js:58:29:58:35 | req.url | +| tst.js:61:29:61:35 | tainted | +| tst.js:61:29:61:35 | tainted | +| tst.js:64:30:64:36 | tainted | +| tst.js:64:30:64:36 | tainted | edges | tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted | | tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted | @@ -75,6 +85,15 @@ edges | tst.js:43:46:43:52 | tainted | tst.js:43:13:43:54 | `http:/ ... inted}` | | tst.js:45:50:45:56 | tainted | tst.js:45:13:45:56 | 'http:/ ... tainted | | tst.js:45:50:45:56 | tainted | tst.js:45:13:45:56 | 'http:/ ... tainted | +| tst.js:58:9:58:52 | tainted | tst.js:61:29:61:35 | tainted | +| tst.js:58:9:58:52 | tainted | tst.js:61:29:61:35 | tainted | +| tst.js:58:9:58:52 | tainted | tst.js:64:30:64:36 | tainted | +| tst.js:58:9:58:52 | tainted | tst.js:64:30:64:36 | tainted | +| tst.js:58:19:58:42 | url.par ... , true) | tst.js:58:19:58:48 | url.par ... ).query | +| tst.js:58:19:58:48 | url.par ... ).query | tst.js:58:19:58:52 | url.par ... ery.url | +| tst.js:58:19:58:52 | url.par ... ery.url | tst.js:58:9:58:52 | tainted | +| tst.js:58:29:58:35 | req.url | tst.js:58:19:58:42 | url.par ... , true) | +| tst.js:58:29:58:35 | req.url | tst.js:58:19:58:42 | url.par ... , true) | #select | tst.js:18:5:18:20 | request(tainted) | tst.js:14:29:14:35 | req.url | tst.js:18:13:18:19 | tainted | The $@ of this request depends on $@. | tst.js:18:13:18:19 | tainted | URL | tst.js:14:29:14:35 | req.url | a user-provided value | | tst.js:20:5:20:24 | request.get(tainted) | tst.js:14:29:14:35 | req.url | tst.js:20:17:20:23 | tainted | The $@ of this request depends on $@. | tst.js:20:17:20:23 | tainted | URL | tst.js:14:29:14:35 | req.url | a user-provided value | @@ -88,3 +107,5 @@ edges | tst.js:41:5:41:52 | request ... nted}`) | tst.js:14:29:14:35 | req.url | tst.js:41:13:41:51 | `http:/ ... inted}` | The $@ of this request depends on $@. | tst.js:41:13:41:51 | `http:/ ... inted}` | URL | tst.js:14:29:14:35 | req.url | a user-provided value | | tst.js:43:5:43:55 | request ... nted}`) | tst.js:14:29:14:35 | req.url | tst.js:43:13:43:54 | `http:/ ... inted}` | The $@ of this request depends on $@. | tst.js:43:13:43:54 | `http:/ ... inted}` | URL | tst.js:14:29:14:35 | req.url | a user-provided value | | tst.js:45:5:45:57 | request ... ainted) | tst.js:14:29:14:35 | req.url | tst.js:45:13:45:56 | 'http:/ ... tainted | The $@ of this request depends on $@. | tst.js:45:13:45:56 | 'http:/ ... tainted | URL | tst.js:14:29:14:35 | req.url | a user-provided value | +| tst.js:61:2:61:37 | client. ... inted}) | tst.js:58:29:58:35 | req.url | tst.js:61:29:61:35 | tainted | The $@ of this request depends on $@. | tst.js:61:29:61:35 | tainted | URL | tst.js:58:29:58:35 | req.url | a user-provided value | +| tst.js:64:3:64:38 | client. ... inted}) | tst.js:58:29:58:35 | req.url | tst.js:64:30:64:36 | tainted | The $@ of this request depends on $@. | tst.js:64:30:64:36 | tainted | URL | tst.js:58:29:58:35 | req.url | a user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-918/tst.js b/javascript/ql/test/query-tests/Security/CWE-918/tst.js index 848400aeca0a..b663d5366326 100644 --- a/javascript/ql/test/query-tests/Security/CWE-918/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-918/tst.js @@ -52,3 +52,15 @@ var server = http.createServer(function(req, res) { request(`${base}${tainted}`); // OK - assumed safe }) + +var CDP = require("chrome-remote-interface"); +var server = http.createServer(async function(req, res) { + var tainted = url.parse(req.url, true).query.url; + + var client = await CDP(options); + client.Page.navigate({url: tainted}); // NOT OK. + + CDP(options, (client) => { + client.Page.navigate({url: tainted}); // NOT OK. + }); +})