expand how indirectCommandArguments are found

Author: erik-krogh

javascript/ql/src/semmle/javascript/security/dataflow/IndirectCommandArgument.qll

@@ -41,7 +41,14 @@ private DataFlow::SourceNode argumentList(SystemCommandExecution sys, DataFlow::
   t.start() and
   result = sys.getArgumentList().getALocalSource()
   or
-  exists(DataFlow::TypeBackTracker t2 | result = argumentList(sys, t2).backtrack(t2, t))
+  exists(DataFlow::TypeBackTracker t2, DataFlow::SourceNode pred | 
+    pred = argumentList(sys, t2) 
+  |
+    result = pred.backtrack(t2, t)
+    or
+    t = t2.continue() and
+    TaintTracking::arrayFunctionTaintStep(result, pred, _)
+  )
 }
 
 /**
@@ -68,18 +75,11 @@ predicate isIndirectCommandArgument(DataFlow::Node source, SystemCommandExecutio
     shellCmd(shell.asExpr(), dashC) and
     shell = commandArgument(sys, DataFlow::TypeBackTracker::end()) and
     args.getAPropertyWrite().getRhs().mayHaveStringValue(dashC) and
+    args = argumentList(sys, DataFlow::TypeBackTracker::end()) and
     (
-      args = argumentList(sys, DataFlow::TypeBackTracker::end()) and
-      source = args.getAPropertyWrite().getRhs()
+      source = argumentList(sys, DataFlow::TypeBackTracker::end())
       or
-      exists(DataFlow::MethodCallNode concatCall |
-        args = concatCall.getReceiver() and
-        concatCall.getMethodName() = "concat" and
-        concatCall = argumentList(sys, DataFlow::TypeBackTracker::end())
-      |
-        source = concatCall.getAnArgument() or
-        source = concatCall.getAnArgument().getALocalSource().getAPropertyWrite().getRhs()
-      )
+      source = argumentList(sys, DataFlow::TypeBackTracker::end()).getAPropertyWrite().getRhs()
     )
   )
 }

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

@@ -22,12 +22,23 @@ nodes
 | child_process-test.js:25:13:25:31 | "foo" + cmd + "bar" |
 | child_process-test.js:25:13:25:31 | "foo" + cmd + "bar" |
 | child_process-test.js:25:21:25:23 | cmd |
+| child_process-test.js:39:18:39:30 | [ flag, cmd ] |
+| child_process-test.js:39:18:39:30 | [ flag, cmd ] |
 | child_process-test.js:39:26:39:28 | cmd |
 | child_process-test.js:39:26:39:28 | cmd |
 | child_process-test.js:43:15:43:17 | cmd |
 | child_process-test.js:43:15:43:17 | cmd |
 | child_process-test.js:50:15:50:17 | cmd |
 | child_process-test.js:50:15:50:17 | cmd |
+| child_process-test.js:53:25:53:58 | ['/C',  ... , cmd]) |
+| child_process-test.js:53:25:53:58 | ['/C',  ... , cmd]) |
+| child_process-test.js:53:46:53:57 | ["bar", cmd] |
+| child_process-test.js:53:46:53:57 | ["bar", cmd] |
+| child_process-test.js:53:54:53:56 | cmd |
+| child_process-test.js:53:54:53:56 | cmd |
+| child_process-test.js:54:25:54:49 | ['/C',  ... at(cmd) |
+| child_process-test.js:54:25:54:49 | ['/C',  ... at(cmd) |
+| child_process-test.js:54:46:54:48 | cmd |
 | execSeries.js:3:20:3:22 | arr |
 | execSeries.js:6:14:6:16 | arr |
 | execSeries.js:6:14:6:21 | arr[i++] |
@@ -100,13 +111,24 @@ edges
 | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:43:15:43:17 | cmd |
 | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:50:15:50:17 | cmd |
 | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:50:15:50:17 | cmd |
+| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:53:54:53:56 | cmd |
+| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:53:54:53:56 | cmd |
+| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:54:46:54:48 | cmd |
 | child_process-test.js:6:15:6:38 | url.par ... , true) | child_process-test.js:6:15:6:44 | url.par ... ).query |
 | child_process-test.js:6:15:6:44 | url.par ... ).query | child_process-test.js:6:15:6:49 | url.par ... ry.path |
 | child_process-test.js:6:15:6:49 | url.par ... ry.path | child_process-test.js:6:9:6:49 | cmd |
 | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:6:15:6:38 | url.par ... , true) |
 | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:6:15:6:38 | url.par ... , true) |
 | child_process-test.js:25:21:25:23 | cmd | child_process-test.js:25:13:25:31 | "foo" + cmd + "bar" |
 | child_process-test.js:25:21:25:23 | cmd | child_process-test.js:25:13:25:31 | "foo" + cmd + "bar" |
+| child_process-test.js:39:26:39:28 | cmd | child_process-test.js:39:18:39:30 | [ flag, cmd ] |
+| child_process-test.js:39:26:39:28 | cmd | child_process-test.js:39:18:39:30 | [ flag, cmd ] |
+| child_process-test.js:53:46:53:57 | ["bar", cmd] | child_process-test.js:53:25:53:58 | ['/C',  ... , cmd]) |
+| child_process-test.js:53:46:53:57 | ["bar", cmd] | child_process-test.js:53:25:53:58 | ['/C',  ... , cmd]) |
+| child_process-test.js:53:54:53:56 | cmd | child_process-test.js:53:46:53:57 | ["bar", cmd] |
+| child_process-test.js:53:54:53:56 | cmd | child_process-test.js:53:46:53:57 | ["bar", cmd] |
+| child_process-test.js:54:46:54:48 | cmd | child_process-test.js:54:25:54:49 | ['/C',  ... at(cmd) |
+| child_process-test.js:54:46:54:48 | cmd | child_process-test.js:54:25:54:49 | ['/C',  ... at(cmd) |
 | execSeries.js:3:20:3:22 | arr | execSeries.js:6:14:6:16 | arr |
 | execSeries.js:6:14:6:16 | arr | execSeries.js:6:14:6:21 | arr[i++] |
 | execSeries.js:6:14:6:21 | arr[i++] | execSeries.js:14:24:14:30 | command |
@@ -165,11 +187,16 @@ edges
 | child_process-test.js:22:18:22:20 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:22:18:22:20 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:23:13:23:15 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:23:13:23:15 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:25:13:25:31 | "foo" + cmd + "bar" | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:25:13:25:31 | "foo" + cmd + "bar" | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
+| child_process-test.js:39:5:39:31 | cp.spaw ...  cmd ]) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:39:18:39:30 | [ flag, cmd ] | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:39:5:39:31 | cp.spaw ...  cmd ]) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:39:26:39:28 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:44:5:44:34 | cp.exec ... , args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:43:15:43:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:51:5:51:39 | cp.exec ... , args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:50:15:50:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
-| child_process-test.js:53:5:53:51 | cp.spaw ... (args)) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:50:15:50:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
-| child_process-test.js:58:3:58:21 | cp.spawn(cmd, args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:43:15:43:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
+| child_process-test.js:53:5:53:59 | cp.spaw ...  cmd])) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:53:25:53:58 | ['/C',  ... , cmd]) | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
+| child_process-test.js:53:5:53:59 | cp.spaw ...  cmd])) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:53:46:53:57 | ["bar", cmd] | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
+| child_process-test.js:53:5:53:59 | cp.spaw ...  cmd])) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:53:54:53:56 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
+| child_process-test.js:54:5:54:50 | cp.spaw ... t(cmd)) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:54:25:54:49 | ['/C',  ... at(cmd) | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
+| child_process-test.js:59:5:59:39 | cp.exec ... , args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:50:15:50:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
+| child_process-test.js:64:3:64:21 | cp.spawn(cmd, args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:43:15:43:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | execSeries.js:14:41:14:47 | command | execSeries.js:18:34:18:40 | req.url | execSeries.js:14:41:14:47 | command | This command depends on $@. | execSeries.js:18:34:18:40 | req.url | a user-provided value |
 | other.js:7:33:7:35 | cmd | other.js:5:25:5:31 | req.url | other.js:7:33:7:35 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
 | other.js:8:28:8:30 | cmd | other.js:5:25:5:31 | req.url | other.js:8:28:8:30 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |

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

@@ -50,7 +50,13 @@ var server = http.createServer(function(req, res) {
     args[1] = cmd;
     cp.execFile(`/bin` + "/bash", args); // NOT OK
 
-    cp.spawn('cmd.exe', ['/C', 'foo'].concat(args)); // NOT OK
+    cp.spawn('cmd.exe', ['/C', 'foo'].concat(["bar", cmd])); // NOT OK
+    cp.spawn('cmd.exe', ['/C', 'foo'].concat(cmd)); // NOT OK
+
+	let myArgs = [];
+    myArgs.push(`-` + "c");
+    myArgs.push(cmd);
+    cp.execFile(`/bin` + "/bash", args); // NOT OK
 
 });