Merge pull request #11013 from erik-krogh/sndCmd

JS: second-order-command-injection

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

@@ -0,0 +1,216 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * second order command injection, as well as
+ * extension points for adding your own.
+ */
+
+import javascript
+private import semmle.javascript.PackageExports as Exports
+private import semmle.javascript.security.TaintedObjectCustomizations
+
+/** Classes and predicates for reasoning about second order command injection. */
+module SecondOrderCommandInjection {
+  /** A shell command that allows for second order command injection. */
+  private class VulnerableCommand extends string {
+    VulnerableCommand() { this = ["git", "hg"] }
+
+    /**
+     * Gets a vulnerable subcommand of this command.
+     * E.g. `git` has `clone` and `pull` as vulnerable subcommands.
+     * And every command of `hg` is vulnerable due to `--config=alias.<alias>=<command>`.
+     */
+    bindingset[result]
+    string getAVulnerableSubCommand() {
+      this = "git" and result = ["clone", "ls-remote", "fetch", "pull"]
+      or
+      this = "hg" and exists(result)
+    }
+
+    /** Gets an example argument that can cause this command to execute arbitrary code. */
+    string getVulnerableArgumentExample() {
+      this = "git" and result = "--upload-pack"
+      or
+      this = "hg" and result = "--config=alias.<alias>=<command>"
+    }
+  }
+
+  /** A source for second order command injection. */
+  abstract class Source extends DataFlow::Node {
+    /** Gets a string that describes the source. For use in the alert message. */
+    abstract string describe();
+
+    /** Gets a label for which this is a source. */
+    abstract DataFlow::FlowLabel getALabel();
+  }
+
+  /** A parameter of an exported function, seen as a source for second order command injection. */
+  class ExternalInputSource extends Source {
+    ExternalInputSource() { this = Exports::getALibraryInputParameter() }
+
+    override string describe() { result = "library input" }
+
+    override DataFlow::FlowLabel getALabel() { result = TaintedObject::label() or result.isTaint() }
+  }
+
+  /** A source of remote flow, seen as a source for second order command injection. */
+  class RemoteFlowAsSource extends Source instanceof RemoteFlowSource {
+    override string describe() { result = "a user-provided value" }
+
+    override DataFlow::FlowLabel getALabel() { result.isTaint() }
+  }
+
+  private class TaintedObjectSourceAsSource extends Source instanceof TaintedObject::Source {
+    override DataFlow::FlowLabel getALabel() { result = TaintedObject::label() }
+
+    override string describe() { result = "a user-provided value" }
+  }
+
+  /** A sanitizer for second order command injection. */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /** A sink for second order command injection. */
+  abstract class Sink extends DataFlow::Node {
+    /** Gets a label for which this is a sink. */
+    abstract DataFlow::FlowLabel getALabel();
+
+    /** Gets the command getting invoked. I.e. `git` or `hg`. */
+    abstract string getCommand();
+
+    /**
+     * Gets an example argument for the comand that allows for second order command injection.
+     * E.g. `--upload-pack` for `git`.
+     */
+    abstract string getVulnerableArgumentExample();
+  }
+
+  /**
+   * A sink that invokes a command described by the `VulnerableCommand` class.
+   */
+  abstract class VulnerableCommandSink extends Sink {
+    VulnerableCommand cmd;
+
+    override string getCommand() { result = cmd }
+
+    override string getVulnerableArgumentExample() { result = cmd.getVulnerableArgumentExample() }
+  }
+
+  /** A call that (indirectly) executes a shell command with a list of arguments. */
+  abstract private class CommandExecutingCall extends DataFlow::CallNode {
+    /** Gets the dataflow node representing the command to execute. */
+    abstract DataFlow::Node getCommandArg();
+
+    /** Gets the dataflow node representing the arguments to the command. */
+    abstract DataFlow::Node getArgList();
+  }
+
+  /** A `SystemCommandExecution` seen as a command executing call. */
+  private class SystemExecAsCmdCall extends CommandExecutingCall instanceof SystemCommandExecution {
+    override DataFlow::Node getCommandArg() {
+      result = SystemCommandExecution.super.getACommandArgument()
+    }
+
+    override DataFlow::Node getArgList() { result = SystemCommandExecution.super.getArgumentList() }
+  }
+
+  /** A function whose parameters is directly used a command and argument list for a shell invocation. */
+  private class IndirectCmdFunc extends DataFlow::FunctionNode {
+    int cmdIndex;
+    int argIndex;
+
+    IndirectCmdFunc() {
+      exists(CommandExecutingCall call |
+        this.getParameter(cmdIndex).flowsTo(call.getCommandArg()) and
+        this.getParameter(argIndex).flowsTo(call.getArgList())
+      )
+    }
+
+    /** Gets the parameter index that indicates the command to be executed. */
+    int getCmdIndex() { result = cmdIndex }
+
+    /** Gets the parameter index that indicates the argument list to be passed to the command. */
+    int getArgIndex() { result = argIndex }
+  }
+
+  /** A call to a function that eventually executes a shell command with a list of arguments. */
+  private class IndirectExecCall extends DataFlow::CallNode, CommandExecutingCall {
+    IndirectCmdFunc func;
+
+    IndirectExecCall() { this.getACallee() = func.getFunction() }
+
+    override DataFlow::Node getCommandArg() { result = this.getArgument(func.getCmdIndex()) }
+
+    override DataFlow::Node getArgList() { result = this.getArgument(func.getArgIndex()) }
+  }
+
+  /** Gets a dataflow node that ends up being used as an argument list to an invocation of `git` or `hg`. */
+  private DataFlow::SourceNode usedAsVersionControlArgs(
+    DataFlow::TypeBackTracker t, DataFlow::Node argList, VulnerableCommand cmd
+  ) {
+    t.start() and
+    exists(CommandExecutingCall exec | exec.getCommandArg().mayHaveStringValue(cmd) |
+      exec.getArgList() = argList and
+      result = argList.getALocalSource()
+    )
+    or
+    exists(DataFlow::TypeBackTracker t2 |
+      result = usedAsVersionControlArgs(t2, argList, cmd).backtrack(t2, t)
+    )
+  }
+
+  /** An argument to an invocation of `git`/`hg` that can cause second order command injection. */
+  class ArgSink extends VulnerableCommandSink {
+    ArgSink() {
+      exists(DataFlow::ArrayCreationNode args |
+        args = usedAsVersionControlArgs(DataFlow::TypeBackTracker::end(), _, cmd)
+      |
+        this = [args.getAnElement(), args.getASpreadArgument()] and
+        args.getElement(0).mayHaveStringValue(cmd.getAVulnerableSubCommand()) and
+        // not an "--" argument (even if it's earlier, then we assume it's on purpose)
+        not args.getElement(_).mayHaveStringValue("--")
+      )
+    }
+
+    override DataFlow::FlowLabel getALabel() { result.isTaint() }
+  }
+
+  /**
+   * An arguments array given to an invocation of `git` or `hg` that can cause second order command injection.
+   */
+  class ArgsArraySink extends VulnerableCommandSink {
+    ArgsArraySink() {
+      exists(SystemExecAsCmdCall exec | exec.getCommandArg().mayHaveStringValue(cmd) |
+        this = exec.getArgList()
+      )
+    }
+
+    // only vulnerable if an attacker controls the entire array
+    override DataFlow::FlowLabel getALabel() { result = TaintedObject::label() }
+  }
+
+  /**
+   * A sanitizer that blocks flow when a string is tested to start with a certain prefix.
+   */
+  class PrefixStringSanitizer extends TaintTracking::SanitizerGuardNode instanceof StringOps::StartsWith {
+    override predicate sanitizes(boolean outcome, Expr e) {
+      e = super.getBaseString().asExpr() and
+      outcome = super.getPolarity()
+    }
+  }
+
+  /**
+   * A sanitizer that blocks flow when a string does not start with "--"
+   */
+  class DoubleDashSanitizer extends TaintTracking::SanitizerGuardNode instanceof StringOps::StartsWith {
+    DoubleDashSanitizer() { super.getSubstring().mayHaveStringValue("--") }
+
+    override predicate sanitizes(boolean outcome, Expr e) {
+      e = super.getBaseString().asExpr() and
+      outcome = super.getPolarity().booleanNot()
+    }
+  }
+
+  /** A call to path.relative which sanitizes the taint. */
+  class PathRelativeSanitizer extends Sanitizer {
+    PathRelativeSanitizer() { this = NodeJSLib::Path::moduleMember("relative").getACall() }
+  }
+}

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

@@ -0,0 +1,41 @@
+/**
+ * Provides a taint tracking configuration for reasoning about
+ * second order command-injection vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if
+ * `SecondOrderCommandInjection::Configuration` is needed, otherwise
+ * `SecondOrderCommandInjectionCustomizations` should be imported instead.
+ */
+
+import javascript
+import SecondOrderCommandInjectionCustomizations::SecondOrderCommandInjection
+private import semmle.javascript.security.TaintedObject
+
+/**
+ * A taint-tracking configuration for reasoning about command-injection vulnerabilities.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "SecondOrderCommandInjection" }
+
+  override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
+    source.(Source).getALabel() = label
+  }
+
+  override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
+    sink.(Sink).getALabel() = label
+  }
+
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+
+  override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
+    guard instanceof PrefixStringSanitizer or
+    guard instanceof DoubleDashSanitizer or
+    guard instanceof TaintedObject::SanitizerGuard
+  }
+
+  override predicate isAdditionalFlowStep(
+    DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
+  ) {
+    TaintedObject::step(src, trg, inlbl, outlbl)
+  }
+}

javascript/ql/src/Security/CWE-078/SecondOrderCommandInjection.qhelp

@@ -0,0 +1,46 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Some shell commands, like <code>git ls-remote</code>, can execute 
+arbitrary commands if a user provides a malicious URL that starts with 
+<code>--upload-pack</code>. This can be used to execute arbitrary code on
+the server.
+</p>
+
+</overview>
+
+<recommendation>
+
+<p>
+Sanitize user input before passing it to the shell command. For example,
+ensure that URLs are valid and do not contain malicious commands.
+</p>
+
+</recommendation>
+<example>
+
+<p>
+The following example shows code that executes <code>git ls-remote</code> on a
+URL that can be controlled by a malicious user.
+</p>
+
+<sample src="examples/second-order-command-injection.js" />
+
+<p>
+The problem has been fixed in the snippet below, where the URL is validated before
+being passed to the shell command.
+</p>
+
+<sample src="examples/second-order-command-injection-fixed.js" />
+
+</example>
+<references>
+<li>Max Justicz: <a href="https://justi.cz/security/2021/04/20/cocoapods-rce.html">Hacking 3,000,000 apps at once through CocoaPods</a>.</li>
+<li>Git: <a href="https://git-scm.com/docs/git-ls-remote/2.22.0#Documentation/git-ls-remote.txt---upload-packltexecgt">Git - git-ls-remote Documentation</a>.</li>
+<li>OWASP: <a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.</li>
+
+</references>
+</qhelp>

javascript/ql/src/Security/CWE-078/SecondOrderCommandInjection.ql

@@ -0,0 +1,25 @@
+/**
+ * @name Second order command injection
+ * @description Using user-controlled data as arguments to some commands, such as git clone,
+ *              can allow arbitrary commands to be executed.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 7.0
+ * @precision high
+ * @id js/second-order-command-line-injection
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-088
+ */
+
+import javascript
+import DataFlow::PathGraph
+import semmle.javascript.security.dataflow.SecondOrderCommandInjectionQuery
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
+where cfg.hasFlowPath(source, sink) and sinkNode = sink.getNode()
+select sink.getNode(), source, sink,
+  "Command line argument that depends on $@ can execute an arbitrary command if " +
+    sinkNode.getVulnerableArgumentExample() + " is used with " + sinkNode.getCommand() + ".",
+  source.getNode(), source.getNode().(Source).describe()

javascript/ql/src/Security/CWE-078/examples/second-order-command-injection-fixed.js

@@ -0,0 +1,12 @@
+const express = require("express");
+const app = express();
+
+const cp = require("child_process");
+
+app.get("/ls-remote", (req, res) => {
+  const remote = req.query.remote;
+  if (!(remote.startsWith("git@") || remote.startsWith("https://"))) {
+    throw new Error("Invalid remote: " + remote);
+  }
+  cp.execFile("git", ["ls-remote", remote]); // OK
+});

javascript/ql/src/Security/CWE-078/examples/second-order-command-injection.js

@@ -0,0 +1,9 @@
+const express = require("express");
+const app = express();
+
+const cp = require("child_process");
+
+app.get("/ls-remote", (req, res) => {
+  const remote = req.query.remote;
+  cp.execFile("git", ["ls-remote", remote]); // NOT OK
+});

javascript/ql/src/change-notes/2022-09-05-second-order-command-injection.md

@@ -0,0 +1,7 @@
+---
+category: newQuery
+---
+* Added a new query, `js/second-order-command-line-injection`, to detect shell
+  commands that may execute arbitrary code when the user has control over 
+  the arguments to a command-line program.
+  This currently flags up unsafe invocations of git and hg.

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

@@ -50,8 +50,6 @@ nodes
 | 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 |
-| child_process-test.js:85:37:85:54 | req.query.fileName |
-| child_process-test.js:85:37:85:54 | req.query.fileName |
 | child_process-test.js:94:11:94:35 | "ping " ... ms.host |
 | child_process-test.js:94:11:94:35 | "ping " ... ms.host |
 | child_process-test.js:94:21:94:30 | ctx.params |
@@ -134,16 +132,6 @@ nodes
 | form-parsers.js:59:10:59:33 | "touch  ... ilename |
 | form-parsers.js:59:21:59:24 | part |
 | form-parsers.js:59:21:59:33 | part.filename |
-| lib/subLib4/index.js:6:32:6:35 | name |
-| lib/subLib4/index.js:7:18:7:21 | name |
-| lib/subLib4/subsub.js:3:28:3:31 | name |
-| lib/subLib4/subsub.js:4:10:4:25 | "rm -rf " + name |
-| lib/subLib4/subsub.js:4:10:4:25 | "rm -rf " + name |
-| lib/subLib4/subsub.js:4:22:4:25 | name |
-| lib/subLib/index.js:7:32:7:35 | name |
-| lib/subLib/index.js:8:10:8:25 | "rm -rf " + name |
-| lib/subLib/index.js:8:10:8:25 | "rm -rf " + name |
-| lib/subLib/index.js:8:22:8:25 | name |
 | other.js:5:9:5:49 | cmd |
 | other.js:5:15:5:38 | url.par ... , true) |
 | other.js:5:15:5:44 | url.par ... ).query |
@@ -238,10 +226,6 @@ edges
 | child_process-test.js:73:25:73:31 | req.url | child_process-test.js:73:15:73:38 | url.par ... , true) |
 | child_process-test.js:73:25:73:31 | req.url | child_process-test.js:73:15:73:38 | url.par ... , true) |
 | 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/subLib4/index.js:6:32:6:35 | name |
-| child_process-test.js:85:37:85:54 | req.query.fileName | lib/subLib4/index.js:6:32:6:35 | name |
-| 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 |
 | child_process-test.js:94:21:94:30 | ctx.params | child_process-test.js:94:21:94:35 | ctx.params.host |
 | child_process-test.js:94:21:94:30 | ctx.params | child_process-test.js:94:21:94:35 | ctx.params.host |
 | child_process-test.js:94:21:94:35 | ctx.params.host | child_process-test.js:94:11:94:35 | "ping " ... ms.host |
@@ -314,14 +298,6 @@ edges
 | form-parsers.js:59:21:59:24 | part | form-parsers.js:59:21:59:33 | part.filename |
 | form-parsers.js:59:21:59:33 | part.filename | form-parsers.js:59:10:59:33 | "touch  ... ilename |
 | form-parsers.js:59:21:59:33 | part.filename | form-parsers.js:59:10:59:33 | "touch  ... ilename |
-| lib/subLib4/index.js:6:32:6:35 | name | lib/subLib4/index.js:7:18:7:21 | name |
-| lib/subLib4/index.js:7:18:7:21 | name | lib/subLib4/subsub.js:3:28:3:31 | name |
-| lib/subLib4/subsub.js:3:28:3:31 | name | lib/subLib4/subsub.js:4:22:4:25 | name |
-| lib/subLib4/subsub.js:4:22:4:25 | name | lib/subLib4/subsub.js:4:10:4:25 | "rm -rf " + name |
-| lib/subLib4/subsub.js:4:22:4:25 | name | lib/subLib4/subsub.js:4:10:4:25 | "rm -rf " + name |
-| 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:8:10:8:25 | "rm -rf " + name |
-| lib/subLib/index.js:8:22:8:25 | name | lib/subLib/index.js:8:10:8:25 | "rm -rf " + name |
 | other.js:5:9:5:49 | cmd | other.js:7:33:7:35 | cmd |
 | other.js:5:9:5:49 | cmd | other.js:7:33:7:35 | cmd |
 | other.js:5:9:5:49 | cmd | other.js:8:28:8:30 | cmd |
@@ -398,8 +374,6 @@ edges
 | form-parsers.js:41:10:41:31 | "touch  ... ds.name | form-parsers.js:40:26:40:31 | fields | form-parsers.js:41:10:41:31 | "touch  ... ds.name | This command line depends on a $@. | form-parsers.js:40:26:40:31 | fields | user-provided value |
 | form-parsers.js:53:10:53:31 | "touch  ... ds.name | form-parsers.js:52:34:52:39 | fields | form-parsers.js:53:10:53:31 | "touch  ... ds.name | This command line depends on a $@. | form-parsers.js:52:34:52:39 | fields | user-provided value |
 | form-parsers.js:59:10:59:33 | "touch  ... ilename | form-parsers.js:58:30:58:33 | part | form-parsers.js:59:10:59:33 | "touch  ... ilename | This command line depends on a $@. | form-parsers.js:58:30:58:33 | part | user-provided value |
-| lib/subLib4/subsub.js:4:10:4:25 | "rm -rf " + name | child_process-test.js:85:37:85:54 | req.query.fileName | lib/subLib4/subsub.js:4:10:4:25 | "rm -rf " + name | This command line depends on a $@. | child_process-test.js:85:37:85:54 | req.query.fileName | 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 line depends on a $@. | child_process-test.js:85:37:85:54 | req.query.fileName | 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 line depends on a $@. | other.js:5:25:5:31 | req.url | 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 line depends on a $@. | other.js:5:25:5:31 | req.url | user-provided value |
 | other.js:9:32:9:34 | cmd | other.js:5:25:5:31 | req.url | other.js:9:32:9:34 | cmd | This command line depends on a $@. | other.js:5:25:5:31 | req.url | user-provided value |

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

@@ -4,6 +4,7 @@ import semmle.javascript.security.dataflow.CommandInjection
 import semmle.javascript.security.dataflow.IndirectCommandInjection
 import semmle.javascript.security.dataflow.ShellCommandInjectionFromEnvironment
 import semmle.javascript.security.dataflow.UnsafeShellCommandConstruction
+import semmle.javascript.security.dataflow.SecondOrderCommandInjectionQuery
 
 class CommandInjectionConsistency extends ConsistencyConfiguration {
   CommandInjectionConsistency() { this = "ComandInjection" }

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

@@ -0,0 +1,58 @@
+nodes
+| second-order.js:6:9:6:33 | remote |
+| second-order.js:6:18:6:33 | req.query.remote |
+| second-order.js:6:18:6:33 | req.query.remote |
+| second-order.js:7:33:7:38 | remote |
+| second-order.js:7:33:7:38 | remote |
+| second-order.js:9:29:9:34 | remote |
+| second-order.js:9:29:9:34 | remote |
+| second-order.js:11:33:11:38 | remote |
+| second-order.js:11:33:11:38 | remote |
+| second-order.js:13:9:13:31 | myArgs |
+| second-order.js:13:18:13:31 | req.query.args |
+| second-order.js:13:18:13:31 | req.query.args |
+| second-order.js:15:19:15:24 | myArgs |
+| second-order.js:15:19:15:24 | myArgs |
+| second-order.js:26:35:26:40 | remote |
+| second-order.js:26:35:26:40 | remote |
+| second-order.js:29:19:29:32 | req.query.args |
+| second-order.js:29:19:29:32 | req.query.args |
+| second-order.js:29:19:29:32 | req.query.args |
+| second-order.js:40:28:40:43 | req.query.remote |
+| second-order.js:40:28:40:43 | req.query.remote |
+| second-order.js:40:28:40:43 | req.query.remote |
+| second-order.js:42:31:42:46 | req.query.remote |
+| second-order.js:42:31:42:46 | req.query.remote |
+| second-order.js:42:31:42:46 | req.query.remote |
+| second-order.js:44:18:44:31 | req.query.args |
+| second-order.js:44:18:44:31 | req.query.args |
+| second-order.js:44:18:44:31 | req.query.args |
+edges
+| second-order.js:6:9:6:33 | remote | second-order.js:7:33:7:38 | remote |
+| second-order.js:6:9:6:33 | remote | second-order.js:7:33:7:38 | remote |
+| second-order.js:6:9:6:33 | remote | second-order.js:9:29:9:34 | remote |
+| second-order.js:6:9:6:33 | remote | second-order.js:9:29:9:34 | remote |
+| second-order.js:6:9:6:33 | remote | second-order.js:11:33:11:38 | remote |
+| second-order.js:6:9:6:33 | remote | second-order.js:11:33:11:38 | remote |
+| second-order.js:6:9:6:33 | remote | second-order.js:26:35:26:40 | remote |
+| second-order.js:6:9:6:33 | remote | second-order.js:26:35:26:40 | remote |
+| second-order.js:6:18:6:33 | req.query.remote | second-order.js:6:9:6:33 | remote |
+| second-order.js:6:18:6:33 | req.query.remote | second-order.js:6:9:6:33 | remote |
+| second-order.js:13:9:13:31 | myArgs | second-order.js:15:19:15:24 | myArgs |
+| second-order.js:13:9:13:31 | myArgs | second-order.js:15:19:15:24 | myArgs |
+| second-order.js:13:18:13:31 | req.query.args | second-order.js:13:9:13:31 | myArgs |
+| second-order.js:13:18:13:31 | req.query.args | second-order.js:13:9:13:31 | myArgs |
+| second-order.js:29:19:29:32 | req.query.args | second-order.js:29:19:29:32 | req.query.args |
+| second-order.js:40:28:40:43 | req.query.remote | second-order.js:40:28:40:43 | req.query.remote |
+| second-order.js:42:31:42:46 | req.query.remote | second-order.js:42:31:42:46 | req.query.remote |
+| second-order.js:44:18:44:31 | req.query.args | second-order.js:44:18:44:31 | req.query.args |
+#select
+| second-order.js:7:33:7:38 | remote | second-order.js:6:18:6:33 | req.query.remote | second-order.js:7:33:7:38 | remote | Command line argument that depends on $@ can execute an arbitrary command if --upload-pack is used with git. | second-order.js:6:18:6:33 | req.query.remote | a user-provided value |
+| second-order.js:9:29:9:34 | remote | second-order.js:6:18:6:33 | req.query.remote | second-order.js:9:29:9:34 | remote | Command line argument that depends on $@ can execute an arbitrary command if --upload-pack is used with git. | second-order.js:6:18:6:33 | req.query.remote | a user-provided value |
+| second-order.js:11:33:11:38 | remote | second-order.js:6:18:6:33 | req.query.remote | second-order.js:11:33:11:38 | remote | Command line argument that depends on $@ can execute an arbitrary command if --upload-pack is used with git. | second-order.js:6:18:6:33 | req.query.remote | a user-provided value |
+| second-order.js:15:19:15:24 | myArgs | second-order.js:13:18:13:31 | req.query.args | second-order.js:15:19:15:24 | myArgs | Command line argument that depends on $@ can execute an arbitrary command if --upload-pack is used with git. | second-order.js:13:18:13:31 | req.query.args | a user-provided value |
+| second-order.js:26:35:26:40 | remote | second-order.js:6:18:6:33 | req.query.remote | second-order.js:26:35:26:40 | remote | Command line argument that depends on $@ can execute an arbitrary command if --upload-pack is used with git. | second-order.js:6:18:6:33 | req.query.remote | a user-provided value |
+| second-order.js:29:19:29:32 | req.query.args | second-order.js:29:19:29:32 | req.query.args | second-order.js:29:19:29:32 | req.query.args | Command line argument that depends on $@ can execute an arbitrary command if --upload-pack is used with git. | second-order.js:29:19:29:32 | req.query.args | a user-provided value |
+| second-order.js:40:28:40:43 | req.query.remote | second-order.js:40:28:40:43 | req.query.remote | second-order.js:40:28:40:43 | req.query.remote | Command line argument that depends on $@ can execute an arbitrary command if --config=alias.<alias>=<command> is used with hg. | second-order.js:40:28:40:43 | req.query.remote | a user-provided value |
+| second-order.js:42:31:42:46 | req.query.remote | second-order.js:42:31:42:46 | req.query.remote | second-order.js:42:31:42:46 | req.query.remote | Command line argument that depends on $@ can execute an arbitrary command if --config=alias.<alias>=<command> is used with hg. | second-order.js:42:31:42:46 | req.query.remote | a user-provided value |
+| second-order.js:44:18:44:31 | req.query.args | second-order.js:44:18:44:31 | req.query.args | second-order.js:44:18:44:31 | req.query.args | Command line argument that depends on $@ can execute an arbitrary command if --config=alias.<alias>=<command> is used with hg. | second-order.js:44:18:44:31 | req.query.args | a user-provided value |

javascript/ql/test/query-tests/Security/CWE-078/SecondOrderCommandInjection/SecondOrderCommandInjection.qlref

@@ -0,0 +1 @@
+Security/CWE-078/SecondOrderCommandInjection.ql

javascript/ql/test/query-tests/Security/CWE-078/SecondOrderCommandInjection/second-order.js

@@ -0,0 +1,53 @@
+const express = require("express");
+const app = express();
+const { execFile } = require("child_process");
+
+app.get("/", (req, res) => {
+  const remote = req.query.remote;
+  execFile("git", ["ls-remote", remote]); // NOT OK
+
+  execFile("git", ["fetch", remote]); // NOT OK
+
+  indirect("git", ["ls-remote", remote]); // NOT OK
+
+  const myArgs = req.query.args;
+
+  execFile("git", myArgs); // NOT OK
+
+  if (remote.startsWith("--")) {
+    execFile("git", ["ls-remote", remote, "HEAD"]); // OK - it is very explicit that options that allowed here. 
+  } else {
+    execFile("git", ["ls-remote", remote, "HEAD"]); // OK - it's not an option
+  }
+
+  if (remote.startsWith("git@")) {
+    execFile("git", ["ls-remote", remote, "HEAD"]); // OK - it's a git URL
+  } else {
+    execFile("git", ["ls-remote", remote, "HEAD"]); // NOT OK - unknown starting string
+  }
+
+  execFile("git", req.query.args); // NOT OK - unknown args
+
+  execFile("git", ["add", req.query.args]); // OK - git add is not a command that can be used to execute arbitrary code
+
+  execFile("git", ["add", req.query.remote].concat([otherargs()])); // OK - git add is not a command that can be used to execute arbitrary code
+
+  execFile("git", ["ls-remote", req.query.remote].concat(req.query.otherArgs)); // NOT OK - but not found [INCONSISTENCY]. It's hard to track through concat.
+
+  execFile("git", ["add", "fpp"].concat(req.query.notVulnerable)); // OK
+
+  // hg
+  execFile("hg", ["clone", req.query.remote]); // NOT OK
+
+  execFile("hg", ["whatever", req.query.remote]); // NOT OK - `--config=alias.whatever=touch pwned`
+
+  execFile("hg", req.query.args); // NOT OK - unknown args
+
+  execFile("hg", ["clone", "--", req.query.remote]); // OK
+});
+
+function indirect(cmd, args) {
+  execFile(cmd, args); //  - OK - ish, the vulnerability not reported here
+}
+
+app.listen(3000, () => console.log("Example app listening on port 3000!"));

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

@@ -1,8 +1,4 @@
 readFile
-| lib/lib.js:71:2:71:32 | cp.exec ... + name) | fs.readFile("/foO/BAR/" + name) |
-| lib/lib.js:73:2:73:32 | cp.exec ... + "\\"") | fs.readFile(""" + name + """) |
-| lib/lib.js:75:2:75:30 | cp.exec ...  + "'") | fs.readFile("'" + name + "'") |
-| lib/lib.js:77:2:77:38 | cp.exec ...  + "'") | fs.readFile("'/foo/bar" + name + "'") |
 | uselesscat.js:10:1:10:43 | exec("c ... ut) {}) | fs.readFile("foo/bar", function(err, out) {...}) |
 | uselesscat.js:12:1:14:2 | exec("c ... ut);\\n}) | fs.readFile("/proc/" + id + "/status", function(err, out) {...}) |
 | uselesscat.js:16:1:16:29 | execSyn ... uinfo') | fs.readFileSync("/proc/cpuinfo") |
@@ -39,31 +35,6 @@ readFile
 | uselesscat.js:163:1:163:42 | execmod ... utf8'}) | fs.readFile("foo/bar") |
 | uselesscat.js:164:1:164:76 | execmod ... (out)}) | fs.readFile("foo/bar", {encoding: 'utf8'}, (err, out) => {...}) |
 syncCommand
-| child_process-test.js:9:5:9:22 | cp.execSync("foo") |
-| child_process-test.js:11:5:11:26 | cp.exec ... ("foo") |
-| child_process-test.js:13:5:13:23 | cp.spawnSync("foo") |
-| child_process-test.js:18:5:18:20 | cp.execSync(cmd) |
-| child_process-test.js:20:5:20:24 | cp.execFileSync(cmd) |
-| child_process-test.js:22:5:22:21 | cp.spawnSync(cmd) |
-| command-line-parameter-command-injection.js:11:2:11:21 | cp.execSync(args[0]) |
-| command-line-parameter-command-injection.js:12:2:12:33 | cp.exec ... rgs[0]) |
-| command-line-parameter-command-injection.js:15:2:15:26 | cp.exec ... rgs[0]) |
-| command-line-parameter-command-injection.js:16:2:16:38 | cp.exec ... rgs[0]) |
-| command-line-parameter-command-injection.js:19:2:19:18 | cp.execSync(arg0) |
-| command-line-parameter-command-injection.js:20:2:20:30 | cp.exec ... + arg0) |
-| command-line-parameter-command-injection.js:26:2:26:51 | cp.exec ... tion"`) |
-| command-line-parameter-command-injection.js:27:2:27:58 | cp.exec ... tion"`) |
-| lib/lib.js:419:2:419:52 | cp.exec ...  true}) |
-| lib/lib.js:420:2:420:49 | cp.spaw ...  true}) |
-| other.js:7:5:7:36 | require ... nc(cmd) |
-| other.js:9:5:9:35 | require ... nc(cmd) |
-| other.js:12:5:12:30 | require ... nc(cmd) |
-| other.js:30:5:30:36 | require ... ")(cmd) |
-| third-party-command-injection.js:6:9:6:28 | cp.execSync(command) |
-| tst_shell-command-injection-from-environment.js:5:2:5:62 | cp.exec ... emp")]) |
-| tst_shell-command-injection-from-environment.js:6:2:6:54 | cp.exec ... temp")) |
-| tst_shell-command-injection-from-environment.js:9:2:9:58 | execa.s ... temp")) |
-| tst_shell-command-injection-from-environment.js:12:2:12:34 | execa.s ... + safe) |
 | uselesscat.js:16:1:16:29 | execSyn ... uinfo') |
 | uselesscat.js:18:1:18:26 | execSyn ... path}`) |
 | uselesscat.js:20:1:20:36 | execSyn ... wc -l') |
@@ -95,21 +66,6 @@ syncCommand
 | uselesscat.js:158:16:158:46 | cspawn. ... /bar']) |
 | uselesscat.js:159:16:159:68 | cspawn. ... tf8' }) |
 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' } |
-| lib/lib.js:417:2:417:66 | cp.exec ...  => {}) | lib/lib.js:417:35:417:47 | {shell: true} |
-| lib/lib.js:418:2:418:45 | cp.spaw ...  true}) | lib/lib.js:418:32:418:44 | {shell: true} |
-| lib/lib.js:419:2:419:52 | cp.exec ...  true}) | lib/lib.js:419:39:419:51 | {shell: true} |
-| lib/lib.js:420:2:420:49 | cp.spaw ...  true}) | lib/lib.js:420:36:420:48 | {shell: true} |
-| lib/lib.js:424:2:424:40 | spawn(" ... WN_OPT) | lib/lib.js:424:31:424:39 | SPAWN_OPT |
-| lib/lib.js:427:2:427:28 | spawn(" ... WN_OPT) | lib/lib.js:427:19:427:27 | SPAWN_OPT |
-| lib/lib.js:428:2:428:70 | spawn(" ... WN_OPT) | lib/lib.js:428:61:428:69 | SPAWN_OPT |
 | uselesscat.js:28:1:28:39 | execSyn ...  1000}) | uselesscat.js:28:28:28:38 | {uid: 1000} |
 | uselesscat.js:30:1:30:64 | exec('c ... t) { }) | uselesscat.js:30:26:30:38 | { cwd: './' } |
 | uselesscat.js:34:1:34:54 | execSyn ... utf8'}) | uselesscat.js:34:36:34:53 | {encoding: 'utf8'} |