JavaScript: Do even more type tracking in command injection.

Author: Max Schaefer

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

@@ -63,6 +63,26 @@ private DataFlow::SourceNode argumentList(SystemCommandExecution sys) {
   result = argumentList(sys, DataFlow::TypeBackTracker::end())
 }
 
+/**
+ * Gets a data-flow node whose value ends up being interpreted as an element of the argument list
+ * `args` after a flow summarised by `t`.
+ */
+private DataFlow::Node argumentListElement(DataFlow::SourceNode args, DataFlow::TypeBackTracker t) {
+  t.start() and
+  args = argumentList(_) and
+  result = args.getAPropertyWrite().getRhs()
+  or
+  exists(DataFlow::TypeBackTracker t2 | t2 = t.smallstep(result, argumentListElement(args, t2)))
+}
+
+/**
+ * Gets a data-flow node whose value ends up being interpreted as an element of the argument list
+ * `args`.
+ */
+private DataFlow::Node argumentListElement(DataFlow::SourceNode args) {
+  result = argumentListElement(args, DataFlow::TypeBackTracker::end())
+}
+
 /**
  * Holds if `source` contributes to the arguments of an indirect command execution `sys`.
  *
@@ -86,8 +106,8 @@ predicate isIndirectCommandArgument(DataFlow::Node source, SystemCommandExecutio
   exists(DataFlow::ArrayCreationNode args, DataFlow::Node shell, string dashC |
     shellCmd(shell.asExpr(), dashC) and
     shell = commandArgument(sys) and
-    args.getAPropertyWrite().getRhs().mayHaveStringValue(dashC) and
     args = argumentList(sys) and
+    argumentListElement(args).mayHaveStringValue(dashC) and
     (
       source = argumentList(sys)
       or

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

@@ -67,6 +67,18 @@ nodes
 | exec-sh2.js:14:25:14:31 | req.url |
 | exec-sh2.js:14:25:14:31 | req.url |
 | exec-sh2.js:15:12:15:14 | cmd |
+| exec-sh.js:13:17:13:23 | command |
+| exec-sh.js:15:32:15:51 | [shell.arg, command] |
+| exec-sh.js:15:32:15:51 | [shell.arg, command] |
+| exec-sh.js:15:44:15:50 | command |
+| exec-sh.js:15:44:15:50 | command |
+| exec-sh.js:19:9:19:49 | cmd |
+| exec-sh.js:19:15:19:38 | url.par ... , true) |
+| exec-sh.js:19:15:19:44 | url.par ... ).query |
+| exec-sh.js:19:15:19:49 | url.par ... ry.path |
+| exec-sh.js:19:25:19:31 | req.url |
+| exec-sh.js:19:25:19:31 | req.url |
+| exec-sh.js:20:12:20:14 | cmd |
 | execSeries.js:3:20:3:22 | arr |
 | execSeries.js:6:14:6:16 | arr |
 | execSeries.js:6:14:6:21 | arr[i++] |
@@ -197,6 +209,17 @@ edges
 | exec-sh2.js:14:25:14:31 | req.url | exec-sh2.js:14:15:14:38 | url.par ... , true) |
 | exec-sh2.js:14:25:14:31 | req.url | exec-sh2.js:14:15:14:38 | url.par ... , true) |
 | exec-sh2.js:15:12:15:14 | cmd | exec-sh2.js:9:17:9:23 | command |
+| exec-sh.js:13:17:13:23 | command | exec-sh.js:15:44:15:50 | command |
+| exec-sh.js:13:17:13:23 | command | exec-sh.js:15:44:15:50 | command |
+| exec-sh.js:15:44:15:50 | command | exec-sh.js:15:32:15:51 | [shell.arg, command] |
+| exec-sh.js:15:44:15:50 | command | exec-sh.js:15:32:15:51 | [shell.arg, command] |
+| exec-sh.js:19:9:19:49 | cmd | exec-sh.js:20:12:20:14 | cmd |
+| exec-sh.js:19:15:19:38 | url.par ... , true) | exec-sh.js:19:15:19:44 | url.par ... ).query |
+| exec-sh.js:19:15:19:44 | url.par ... ).query | exec-sh.js:19:15:19:49 | url.par ... ry.path |
+| exec-sh.js:19:15:19:49 | url.par ... ry.path | exec-sh.js:19:9:19:49 | cmd |
+| exec-sh.js:19:25:19:31 | req.url | exec-sh.js:19:15:19:38 | url.par ... , true) |
+| exec-sh.js:19:25:19:31 | req.url | exec-sh.js:19:15:19:38 | url.par ... , true) |
+| exec-sh.js:20:12:20:14 | cmd | exec-sh.js:13:17:13:23 | command |
 | 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 |
@@ -285,6 +308,8 @@ edges
 | child_process-test.js:83:19:83:36 | req.query.fileName | child_process-test.js:83:19:83:36 | req.query.fileName | child_process-test.js:83:19:83:36 | req.query.fileName | This command depends on $@. | child_process-test.js:83:19:83:36 | req.query.fileName | a user-provided value |
 | exec-sh2.js:10:12:10:57 | cp.spaw ... ptions) | exec-sh2.js:14:25:14:31 | req.url | exec-sh2.js:10:33:10:47 | ["-c", command] | This command depends on $@. | exec-sh2.js:14:25:14:31 | req.url | a user-provided value |
 | exec-sh2.js:10:12:10:57 | cp.spaw ... ptions) | exec-sh2.js:14:25:14:31 | req.url | exec-sh2.js:10:40:10:46 | command | This command depends on $@. | exec-sh2.js:14:25:14:31 | req.url | a user-provided value |
+| exec-sh.js:15:12:15:61 | cp.spaw ... ptions) | exec-sh.js:19:25:19:31 | req.url | exec-sh.js:15:32:15:51 | [shell.arg, command] | This command depends on $@. | exec-sh.js:19:25:19:31 | req.url | a user-provided value |
+| exec-sh.js:15:12:15:61 | cp.spaw ... ptions) | exec-sh.js:19:25:19:31 | req.url | exec-sh.js:15:44:15:50 | command | This command depends on $@. | exec-sh.js:19:25:19: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 |
 | lib/subLib/index.js:8:10:8:25 | "rm -rf " + name | child_process-test.js:85:37:85:54 | req.query.fileName | lib/subLib/index.js:8:10:8:25 | "rm -rf " + name | This command depends on $@. | child_process-test.js:85:37:85:54 | req.query.fileName | 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 |

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

@@ -1 +0,0 @@
-| query-tests/Security/CWE-078/exec-sh.js:15 | expected an alert, but found none | BAD | ComandInjection |