Merge pull request github#3498 from max-schaefer/js/remote-exec

Approved by esbena

Author: semmle-qlci

change-notes/1.25/analysis-javascript.md

@@ -9,6 +9,8 @@
   - [jQuery](https://jquery.com/)
   - [marsdb](https://www.npmjs.com/package/marsdb)
   - [minimongo](https://www.npmjs.com/package/minimongo/)
+  - [ssh2](https://www.npmjs.com/package/ssh2)
+  - [ssh2-streams](https://www.npmjs.com/package/ssh2-streams)
 
 ## New queries
 

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

@@ -51,13 +51,9 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I
         )
         or
         shell = true and
-        (
-          mod = "exec" and
-          optionsArg = -2 and
-          cmdArg = 0
-          or
-          mod = "remote-exec" and cmdArg = 1 and optionsArg = -1
-        )
+        mod = "exec" and
+        optionsArg = -2 and
+        cmdArg = 0
       ) and
       callee = DataFlow::moduleImport(mod)
     |
@@ -97,3 +93,33 @@ private boolean getSync(string name) {
   then result = true
   else result = false
 }
+
+private class RemoteCommandExecutor extends SystemCommandExecution, DataFlow::InvokeNode {
+  int cmdArg;
+
+  RemoteCommandExecutor() {
+    this = DataFlow::moduleImport("remote-exec").getACall() and
+    cmdArg = 1
+    or
+    exists(DataFlow::SourceNode ssh2, DataFlow::SourceNode client |
+      ssh2 = DataFlow::moduleImport("ssh2") and
+      (client = ssh2 or client = ssh2.getAPropertyRead("Client")) and
+      this = client.getAnInstantiation().getAMethodCall("exec") and
+      cmdArg = 0
+    )
+    or
+    exists(DataFlow::SourceNode ssh2stream |
+      ssh2stream = DataFlow::moduleMember("ssh2-streams", "SSH2Stream") and
+      this = ssh2stream.getAnInstantiation().getAMethodCall("exec") and
+      cmdArg = 1
+    )
+  }
+
+  override DataFlow::Node getACommandArgument() { result = getArgument(cmdArg) }
+
+  override predicate isShellInterpreted(DataFlow::Node arg) { arg = getACommandArgument() }
+
+  override predicate isSync() { none() }
+
+  override DataFlow::Node getOptionsArg() { none() }
+}

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

@@ -93,6 +93,12 @@ nodes
 | other.js:18:22:18:24 | cmd |
 | other.js:19:36:19:38 | cmd |
 | other.js:19:36:19:38 | cmd |
+| other.js:22:21:22:23 | cmd |
+| other.js:22:21:22:23 | cmd |
+| other.js:23:28:23:30 | cmd |
+| other.js:23:28:23:30 | cmd |
+| other.js:26:34:26:36 | cmd |
+| other.js:26:34:26:36 | 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 |
@@ -184,6 +190,12 @@ edges
 | other.js:5:9:5:49 | cmd | other.js:18:22:18:24 | cmd |
 | other.js:5:9:5:49 | cmd | other.js:19:36:19:38 | cmd |
 | other.js:5:9:5:49 | cmd | other.js:19:36:19:38 | cmd |
+| other.js:5:9:5:49 | cmd | other.js:22:21:22:23 | cmd |
+| other.js:5:9:5:49 | cmd | other.js:22:21:22:23 | cmd |
+| other.js:5:9:5:49 | cmd | other.js:23:28:23:30 | cmd |
+| 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: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 |
@@ -226,4 +238,7 @@ edges
 | other.js:17:27:17:29 | cmd | other.js:5:25:5:31 | req.url | other.js:17:27:17:29 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
 | other.js:18:22:18:24 | cmd | other.js:5:25:5:31 | req.url | other.js:18:22:18:24 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
 | other.js:19:36:19:38 | cmd | other.js:5:25:5:31 | req.url | other.js:19:36:19:38 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
+| 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 |
 | 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

@@ -17,4 +17,11 @@ var server = http.createServer(function(req, res) {
     require("exec-async")(cmd); // NOT OK
     require("execa")(cmd); // NOT OK
     require("remote-exec")(target, cmd); // NOT OK
+
+    const ssh2 = require("ssh2");
+    new ssh2().exec(cmd); // NOT OK
+    new ssh2.Client().exec(cmd); // NOT OK
+
+    const SSH2Stream = require("ssh2-streams").SSH2Stream;
+    new SSH2Stream().exec(false, cmd); // NOT OK
 });