Merge pull request #11082 from erik-krogh/shellArr

JS: treat arrays that gets executed with shell:true as a sink for `js/shell-command-constructed-from-input`

Author: erik-krogh

javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll

@@ -156,21 +156,27 @@ module UnsafeShellCommandConstruction {
   }
 
   /**
-   * Gets a node that ends up in an array that is ultimately executed as a shell script by `sys`.
+   * Holds if the arguments array given to `sys` is joined as a string because `shell` is set to true.
    */
-  private DataFlow::SourceNode endsInShellExecutedArray(
-    DataFlow::TypeBackTracker t, SystemCommandExecution sys
-  ) {
-    t.start() and
-    result = sys.getArgumentList().getALocalSource() and
-    // the array gets joined to a string when `shell` is set to true.
+  predicate executesArrayAsShell(SystemCommandExecution sys) {
     sys.getOptionsArg()
         .getALocalSource()
         .getAPropertyWrite("shell")
         .getRhs()
         .asExpr()
         .(BooleanLiteral)
         .getValue() = "true"
+  }
+
+  /**
+   * Gets a node that ends up in an array that is ultimately executed as a shell script by `sys`.
+   */
+  private DataFlow::SourceNode endsInShellExecutedArray(
+    DataFlow::TypeBackTracker t, SystemCommandExecution sys
+  ) {
+    t.start() and
+    result = sys.getArgumentList().getALocalSource() and
+    executesArrayAsShell(sys)
     or
     exists(DataFlow::TypeBackTracker t2 |
       result = endsInShellExecutedArray(t2, sys).backtrack(t2, t)
@@ -193,6 +199,10 @@ module UnsafeShellCommandConstruction {
         or
         this = arr.getAMethodCall(["push", "unshift"]).getAnArgument()
       )
+      or
+      this = sys.getArgumentList() and
+      not this instanceof DataFlow::ArrayCreationNode and
+      executesArrayAsShell(sys)
     }
 
     override string getSinkType() { result = "shell argument" }

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

@@ -227,8 +227,14 @@ nodes
 | lib/lib.js:420:29:420:32 | name |
 | lib/lib.js:424:24:424:27 | name |
 | lib/lib.js:424:24:424:27 | name |
+| lib/lib.js:425:6:425:13 | arr |
+| lib/lib.js:425:12:425:13 | [] |
 | lib/lib.js:426:11:426:14 | name |
 | lib/lib.js:426:11:426:14 | name |
+| lib/lib.js:427:14:427:16 | arr |
+| lib/lib.js:427:14:427:16 | arr |
+| lib/lib.js:428:14:428:58 | build(" ...  + '-') |
+| lib/lib.js:428:14:428:58 | build(" ...  + '-') |
 | lib/lib.js:428:28:428:51 | (name ? ... ' : '') |
 | lib/lib.js:428:28:428:57 | (name ? ... ) + '-' |
 | lib/lib.js:428:29:428:50 | name ?  ... :' : '' |
@@ -306,6 +312,10 @@ nodes
 | lib/subLib/index.js:7:32:7:35 | name |
 | lib/subLib/index.js:8:22:8:25 | name |
 | lib/subLib/index.js:8:22:8:25 | name |
+| lib/subLib/index.js:13:44:13:46 | arr |
+| lib/subLib/index.js:13:44:13:46 | arr |
+| lib/subLib/index.js:14:22:14:24 | arr |
+| lib/subLib/index.js:14:22:14:24 | arr |
 edges
 | lib/isImported.js:5:49:5:52 | name | lib/isImported.js:6:22:6:25 | name |
 | lib/isImported.js:5:49:5:52 | name | lib/isImported.js:6:22:6:25 | name |
@@ -584,7 +594,13 @@ edges
 | lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name |
 | lib/lib.js:414:40:414:43 | name | lib/lib.js:428:36:428:39 | name |
 | lib/lib.js:414:40:414:43 | name | lib/lib.js:428:36:428:39 | name |
+| lib/lib.js:425:6:425:13 | arr | lib/lib.js:427:14:427:16 | arr |
+| lib/lib.js:425:6:425:13 | arr | lib/lib.js:427:14:427:16 | arr |
+| lib/lib.js:425:12:425:13 | [] | lib/lib.js:425:6:425:13 | arr |
+| lib/lib.js:426:11:426:14 | name | lib/lib.js:425:12:425:13 | [] |
 | lib/lib.js:428:28:428:51 | (name ? ... ' : '') | lib/lib.js:428:28:428:57 | (name ? ... ) + '-' |
+| lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | lib/lib.js:428:14:428:58 | build(" ...  + '-') |
+| lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | lib/lib.js:428:14:428:58 | build(" ...  + '-') |
 | lib/lib.js:428:28:428:57 | (name ? ... ) + '-' | lib/lib.js:431:23:431:26 | last |
 | lib/lib.js:428:29:428:50 | name ?  ... :' : '' | lib/lib.js:428:28:428:51 | (name ? ... ' : '') |
 | lib/lib.js:428:36:428:39 | name | lib/lib.js:428:36:428:45 | name + ':' |
@@ -672,6 +688,10 @@ edges
 | lib/subLib/index.js:7:32:7:35 | name | lib/subLib/index.js:8:22:8:25 | name |
 | lib/subLib/index.js:7:32:7:35 | name | lib/subLib/index.js:8:22:8:25 | name |
 | lib/subLib/index.js:7:32:7:35 | name | lib/subLib/index.js:8:22:8:25 | name |
+| lib/subLib/index.js:13:44:13:46 | arr | lib/subLib/index.js:14:22:14:24 | arr |
+| lib/subLib/index.js:13:44:13:46 | arr | lib/subLib/index.js:14:22:14:24 | arr |
+| lib/subLib/index.js:13:44:13:46 | arr | lib/subLib/index.js:14:22:14:24 | arr |
+| lib/subLib/index.js:13:44:13:46 | arr | lib/subLib/index.js:14:22:14:24 | arr |
 #select
 | lib/isImported.js:6:10:6:25 | "rm -rf " + name | lib/isImported.js:5:49:5:52 | name | lib/isImported.js:6:22:6:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/isImported.js:5:49:5:52 | name | library input | lib/isImported.js:6:2:6:26 | cp.exec ... + name) | shell command |
 | lib/lib2.js:4:10:4:25 | "rm -rf " + name | lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib2.js:3:28:3:31 | name | library input | lib/lib2.js:4:2:4:26 | cp.exec ... + name) | shell command |
@@ -739,6 +759,8 @@ edges
 | lib/lib.js:420:29:420:32 | name | lib/lib.js:414:40:414:43 | name | lib/lib.js:420:29:420:32 | name | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:414:40:414:43 | name | library input | lib/lib.js:420:2:420:49 | cp.spaw ...  true}) | shell command |
 | lib/lib.js:424:24:424:27 | name | lib/lib.js:414:40:414:43 | name | lib/lib.js:424:24:424:27 | name | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:414:40:414:43 | name | library input | lib/lib.js:424:2:424:40 | spawn(" ... WN_OPT) | shell command |
 | lib/lib.js:426:11:426:14 | name | lib/lib.js:414:40:414:43 | name | lib/lib.js:426:11:426:14 | name | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:414:40:414:43 | name | library input | lib/lib.js:427:2:427:28 | spawn(" ... WN_OPT) | shell command |
+| lib/lib.js:427:14:427:16 | arr | lib/lib.js:414:40:414:43 | name | lib/lib.js:427:14:427:16 | arr | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:414:40:414:43 | name | library input | lib/lib.js:427:2:427:28 | spawn(" ... WN_OPT) | shell command |
+| lib/lib.js:428:14:428:58 | build(" ...  + '-') | lib/lib.js:414:40:414:43 | name | lib/lib.js:428:14:428:58 | build(" ...  + '-') | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:414:40:414:43 | name | library input | lib/lib.js:428:2:428:70 | spawn(" ... WN_OPT) | shell command |
 | lib/lib.js:436:19:436:22 | last | lib/lib.js:414:40:414:43 | name | lib/lib.js:436:19:436:22 | last | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:414:40:414:43 | name | library input | lib/lib.js:428:2:428:70 | spawn(" ... WN_OPT) | shell command |
 | lib/lib.js:442:12:442:27 | "rm -rf " + name | lib/lib.js:441:39:441:42 | name | lib/lib.js:442:24:442:27 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:441:39:441:42 | name | library input | lib/lib.js:442:2:442:28 | asyncEx ... + name) | shell command |
 | lib/lib.js:447:13:447:28 | "rm -rf " + name | lib/lib.js:446:20:446:23 | name | lib/lib.js:447:25:447:28 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:446:20:446:23 | name | library input | lib/lib.js:447:3:447:29 | asyncEx ... + name) | shell command |
@@ -760,3 +782,4 @@ edges
 | lib/subLib/amdSub.js:4:10:4:25 | "rm -rf " + name | lib/subLib/amdSub.js:3:28:3:31 | name | lib/subLib/amdSub.js:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib/amdSub.js:3:28:3:31 | name | library input | lib/subLib/amdSub.js:4:2:4:26 | cp.exec ... + name) | shell command |
 | lib/subLib/index.js:4:10:4:25 | "rm -rf " + name | lib/subLib/index.js:3:28:3:31 | name | lib/subLib/index.js:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib/index.js:3:28:3:31 | name | library input | lib/subLib/index.js:4:2:4:26 | cp.exec ... + name) | shell command |
 | lib/subLib/index.js:8:10:8:25 | "rm -rf " + name | lib/subLib/index.js:7:32:7:35 | name | lib/subLib/index.js:8:22:8:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib/index.js:7:32:7:35 | name | library input | lib/subLib/index.js:8:2:8:26 | cp.exec ... + name) | shell command |
+| lib/subLib/index.js:14:22:14:24 | arr | lib/subLib/index.js:13:44:13:46 | arr | lib/subLib/index.js:14:22:14:24 | arr | This shell argument which depends on $@ is later used in a $@. | lib/subLib/index.js:13:44:13:46 | arr | library input | lib/subLib/index.js:14:5:14:40 | cp.spaw ...  true}) | shell command |

javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/subLib/index.js

@@ -8,4 +8,8 @@ module.exports.foo = function (name) {
 	cp.exec("rm -rf " + name); // NOT OK - this is being called explicitly from child_process-test.js
 };
 
-module.exports.amd = require("./amd.js");
+module.exports.amd = require("./amd.js");
+
+module.exports.arrToShell = function (cmd, arr) {
+    cp.spawn("echo", arr, {shell: true}); // NOT OK
+}