JS: Report command injection through child_process argument arrays when shell is enabled

javascript/ql/lib/change-notes/2026-11-02-shell-command-arguments.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The command injection queries now properly recognize argument arrays passed to `spawn` and `execFile` as sinks when the `shell` option is enabled.

javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll

@@ -797,13 +797,32 @@ module NodeJSLib {
     override DataFlow::Node getArgumentList() {
       methodName = ["execFile", "execFileSync", "fork", "spawn", "spawnSync"] and
       // all of the above methods take the argument list as their second argument
-      result = this.getParameter(1).asSink()
+      result = this.getParameter(1).asSink() and
+      not this.isOptionsObject(result) and
+      not result.getALocalSource() instanceof DataFlow::FunctionNode
     }
 
     override predicate isSync() { methodName.matches("%Sync") }
 
+    /** Holds if `node` is known to be an options object rather than an argument array or callback. */
+    pragma[inline]
+    private predicate isOptionsObject(DataFlow::Node node) {
+      not node.getALocalSource() instanceof DataFlow::ArrayCreationNode and
+      (
+        node.getALocalSource() instanceof DataFlow::ObjectLiteralNode
+        or
+        exists(node.getALocalSource().getAPropertyWrite("shell"))
+        or
+        exists(API::Node api |
+          api.asSink() = node and
+          api.getAValueReachingSink().getALocalSource() instanceof DataFlow::ObjectLiteralNode
+        )
+      )
+    }
+
     override DataFlow::Node getOptionsArg() {
       not result.getALocalSource() instanceof DataFlow::FunctionNode and // looks like callback
+      // Shell execution still takes separate arguments for the array and the options object.
       not result.getALocalSource() instanceof DataFlow::ArrayCreationNode and // looks like argumentlist
       not result = this.getParameter(0).asSink() and
       // fork/spawn and all sync methos always has options as the last argument
@@ -812,9 +831,14 @@ module NodeJSLib {
         methodName.matches("spawn%") or
         methodName.matches("%Sync")
       then result = this.getLastArgument()
-      else
-        // the rest (exec/execFile) has the options argument as their second last.
-        result = this.getParameter(this.getNumArgument() - 2).asSink()
+      else (
+        result = this.getParameter(this.getNumArgument() - 2).asSink() and
+        (methodName = "exec" or this.getNumArgument() = 4 or this.isOptionsObject(result))
+        or
+        // Without a callback, require evidence that the last argument is an options object.
+        result = this.getLastArgument() and
+        this.isOptionsObject(result)
+      )
     }
   }
 

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

@@ -5,6 +5,7 @@
  */
 
 import javascript
+private import UnsafeShellCommandConstructionCustomizations
 
 module CommandInjection {
   /**
@@ -55,6 +56,19 @@ module CommandInjection {
     SystemCommandExecutionSink() { this = any(SystemCommandExecution sys).getACommandArgument() }
   }
 
+  /** An argument in a `child_process` argument array that is interpreted by a shell. */
+  private class ChildProcessArgumentSink extends Sink {
+    ChildProcessArgumentSink() {
+      exists(UnsafeShellCommandConstruction::ShellTrueCommandExecutionSink sink |
+        this = sink and
+        sink.getCommandExecution() =
+          API::moduleImport(["mz/child_process", "child_process"])
+              .getMember(["execFile", "execFileSync", "spawn", "spawnSync"])
+              .getMaybePromisifiedCall()
+      )
+    }
+  }
+
   private class SinkFromModel extends Sink {
     SinkFromModel() { ModelOutput::sinkNode(this, "command-injection") }
   }

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

@@ -174,58 +174,65 @@ module UnsafeShellCommandConstruction {
     override DataFlow::Node getAlertLocation() { result = this }
   }
 
-  /**
-   * Holds if the arguments array given to `sys` is joined as a string because `shell` is set to true.
-   */
-  predicate executesArrayAsShell(SystemCommandExecution sys) {
-    sys.getOptionsArg()
-        .getALocalSource()
-        .getAPropertyWrite("shell")
-        .getRhs()
-        .asExpr()
-        .(BooleanLiteral)
-        .getValue() = "true"
+  /** Holds if `node` can reach `use` without another write to the `shell` property of `options`. */
+  private predicate reachesShellOptionUse(
+    DataFlow::SourceNode options, ControlFlowNode node, ControlFlowNode use
+  ) {
+    exists(SystemCommandExecution sys |
+      options = sys.getOptionsArg().getALocalSource() and use = sys.asExpr()
+    ) and
+    (
+      node = use
+      or
+      not node = options.getAPropertyWrite("shell").getWriteNode() and
+      reachesShellOptionUse(options, node.getASuccessor(), use)
+    )
+  }
+
+  /** Gets a value of the `shell` option that can reach the command execution. */
+  private DataFlow::Node getShellOption(SystemCommandExecution sys) {
+    exists(DataFlow::SourceNode options, DataFlow::PropWrite write, Expr use |
+      options = sys.getOptionsArg().getALocalSource() and
+      write = options.getAPropertyWrite("shell") and
+      use = sys.asExpr() and
+      result = write.getRhs()
+    |
+      write.getWriteNode().getContainer() != use.getContainer()
+      or
+      reachesShellOptionUse(options, write.getWriteNode().getASuccessor(), use)
+    )
     or
-    exists(API::Node node |
+    exists(API::Node node, DataFlow::SourceNode options, Expr use |
       node.asSink() = sys.getOptionsArg() and
-      node.getMember("shell").asSink().mayHaveBooleanValue(true)
+      result = node.getMember("shell").asSink() and
+      options = sys.getOptionsArg().getALocalSource() and
+      use = sys.asExpr() and
+      not result = options.getAPropertyWrite("shell").getRhs() and
+      reachesShellOptionUse(options, use.getContainer().getStart(), use)
     )
   }
 
   /**
-   * 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 the `shell` option is enabled.
    */
-  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)
+  predicate executesArrayAsShell(SystemCommandExecution sys) {
+    exists(DataFlow::Node shell | shell = getShellOption(sys) |
+      shell.mayHaveBooleanValue(true)
+      or
+      exists(string path | shell.mayHaveStringValue(path) and path != "")
     )
   }
 
   /**
-   * An argument to a command invocation where the `shell` option is set to true.
+   * An argument to a command invocation where the `shell` option is enabled.
    */
   class ShellTrueCommandExecutionSink extends Sink {
     SystemCommandExecution sys;
 
     ShellTrueCommandExecutionSink() {
-      // `shell` is set to true. That means string-concatenation happens behind the scenes.
+      // `shell` is enabled. That means string-concatenation happens behind the scenes.
       // We just assume that a `shell` option in any library means the same thing as it does in NodeJS.
-      exists(DataFlow::SourceNode arr |
-        arr = endsInShellExecutedArray(DataFlow::TypeBackTracker::end(), sys)
-      |
-        this = arr.(DataFlow::ArrayCreationNode).getAnElement()
-        or
-        this = arr.getAMethodCall(["push", "unshift"]).getAnArgument()
-      )
-      or
       this = sys.getArgumentList() and
-      not this instanceof DataFlow::ArrayCreationNode and
       executesArrayAsShell(sys)
     }
 

javascript/ql/src/change-notes/2026-11-02-shell-command-arguments.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `js/command-line-injection` query now reports remote input that reaches argument arrays when the `shell` option is enabled. The `js/shell-command-constructed-from-input` query also recognizes string-valued shell paths. Both queries now handle `execFile` calls without a callback and check argument-array input at the execution call, avoiding alerts for later array mutations.

javascript/ql/test/library-tests/frameworks/NodeJSLib/SystemCommandExecution.qll

@@ -3,3 +3,9 @@ import javascript
 query predicate test_SystemCommandExecution(SystemCommandExecution cmd, DataFlow::Node res) {
   res = cmd.getACommandArgument()
 }
+
+query predicate test_SystemCommandExecution_getOptionsArg(
+  SystemCommandExecution cmd, DataFlow::Node res
+) {
+  res = cmd.getOptionsArg()
+}

javascript/ql/test/library-tests/frameworks/NodeJSLib/exec.js

@@ -5,3 +5,17 @@ cp.execFileSync("sh", ["-c", "node --version"]);
 cp.fork("foo", ["arg"]);
 cp.spawn("echo", ["Hi"], cb);
 cp.spawnSync("echo", ["Hi", "there"]);
+
+cp.spawn("echo", { shell: true });
+cp.spawn("echo", ["Hi"], { shell: true });
+cp.execFile("echo", { shell: "/bin/sh" });
+cp.execFile("echo", ["Hi"], { shell: "/bin/sh" });
+
+function overloads(args, callback) {
+    cp.exec("echo", callback);
+    cp.execFile("echo", args, callback);
+    cp.spawn("echo", args, { shell: true });
+    cp.execFile("echo", args, { shell: "/bin/sh" });
+    cp.execFile("echo", args, { shell: true }, callback);
+    cp.execFile("echo", { shell: true }, callback);
+}

javascript/ql/test/library-tests/frameworks/NodeJSLib/tests.expected

@@ -230,6 +230,16 @@ test_SystemCommandExecution
 | exec.js:5:1:5:23 | cp.fork ... "arg"]) | exec.js:5:9:5:13 | "foo" |
 | exec.js:6:1:6:28 | cp.spaw ... "], cb) | exec.js:6:10:6:15 | "echo" |
 | exec.js:7:1:7:37 | cp.spaw ... here"]) | exec.js:7:14:7:19 | "echo" |
+| exec.js:9:1:9:33 | cp.spaw ... true }) | exec.js:9:10:9:15 | "echo" |
+| exec.js:10:1:10:41 | cp.spaw ... true }) | exec.js:10:10:10:15 | "echo" |
+| exec.js:11:1:11:41 | cp.exec ... /sh" }) | exec.js:11:13:11:18 | "echo" |
+| exec.js:12:1:12:49 | cp.exec ... /sh" }) | exec.js:12:13:12:18 | "echo" |
+| exec.js:15:5:15:29 | cp.exec ... llback) | exec.js:15:13:15:18 | "echo" |
+| exec.js:16:5:16:39 | cp.exec ... llback) | exec.js:16:17:16:22 | "echo" |
+| exec.js:17:5:17:43 | cp.spaw ... true }) | exec.js:17:14:17:19 | "echo" |
+| exec.js:18:5:18:51 | cp.exec ... /sh" }) | exec.js:18:17:18:22 | "echo" |
+| exec.js:19:5:19:56 | cp.exec ... llback) | exec.js:19:17:19:22 | "echo" |
+| exec.js:20:5:20:50 | cp.exec ... llback) | exec.js:20:17:20:22 | "echo" |
 test_HeaderDefinition_defines
 | src/http.js:13:3:13:44 | res.set ... /html') | content-type | text/html |
 | src/https.js:13:3:13:44 | res.set ... /html') | content-type | text/html |
@@ -417,3 +427,19 @@ test_SystemCommandExecution_getAnArgumentForCommand
 | exec.js:5:1:5:23 | cp.fork ... "arg"]) | exec.js:5:16:5:22 | ["arg"] |
 | exec.js:6:1:6:28 | cp.spaw ... "], cb) | exec.js:6:18:6:23 | ["Hi"] |
 | exec.js:7:1:7:37 | cp.spaw ... here"]) | exec.js:7:22:7:36 | ["Hi", "there"] |
+| exec.js:10:1:10:41 | cp.spaw ... true }) | exec.js:10:18:10:23 | ["Hi"] |
+| exec.js:12:1:12:49 | cp.exec ... /sh" }) | exec.js:12:21:12:26 | ["Hi"] |
+| exec.js:16:5:16:39 | cp.exec ... llback) | exec.js:16:25:16:28 | args |
+| exec.js:17:5:17:43 | cp.spaw ... true }) | exec.js:17:22:17:25 | args |
+| exec.js:18:5:18:51 | cp.exec ... /sh" }) | exec.js:18:25:18:28 | args |
+| exec.js:19:5:19:56 | cp.exec ... llback) | exec.js:19:25:19:28 | args |
+test_SystemCommandExecution_getOptionsArg
+| exec.js:6:1:6:28 | cp.spaw ... "], cb) | exec.js:6:26:6:27 | cb |
+| exec.js:9:1:9:33 | cp.spaw ... true }) | exec.js:9:18:9:32 | { shell: true } |
+| exec.js:10:1:10:41 | cp.spaw ... true }) | exec.js:10:26:10:40 | { shell: true } |
+| exec.js:11:1:11:41 | cp.exec ... /sh" }) | exec.js:11:21:11:40 | { shell: "/bin/sh" } |
+| exec.js:12:1:12:49 | cp.exec ... /sh" }) | exec.js:12:29:12:48 | { shell: "/bin/sh" } |
+| exec.js:17:5:17:43 | cp.spaw ... true }) | exec.js:17:28:17:42 | { shell: true } |
+| exec.js:18:5:18:51 | cp.exec ... /sh" }) | exec.js:18:31:18:50 | { shell: "/bin/sh" } |
+| exec.js:19:5:19:56 | cp.exec ... llback) | exec.js:19:31:19:45 | { shell: true } |
+| exec.js:20:5:20:50 | cp.exec ... llback) | exec.js:20:25:20:39 | { shell: true } |