Merge pull request #4337 from max-schaefer/js/fix-indirect-command-injection

Approved by asgerf

Author: codeql-ci

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

@@ -30,7 +30,7 @@ private DataFlow::Node commandArgument(SystemCommandExecution sys, DataFlow::Typ
   t.start() and
   result = sys.getACommandArgument()
   or
-  exists(DataFlow::TypeBackTracker t2 | t = t2.smallstep(result, commandArgument(sys, t2)))
+  exists(DataFlow::TypeBackTracker t2 | t2 = t.smallstep(result, commandArgument(sys, t2)))
 }
 
 /**
@@ -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,12 +106,12 @@ 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
-    (
-      source = argumentList(sys)
-      or
-      source = argumentList(sys).getAPropertyWrite().getRhs()
+    argumentListElement(args).mayHaveStringValue(dashC) and
+    exists(DataFlow::SourceNode argsSource | argsSource = argumentList(sys) |
+      if exists(argsSource.getAPropertyWrite())
+      then source = argsSource.getAPropertyWrite().getRhs()
+      else source = argsSource
     )
   )
 }

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

@@ -23,8 +23,6 @@ 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 |
@@ -36,7 +34,6 @@ nodes
 | child_process-test.js:56:25:56:58 | ['/C',  ... , cmd]) |
 | child_process-test.js:56:25:56:58 | ['/C',  ... , cmd]) |
 | child_process-test.js:56:46:56:57 | ["bar", cmd] |
-| child_process-test.js:56:46:56:57 | ["bar", cmd] |
 | child_process-test.js:56:54:56:56 | cmd |
 | child_process-test.js:56:54:56:56 | cmd |
 | child_process-test.js:57:25:57:49 | ['/C',  ... at(cmd) |
@@ -55,6 +52,26 @@ nodes
 | child_process-test.js:83:19:83:36 | req.query.fileName |
 | child_process-test.js:85:37:85:54 | req.query.fileName |
 | child_process-test.js:85:37:85:54 | req.query.fileName |
+| exec-sh2.js:9:17:9:23 | command |
+| exec-sh2.js:10:40:10:46 | command |
+| exec-sh2.js:10:40:10:46 | command |
+| exec-sh2.js:14:9:14:49 | cmd |
+| exec-sh2.js:14:15:14:38 | url.par ... , true) |
+| exec-sh2.js:14:15:14:44 | url.par ... ).query |
+| exec-sh2.js:14:15:14:49 | url.par ... ry.path |
+| 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: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++] |
@@ -156,12 +173,9 @@ edges
 | 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:56:46:56:57 | ["bar", cmd] | child_process-test.js:56:25:56:58 | ['/C',  ... , cmd]) |
 | child_process-test.js:56:46:56:57 | ["bar", cmd] | child_process-test.js:56:25:56:58 | ['/C',  ... , cmd]) |
 | child_process-test.js:56:54:56:56 | cmd | child_process-test.js:56:46:56:57 | ["bar", cmd] |
-| child_process-test.js:56:54:56:56 | cmd | child_process-test.js:56:46:56:57 | ["bar", cmd] |
 | child_process-test.js:57:46:57:48 | cmd | child_process-test.js:57:25:57:49 | ['/C',  ... at(cmd) |
 | child_process-test.js:57:46:57:48 | cmd | child_process-test.js:57:25:57:49 | ['/C',  ... at(cmd) |
 | child_process-test.js:73:9:73:49 | cmd | child_process-test.js:75:29:75:31 | cmd |
@@ -174,6 +188,24 @@ 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:85:37:85:54 | req.query.fileName | lib/subLib/index.js:7:32:7:35 | name |
 | child_process-test.js:85:37:85:54 | req.query.fileName | lib/subLib/index.js:7:32:7:35 | name |
+| exec-sh2.js:9:17:9:23 | command | exec-sh2.js:10:40:10:46 | command |
+| exec-sh2.js:9:17:9:23 | command | exec-sh2.js:10:40:10:46 | command |
+| exec-sh2.js:14:9:14:49 | cmd | exec-sh2.js:15:12:15:14 | cmd |
+| exec-sh2.js:14:15:14:38 | url.par ... , true) | exec-sh2.js:14:15:14:44 | url.par ... ).query |
+| exec-sh2.js:14:15:14:44 | url.par ... ).query | exec-sh2.js:14:15:14:49 | url.par ... ry.path |
+| exec-sh2.js:14:15:14:49 | url.par ... ry.path | exec-sh2.js:14:9:14:49 | cmd |
+| 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: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 |
@@ -247,19 +279,19 @@ 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:54:5:54:39 | cp.exec ... , args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:53:15:53:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:56:5:56:59 | cp.spaw ...  cmd])) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:56:25:56: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:56:5:56:59 | cp.spaw ...  cmd])) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:56:46:56: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:56:5:56:59 | cp.spaw ...  cmd])) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:56:54:56:56 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:57:5:57:50 | cp.spaw ... t(cmd)) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:6:15:6:49 | url.par ... ry.path | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:57:5:57:50 | cp.spaw ... t(cmd)) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:57:25:57: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:62:5:62:39 | cp.exec ... , args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:53:15:53:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:67:3:67:21 | cp.spawn(cmd, args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:48:15:48:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:75:29:75:31 | cmd | child_process-test.js:73:25:73:31 | req.url | child_process-test.js:75:29:75:31 | cmd | This command depends on $@. | child_process-test.js:73:25:73:31 | req.url | a user-provided value |
 | 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: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: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/UselessUseOfCat.expected

@@ -94,6 +94,8 @@ options
 | child_process-test.js:56:5:56:59 | cp.spaw ...  cmd])) | child_process-test.js:56:25:56:58 | ['/C',  ... , cmd]) |
 | child_process-test.js:57:5:57:50 | cp.spaw ... t(cmd)) | child_process-test.js:57:25:57:49 | ['/C',  ... at(cmd) |
 | child_process-test.js:67:3:67:21 | cp.spawn(cmd, args) | child_process-test.js:67:17:67:20 | args |
+| exec-sh2.js:10:12:10:57 | cp.spaw ... ptions) | exec-sh2.js:10:50:10:56 | options |
+| exec-sh.js:15:12:15:61 | cp.spaw ... ptions) | exec-sh.js:15:54:15:60 | options |
 | lib/lib.js:152:2:152:23 | cp.spaw ... gs, cb) | lib/lib.js:152:21:152:22 | cb |
 | lib/lib.js:159:2:159:23 | cp.spaw ... gs, cb) | lib/lib.js:159:21:159:22 | cb |
 | lib/lib.js:163:2:167:2 | cp.spaw ... t' }\\n\\t) | lib/lib.js:166:3:166:22 | { stdio: 'inherit' } |

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

@@ -0,0 +1,21 @@
+const cp = require('child_process'),
+    http = require('http'),
+    url = require('url');
+
+function getShell() {
+    if (process.platform === 'win32') {
+        return { cmd: 'cmd', arg: '/C' }
+    } else {
+        return { cmd: 'sh', arg: '-c' }
+    }
+}
+
+function execSh(command, options) {
+    var shell = getShell()
+    return cp.spawn(shell.cmd, [shell.arg, command], options) // BAD
+}
+
+http.createServer(function (req, res) {
+    let cmd = url.parse(req.url, true).query.path;
+    execSh(cmd);
+});

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

@@ -0,0 +1,16 @@
+const cp = require('child_process'),
+    http = require('http'),
+    url = require('url');
+
+function getShell() {
+    return "sh";
+}
+
+function execSh(command, options) {
+    return cp.spawn(getShell(), ["-c", command], options) // BAD
+};
+
+http.createServer(function (req, res) {
+    let cmd = url.parse(req.url, true).query.path;
+    execSh(cmd);
+});