Skip to content

Commit

Permalink
Merge pull request github#2828 from erik-krogh/CVE24
Browse files Browse the repository at this point in the history 
Approved by esbena
  • Loading branch information
@semmle-qlci
semmle-qlci committed Feb 14, 2020
2 parents 769dce5 + 897bb4d commit da566a4
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 0 deletions.
1 change: 1 addition & 0 deletions change-notes/1.24/analysis-javascript.md
View file
Expand Up @@ -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

Expand Down
68 changes: 68 additions & 0 deletions javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll
View file
Expand Up @@ -549,4 +549,72 @@ module ClientRequest {
)
}
}

/**
* Gets a reference to an 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() }
}
}
21 changes: 21 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
View file
Expand Up @@ -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 |
Expand Down Expand Up @@ -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 |
Expand All @@ -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 |
12 changes: 12 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-918/tst.js
View file
Expand Up @@ -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.
});
})

0 comments on commit da566a4

Please sign in to comment.