add second-order-command-injection query

2026-04-30 19:26:02 +02:00 · 2022-09-01 23:42:39 +02:00
parent 0a7e797090
commit fc2112831c
11 changed files with 442 additions and 0 deletions
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/SecondOrderCommandInjectionCustomizations.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/SecondOrderCommandInjectionCustomizations.qll
@@ -0,0 +1,191 @@
+/**
+ * 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)
+    }
+  }
+
+  /** 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();
+  }
+
+  /** 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 Sink {
+    ArgSink() {
+      exists(DataFlow::ArrayCreationNode args, VulnerableCommand cmd |
+        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 Sink {
+    ArgsArraySink() {
+      exists(SystemExecAsCmdCall exec |
+        exec.getCommandArg().mayHaveStringValue(any(VulnerableCommand 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() }
+  }
+}
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/SecondOrderCommandInjectionQuery.qll
+++ b/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)
+  }
+}
--- a/javascript/ql/src/Security/CWE-078/SecondOrderCommandInjection.qhelp
+++ b/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 can start 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 by for example 
+ensuring 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 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>
--- a/javascript/ql/src/Security/CWE-078/SecondOrderCommandInjection.ql
+++ b/javascript/ql/src/Security/CWE-078/SecondOrderCommandInjection.ql
@@ -0,0 +1,24 @@
+/**
+ * @name Second order command injection
+ * @description Some shell programs allow arbitrary command execution via their command line arguments.
+ *              This is a second order command injection vulnerability.
+ * @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
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  "Command line argument that allows for arbitrary command execution depends on $@.",
+  source.getNode(), source.getNode().(Source).describe()
--- a/javascript/ql/src/Security/CWE-078/examples/second-order-command-injection-fixed.js
+++ b/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
+});
--- a/javascript/ql/src/Security/CWE-078/examples/second-order-command-injection.js
+++ b/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
+});
--- a/javascript/ql/src/change-notes/2022-09-05-second-order-command-injection.md
+++ b/javascript/ql/src/change-notes/2022-09-05-second-order-command-injection.md
@@ -0,0 +1,6 @@
+---
+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.
--- a/javascript/ql/test/query-tests/Security/CWE-078/Consistency.ql
+++ b/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" }
--- a/javascript/ql/test/query-tests/Security/CWE-078/SecondOrderCommandInjection/SecondOrderCommandInjection.expected
+++ b/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 allows for arbitrary command execution depends on $@. | 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 allows for arbitrary command execution depends on $@. | 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 allows for arbitrary command execution depends on $@. | 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 allows for arbitrary command execution depends on $@. | 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 allows for arbitrary command execution depends on $@. | 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 allows for arbitrary command execution depends on $@. | 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 allows for arbitrary command execution depends on $@. | 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 allows for arbitrary command execution depends on $@. | 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 allows for arbitrary command execution depends on $@. | second-order.js:44:18:44:31 | req.query.args | a user-provided value |
--- a/javascript/ql/test/query-tests/Security/CWE-078/SecondOrderCommandInjection/SecondOrderCommandInjection.qlref
+++ b/javascript/ql/test/query-tests/Security/CWE-078/SecondOrderCommandInjection/SecondOrderCommandInjection.qlref
@@ -0,0 +1 @@
+Security/CWE-078/SecondOrderCommandInjection.ql
--- a/javascript/ql/test/query-tests/Security/CWE-078/SecondOrderCommandInjection/second-order.js
+++ b/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!"));
				`@@ -0,0 +1 @@`
				`Security/CWE-078/SecondOrderCommandInjection.ql`