Skip to content

Commit 91762ec

Browse files
author
Max Schaefer
committed
JavaScript: Add partial model for opener.
3.5M weekly downloads. Note that we do not treat the first argument as a command-injection sink. While it is possible to inject commands that way, it is more likely to cause false positives where the user input is concatenated with some prefix that makes the opening heuristic decide to treat it as a URL.
1 parent 9aa26fa commit 91762ec

File tree

4 files changed

+23
-1
lines changed

4 files changed

+23
-1
lines changed

change-notes/1.25/analysis-javascript.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
- [mssql](https://www.npmjs.com/package/mssql)
2020
- [mysql](https://www.npmjs.com/package/mysql)
2121
- [npmlog](https://www.npmjs.com/package/npmlog)
22+
- [opener](https://www.npmjs.com/package/opener)
2223
- [pg](https://www.npmjs.com/package/pg)
2324
- [sequelize](https://www.npmjs.com/package/sequelize)
2425
- [spanner](https://www.npmjs.com/package/spanner)

javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,3 +135,15 @@ private class RemoteCommandExecutor extends SystemCommandExecution, DataFlow::In
135135

136136
override DataFlow::Node getOptionsArg() { none() }
137137
}
138+
139+
private class Opener extends SystemCommandExecution, DataFlow::InvokeNode {
140+
Opener() { this = DataFlow::moduleImport("opener").getACall() }
141+
142+
override DataFlow::Node getACommandArgument() { result = getOptionArgument(1, "command") }
143+
144+
override predicate isShellInterpreted(DataFlow::Node arg) { none() }
145+
146+
override predicate isSync() { none() }
147+
148+
override DataFlow::Node getOptionsArg() { none() }
149+
}

javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,8 @@ nodes
113113
| other.js:28:27:28:29 | cmd |
114114
| other.js:30:33:30:35 | cmd |
115115
| other.js:30:33:30:35 | cmd |
116+
| other.js:34:44:34:46 | cmd |
117+
| other.js:34:44:34:46 | cmd |
116118
| third-party-command-injection.js:5:20:5:26 | command |
117119
| third-party-command-injection.js:5:20:5:26 | command |
118120
| third-party-command-injection.js:6:21:6:27 | command |
@@ -221,6 +223,8 @@ edges
221223
| other.js:5:9:5:49 | cmd | other.js:28:27:28:29 | cmd |
222224
| other.js:5:9:5:49 | cmd | other.js:30:33:30:35 | cmd |
223225
| other.js:5:9:5:49 | cmd | other.js:30:33:30:35 | cmd |
226+
| other.js:5:9:5:49 | cmd | other.js:34:44:34:46 | cmd |
227+
| other.js:5:9:5:49 | cmd | other.js:34:44:34:46 | cmd |
224228
| other.js:5:15:5:38 | url.par ... , true) | other.js:5:15:5:44 | url.par ... ).query |
225229
| other.js:5:15:5:44 | url.par ... ).query | other.js:5:15:5:49 | url.par ... ry.path |
226230
| other.js:5:15:5:49 | url.par ... ry.path | other.js:5:9:5:49 | cmd |
@@ -271,4 +275,5 @@ edges
271275
| other.js:26:34:26:36 | cmd | other.js:5:25:5:31 | req.url | other.js:26:34:26:36 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
272276
| other.js:28:27:28:29 | cmd | other.js:5:25:5:31 | req.url | other.js:28:27:28:29 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
273277
| other.js:30:33:30:35 | cmd | other.js:5:25:5:31 | req.url | other.js:30:33:30:35 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
278+
| other.js:34:44:34:46 | cmd | other.js:5:25:5:31 | req.url | other.js:34:44:34:46 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
274279
| third-party-command-injection.js:6:21:6:27 | command | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | This command depends on $@. | third-party-command-injection.js:5:20:5:26 | command | a server-provided value |

javascript/ql/test/query-tests/Security/CWE-078/other.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
var http = require("http"),
22
url = require("url");
33

4-
var server = http.createServer(function(req, res) {
4+
var server = http.createServer(function (req, res) {
55
let cmd = url.parse(req.url, true).query.path;
66

77
require("cross-spawn").sync(cmd); // NOT OK
@@ -28,4 +28,8 @@ var server = http.createServer(function(req, res) {
2828
require("execa").node(cmd); // NOT OK
2929

3030
require("foreground-child")(cmd); // NOT OK
31+
32+
const opener = require("opener");
33+
opener("http://github.com/" + url.parse(req.url, true).query.user); // OK
34+
opener("http://github.com", { command: cmd }); // NOT OK
3135
});

0 commit comments

Comments
 (0)