Merge pull request #3979 from max-schaefer/js/more-comand-injection-models

Approved by asgerf

change-notes/1.25/analysis-javascript.md

@@ -6,8 +6,10 @@
   - [Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)
   - [bluebird](http://bluebirdjs.com/)
   - [express](https://www.npmjs.com/package/express)
+  - [execa](https://www.npmjs.com/package/execa)
   - [fancy-log](https://www.npmjs.com/package/fancy-log)
   - [fastify](https://www.npmjs.com/package/fastify)
+  - [foreground-child](https://www.npmjs.com/package/foreground-child)
   - [fstream](https://www.npmjs.com/package/fstream)
   - [jGrowl](https://github.com/stanlemon/jGrowl)
   - [jQuery](https://jquery.com/)
@@ -17,6 +19,7 @@
   - [mssql](https://www.npmjs.com/package/mssql)
   - [mysql](https://www.npmjs.com/package/mysql)
   - [npmlog](https://www.npmjs.com/package/npmlog)
+  - [opener](https://www.npmjs.com/package/opener)
   - [pg](https://www.npmjs.com/package/pg)
   - [sequelize](https://www.npmjs.com/package/sequelize)
   - [spanner](https://www.npmjs.com/package/spanner)

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

@@ -32,6 +32,12 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I
           (method = "command" or method = "commandSync")
         ) and
         cmdArg = 0
+        or
+        mod = "execa" and
+        method = "node" and
+        cmdArg = 0 and
+        optionsArg = 1 and
+        shell = false
       |
         callee = DataFlow::moduleMember(mod, method) and
         sync = getSync(method)
@@ -59,6 +65,12 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I
     |
       this = callee.getACall()
     )
+    or
+    this = DataFlow::moduleImport("foreground-child").getACall() and
+    cmdArg = 0 and
+    optionsArg = 1 and
+    shell = false and
+    sync = true
   }
 
   override DataFlow::Node getACommandArgument() { result = getArgument(cmdArg) }
@@ -123,3 +135,15 @@ private class RemoteCommandExecutor extends SystemCommandExecution, DataFlow::In
 
   override DataFlow::Node getOptionsArg() { none() }
 }
+
+private class Opener extends SystemCommandExecution, DataFlow::InvokeNode {
+  Opener() { this = DataFlow::moduleImport("opener").getACall() }
+
+  override DataFlow::Node getACommandArgument() { result = getOptionArgument(1, "command") }
+
+  override predicate isShellInterpreted(DataFlow::Node arg) { none() }
+
+  override predicate isSync() { none() }
+
+  override DataFlow::Node getOptionsArg() { none() }
+}

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

@@ -109,6 +109,12 @@ nodes
 | other.js:23:28:23:30 | cmd |
 | other.js:26:34:26:36 | cmd |
 | other.js:26:34:26:36 | cmd |
+| other.js:28:27:28:29 | cmd |
+| other.js:28:27:28:29 | cmd |
+| other.js:30:33:30:35 | cmd |
+| other.js:30:33:30:35 | cmd |
+| other.js:34:44:34:46 | cmd |
+| other.js:34:44:34:46 | cmd |
 | third-party-command-injection.js:5:20:5:26 | command |
 | third-party-command-injection.js:5:20:5:26 | command |
 | third-party-command-injection.js:6:21:6:27 | command |
@@ -213,6 +219,12 @@ edges
 | other.js:5:9:5:49 | cmd | other.js:23:28:23:30 | cmd |
 | other.js:5:9:5:49 | cmd | other.js:26:34:26:36 | cmd |
 | other.js:5:9:5:49 | cmd | other.js:26:34:26:36 | cmd |
+| other.js:5:9:5:49 | cmd | other.js:28:27:28:29 | cmd |
+| other.js:5:9:5:49 | cmd | other.js:28:27:28:29 | cmd |
+| other.js:5:9:5:49 | cmd | other.js:30:33:30:35 | cmd |
+| other.js:5:9:5:49 | cmd | other.js:30:33:30:35 | cmd |
+| other.js:5:9:5:49 | cmd | other.js:34:44:34:46 | cmd |
+| other.js:5:9:5:49 | cmd | other.js:34:44:34:46 | cmd |
 | other.js:5:15:5:38 | url.par ... , true) | other.js:5:15:5:44 | url.par ... ).query |
 | other.js:5:15:5:44 | url.par ... ).query | other.js:5:15:5:49 | url.par ... ry.path |
 | other.js:5:15:5:49 | url.par ... ry.path | other.js:5:9:5:49 | cmd |
@@ -261,4 +273,7 @@ edges
 | other.js:22:21:22:23 | cmd | other.js:5:25:5:31 | req.url | other.js:22:21:22:23 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
 | other.js:23:28:23:30 | cmd | other.js:5:25:5:31 | req.url | other.js:23:28:23:30 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
 | 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 |
+| 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 |
+| 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 |
+| 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 |
 | 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/UselessUseOfCat.expected

@@ -56,6 +56,7 @@ syncCommand
 | other.js:7:5:7:36 | require ... nc(cmd) |
 | other.js:9:5:9:35 | require ... nc(cmd) |
 | other.js:12:5:12:30 | require ... nc(cmd) |
+| other.js:30:5:30:36 | require ... ")(cmd) |
 | third-party-command-injection.js:6:9:6:28 | cp.execSync(command) |
 | tst_shell-command-injection-from-environment.js:4:2:4:62 | cp.exec ... emp")]) |
 | tst_shell-command-injection-from-environment.js:5:2:5:54 | cp.exec ... temp")) |

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

@@ -1,7 +1,7 @@
 var http = require("http"),
     url = require("url");
 
-var server = http.createServer(function(req, res) {
+var server = http.createServer(function (req, res) {
     let cmd = url.parse(req.url, true).query.path;
 
     require("cross-spawn").sync(cmd); // NOT OK
@@ -24,4 +24,12 @@ var server = http.createServer(function(req, res) {
 
     const SSH2Stream = require("ssh2-streams").SSH2Stream;
     new SSH2Stream().exec(false, cmd); // NOT OK
+
+    require("execa").node(cmd); // NOT OK
+
+    require("foreground-child")(cmd); // NOT OK
+
+    const opener = require("opener");
+    opener("http://github.com/" + url.parse(req.url, true).query.user); // OK
+    opener("http://github.com", { command: cmd }); // NOT OK
 });