Skip to content

Commit da566a4

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #2828 from erik-krogh/CVE24
Approved by esbena
2 parents 769dce5 + 897bb4d commit da566a4

File tree

4 files changed

+102
-0
lines changed

4 files changed

+102
-0
lines changed

‎change-notes/1.24/analysis-javascript.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
- [for-in](https://www.npmjs.com/package/for-in)
2626
- [for-own](https://www.npmjs.com/package/for-own)
2727
- [send](https://www.npmjs.com/package/send)
28+
- [chrome-remote-interface](https://www.npmjs.com/package/chrome-remote-interface)
2829

2930
## New queries
3031

‎javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -549,4 +549,72 @@ module ClientRequest {
549549
)
550550
}
551551
}
552+
553+
/**
554+
* Gets a reference to an instance of `chrome-remote-interface`.
555+
*
556+
* An instantiation of `chrome-remote-interface` either accepts a callback or returns a promise.
557+
*
558+
* The `isPromise` parameter reflects whether the reference is a promise containing
559+
* an instance of `chrome-remote-interface`, or an instance of `chrome-remote-interface`.
560+
*/
561+
private DataFlow::SourceNode chromeRemoteInterface(DataFlow::TypeTracker t, boolean isPromise) {
562+
t.start() and
563+
exists(DataFlow::CallNode call |
564+
call = DataFlow::moduleImport("chrome-remote-interface").getAnInvocation()
565+
|
566+
result = call and isPromise = true
567+
or
568+
result = call.getCallback([0 .. 1]).getParameter(0) and isPromise = false
569+
)
570+
or
571+
exists(DataFlow::TypeTracker t2 | result = chromeRemoteInterface(t2, isPromise).track(t2, t))
572+
or
573+
// Simple promise tracking.
574+
exists(DataFlow::TypeTracker t2, DataFlow::SourceNode pred |
575+
pred = chromeRemoteInterface(t2, true) and
576+
isPromise = false and
577+
(
578+
t2 = t and
579+
exists(AwaitExpr await | DataFlow::valueNode(await.getOperand()).getALocalSource() = pred |
580+
result.getEnclosingExpr() = await
581+
)
582+
or
583+
t2 = t and
584+
exists(DataFlow::MethodCallNode thenCall |
585+
thenCall.getMethodName() = "then" and pred = thenCall.getReceiver().getALocalSource()
586+
|
587+
result = thenCall.getCallback(0).getParameter(0)
588+
)
589+
)
590+
)
591+
}
592+
593+
/**
594+
* A call to navigate a browser controlled by `chrome-remote-interface` to a specific URL.
595+
*/
596+
class ChromeRemoteInterfaceRequest extends ClientRequest::Range, DataFlow::CallNode {
597+
int optionsArg;
598+
599+
ChromeRemoteInterfaceRequest() {
600+
exists(DataFlow::SourceNode instance |
601+
instance = chromeRemoteInterface(DataFlow::TypeTracker::end(), false)
602+
|
603+
optionsArg = 0 and
604+
this = instance.getAPropertyRead("Page").getAMemberCall("navigate")
605+
or
606+
optionsArg = 1 and
607+
this = instance.getAMemberCall("send") and
608+
this.getArgument(0).mayHaveStringValue("Page.navigate")
609+
)
610+
}
611+
612+
override DataFlow::Node getUrl() {
613+
result = getArgument(optionsArg).getALocalSource().getAPropertyWrite("url").getRhs()
614+
}
615+
616+
override DataFlow::Node getHost() { none() }
617+
618+
override DataFlow::Node getADataNode() { none() }
619+
}
552620
}

‎javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,16 @@ nodes
3737
| tst.js:45:13:45:56 | 'http:/ ... tainted |
3838
| tst.js:45:13:45:56 | 'http:/ ... tainted |
3939
| tst.js:45:50:45:56 | tainted |
40+
| tst.js:58:9:58:52 | tainted |
41+
| tst.js:58:19:58:42 | url.par ... , true) |
42+
| tst.js:58:19:58:48 | url.par ... ).query |
43+
| tst.js:58:19:58:52 | url.par ... ery.url |
44+
| tst.js:58:29:58:35 | req.url |
45+
| tst.js:58:29:58:35 | req.url |
46+
| tst.js:61:29:61:35 | tainted |
47+
| tst.js:61:29:61:35 | tainted |
48+
| tst.js:64:30:64:36 | tainted |
49+
| tst.js:64:30:64:36 | tainted |
4050
edges
4151
| tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
4252
| tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
@@ -75,6 +85,15 @@ edges
7585
| tst.js:43:46:43:52 | tainted | tst.js:43:13:43:54 | `http:/ ... inted}` |
7686
| tst.js:45:50:45:56 | tainted | tst.js:45:13:45:56 | 'http:/ ... tainted |
7787
| tst.js:45:50:45:56 | tainted | tst.js:45:13:45:56 | 'http:/ ... tainted |
88+
| tst.js:58:9:58:52 | tainted | tst.js:61:29:61:35 | tainted |
89+
| tst.js:58:9:58:52 | tainted | tst.js:61:29:61:35 | tainted |
90+
| tst.js:58:9:58:52 | tainted | tst.js:64:30:64:36 | tainted |
91+
| tst.js:58:9:58:52 | tainted | tst.js:64:30:64:36 | tainted |
92+
| tst.js:58:19:58:42 | url.par ... , true) | tst.js:58:19:58:48 | url.par ... ).query |
93+
| tst.js:58:19:58:48 | url.par ... ).query | tst.js:58:19:58:52 | url.par ... ery.url |
94+
| tst.js:58:19:58:52 | url.par ... ery.url | tst.js:58:9:58:52 | tainted |
95+
| tst.js:58:29:58:35 | req.url | tst.js:58:19:58:42 | url.par ... , true) |
96+
| tst.js:58:29:58:35 | req.url | tst.js:58:19:58:42 | url.par ... , true) |
7897
#select
7998
| 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 |
8099
| 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
88107
| 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 |
89108
| 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 |
90109
| 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 |
110+
| 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 |
111+
| 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 |

‎javascript/ql/test/query-tests/Security/CWE-918/tst.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,3 +52,15 @@ var server = http.createServer(function(req, res) {
5252

5353
request(`${base}${tainted}`); // OK - assumed safe
5454
})
55+
56+
var CDP = require("chrome-remote-interface");
57+
var server = http.createServer(async function(req, res) {
58+
var tainted = url.parse(req.url, true).query.url;
59+
60+
var client = await CDP(options);
61+
client.Page.navigate({url: tainted}); // NOT OK.
62+
63+
CDP(options, (client) => {
64+
client.Page.navigate({url: tainted}); // NOT OK.
65+
});
66+
})

0 commit comments

Comments
 (0)