Merge pull request #2092 from esben-semmle/js/brittle-system-reflection-command

Approved by mchammer01, xiemaisi

change-notes/1.23/analysis-javascript.md

@@ -19,6 +19,7 @@
 | Unused index variable (`js/unused-index-variable`)                        | correctness                                                       | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. Results are shown on LGTM by default. |
 | Loop bound injection (`js/loop-bound-injection`)                          | security, external/cwe/cwe-834                                      | Highlights loops where a user-controlled object with an arbitrary .length value can trick the server to loop indefinitely. Results are not shown on LGTM by default. |
 | Suspicious method name (`js/suspicious-method-name-declaration`)          | correctness, typescript, methods                                  | Highlights suspiciously named methods where the developer likely meant to write a constructor or function. Results are shown on LGTM by default. |
+| Shell command built from environment values (`js/shell-command-injection-from-environment`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights shell commands that may change behavior inadvertently depending on the execution environment, indicating a possible violation of [CWE-78](https://cwe.mitre.org/data/definitions/78.html). Results are shown on LGTM by default.|
 | Use of returnless function (`js/use-of-returnless-function`)              | maintainability, correctness                                      | Highlights calls where the return value is used, but the callee never returns a value. Results are shown on LGTM by default. |
 | Useless regular expression character escape (`js/useless-regexp-character-escape`) | correctness, security, external/cwe/cwe-20 | Highlights regular expression strings with useless character escapes, indicating a possible violation of [CWE-20](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default. |
 | Unreachable method overloads (`js/unreachable-method-overloads`)          | correctness, typescript                                           | Highlights method overloads that are impossible to use from client code. Results are shown on LGTM by default. |

javascript/config/suites/javascript/security

@@ -9,6 +9,7 @@
 + semmlecode-javascript-queries/Security/CWE-022/ZipSlip.ql: /Security/CWE/CWE-022
 + semmlecode-javascript-queries/Security/CWE-078/CommandInjection.ql: /Security/CWE/CWE-078
 + semmlecode-javascript-queries/Security/CWE-078/IndirectCommandInjection.ql: /Security/CWE/CWE-078
++ semmlecode-javascript-queries/Security/CWE-078/ShellCommandInjectionFromEnvironment: /Security/CWE/CWE-078
 + semmlecode-javascript-queries/Security/CWE-079/ReflectedXss.ql: /Security/CWE/CWE-079
 + semmlecode-javascript-queries/Security/CWE-079/StoredXss.ql: /Security/CWE/CWE-079
 + semmlecode-javascript-queries/Security/CWE-079/Xss.ql: /Security/CWE/CWE-079

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

@@ -0,0 +1,103 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+<overview>
+	<p>
+
+		Dynamically constructing a shell command with values from the
+		local environment, such as file paths, may inadvertently
+		change the meaning of the shell command.
+
+		Such changes can occur when an environment value contains
+		characters that the shell interprets in a special way, for instance
+		quotes and spaces.
+
+		This can result in the shell command misbehaving, or even
+		allowing a malicious user to execute arbitrary commands on the system.
+
+	</p>
+
+
+</overview>
+<recommendation>
+
+	<p>
+
+		If possible, use hard-coded string literals to specify the
+		shell command to run, and provide the dynamic arguments to the shell
+		command separately to avoid interpretation by the shell.
+
+	</p>
+
+	<p>
+
+		Alternatively, if the shell command must be constructed
+		dynamically, then add code to ensure that special characters in
+		environment values do not alter the shell command unexpectedly.
+
+	</p>
+
+</recommendation>
+<example>
+
+	<p>
+
+		The following example shows a dynamically constructed shell
+		command that recursively removes a temporary directory that is located
+		next to the currently executing JavaScript file. Such utilities are
+		often found in custom build scripts.
+
+	</p>
+
+	<sample src="examples/shell-command-injection-from-environment.js" />
+
+	<p>
+
+		The shell command will, however, fail to work as intended if the
+		absolute path of the script's directory contains spaces. In that
+		case, the shell command will interpret the absolute path as multiple
+		paths, instead of a single path.
+	</p>
+
+	<p>
+
+		For instance, if the absolute path of
+		the temporary directory is <code>/home/username/important
+		project/temp</code>, then the shell command will recursively delete
+		<code>/home/username/important</code> and <code>project/temp</code>,
+		where the latter path gets resolved relative to the working directory
+		of the JavaScript process.
+
+	</p>
+
+
+	<p>
+
+		Even worse, although less likely, a malicious user could
+		provide the path <code>/home/username/; cat /etc/passwd #/important
+		project/temp</code> in order to execute the command <code>cat
+		/etc/passwd</code>.
+
+	</p>
+
+	<p>
+
+		To avoid such potentially catastrophic behaviors, provide the
+		directory as an argument that does not get interpreted by a
+		shell:
+
+	</p>
+
+	<sample src="examples/shell-command-injection-from-environment_fixed.js" />
+
+</example>
+<references>
+
+	<li>
+		OWASP:
+		<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
+	</li>
+
+</references>
+</qhelp>

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

@@ -0,0 +1,29 @@
+/**
+ * @name Shell command built from environment values
+ * @description Building a shell command string with values from the enclosing
+ *              environment may cause subtle bugs or vulnerabilities.
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision high
+ * @id js/shell-command-injection-from-environment
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-088
+ */
+
+import javascript
+import DataFlow::PathGraph
+import semmle.javascript.security.dataflow.ShellCommandInjectionFromEnvironment::ShellCommandInjectionFromEnvironment
+
+from
+  Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node highlight,
+  Source sourceNode
+where
+  sourceNode = source.getNode() and
+  cfg.hasFlowPath(source, sink) and
+  if cfg.isSinkWithHighlight(sink.getNode(), _)
+  then cfg.isSinkWithHighlight(sink.getNode(), highlight)
+  else highlight = sink.getNode()
+select highlight, source, sink, "This shell command depends on an uncontrolled $@.", sourceNode,
+  sourceNode.getSourceType()

javascript/ql/src/Security/CWE-078/examples/shell-command-injection-from-environment.js

@@ -0,0 +1,6 @@
+var cp = require("child_process"),
+  path = require("path");
+function cleanupTemp() {
+  let cmd = "rm -rf " + path.join(__dirname, "temp");
+  cp.execSync(cmd); // BAD
+}

javascript/ql/src/Security/CWE-078/examples/shell-command-injection-from-environment_fixed.js

@@ -0,0 +1,7 @@
+var cp = require("child_process"),
+  path = require("path");
+function cleanupTemp() {
+  let cmd = "rm",
+    args = ["-rf", path.join(__dirname, "temp")];
+  cp.execFileSync(cmd, args); // GOOD
+}

javascript/ql/src/semmle/javascript/Concepts.qll

@@ -14,6 +14,9 @@ abstract class SystemCommandExecution extends DataFlow::Node {
   /** Gets an argument to this execution that specifies the command. */
   abstract DataFlow::Node getACommandArgument();
 
+  /** Holds if a shell interprets `arg`. */
+  predicate isShellInterpreted(DataFlow::Node arg) { none() }
+
   /**
    * Gets an argument to this command execution that specifies the argument list
    * to the command.

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

@@ -573,21 +573,32 @@ module NodeJSLib {
       this = DataFlow::moduleMember("child_process", methodName).getACall()
     }
 
-    override DataFlow::Node getACommandArgument() {
+    private DataFlow::Node getACommandArgument(boolean shell) {
       // check whether this is an invocation of an exec/spawn/fork method
       (
-        methodName = "exec" or
+        shell = true and
-        methodName = "execSync" or
+        (
-        methodName = "execFile" or
+          methodName = "exec" or
-        methodName = "execFileSync" or
+          methodName = "execSync"
-        methodName = "spawn" or
+        )
-        methodName = "spawnSync" or
+        or
-        methodName = "fork"
+        shell = false and
+        (
+          methodName = "execFile" or
+          methodName = "execFileSync" or
+          methodName = "spawn" or
+          methodName = "spawnSync" or
+          methodName = "fork"
+        )
       ) and
       // all of the above methods take the command as their first argument
       result = getArgument(0)
     }
 
+    override DataFlow::Node getACommandArgument() { result = getACommandArgument(_) }
+
+    override predicate isShellInterpreted(DataFlow::Node arg) { arg = getACommandArgument(true) }
+
     override DataFlow::Node getArgumentList() {
       (
         methodName = "execFile" or

javascript/ql/src/semmle/javascript/frameworks/ShellJS.qll

@@ -158,6 +158,8 @@ module ShellJS {
     ShellJSExec() { name = "exec" }
 
     override DataFlow::Node getACommandArgument() { result = getArgument(0) }
+
+    override predicate isShellInterpreted(DataFlow::Node arg) { arg = getACommandArgument() }
   }
 
   /**

javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll

@@ -8,18 +8,26 @@ import javascript
 private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::InvokeNode {
   int cmdArg;
 
+  boolean shell;
+
   SystemCommandExecutors() {
     exists(string mod, DataFlow::SourceNode callee |
       exists(string method |
-        mod = "cross-spawn" and method = "sync" and cmdArg = 0
+        mod = "cross-spawn" and method = "sync" and cmdArg = 0 and shell = false
         or
         mod = "execa" and
         (
-          method = "shell" or
+          shell = false and
-          method = "shellSync" or
+          (
-          method = "stdout" or
+            method = "shell" or
-          method = "stderr" or
+            method = "shellSync" or
-          method = "sync"
+            method = "stdout" or
+            method = "stderr" or
+            method = "sync"
+          )
+          or
+          shell = true and
+          (method = "command" or method = "commandSync")
         ) and
         cmdArg = 0
       |
@@ -27,17 +35,24 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I
       )
       or
       (
-        mod = "cross-spawn" and cmdArg = 0
+        shell = false and
+        (
+          mod = "cross-spawn" and cmdArg = 0
+          or
+          mod = "cross-spawn-async" and cmdArg = 0
+          or
+          mod = "exec-async" and cmdArg = 0
+          or
+          mod = "execa" and cmdArg = 0
+        )
         or
-        mod = "cross-spawn-async" and cmdArg = 0
+        shell = true and
-        or
+        (
-        mod = "exec" and cmdArg = 0
+          mod = "exec" and
-        or
+          cmdArg = 0
-        mod = "exec-async" and cmdArg = 0
+          or
-        or
+          mod = "remote-exec" and cmdArg = 1
-        mod = "execa" and cmdArg = 0
+        )
-        or
-        mod = "remote-exec" and cmdArg = 1
       ) and
       callee = DataFlow::moduleImport(mod)
     |
@@ -46,4 +61,8 @@ private class SystemCommandExecutors extends SystemCommandExecution, DataFlow::I
   }
 
   override DataFlow::Node getACommandArgument() { result = getArgument(cmdArg) }
+
+  override predicate isShellInterpreted(DataFlow::Node arg) {
+    arg = getACommandArgument() and shell = true
+  }
 }

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

@@ -0,0 +1,34 @@
+/**
+ * Provides a taint tracking configuration for reasoning about
+ * command-injection vulnerabilities (CWE-078).
+ *
+ * Note, for performance reasons: only import this file if
+ * `ShellCommandInjectionFromEnvironment::Configuration` is needed, otherwise
+ * `ShellCommandInjectionFromEnvironmentCustomizations` should be imported instead.
+ */
+
+import javascript
+
+module ShellCommandInjectionFromEnvironment {
+  import ShellCommandInjectionFromEnvironmentCustomizations::ShellCommandInjectionFromEnvironment
+  import IndirectCommandArgument
+
+  /**
+   * A taint-tracking configuration for reasoning about command-injection vulnerabilities.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "ShellCommandInjectionFromEnvironment" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    predicate isSinkWithHighlight(DataFlow::Node sink, DataFlow::Node highlight) {
+      sink instanceof Sink and highlight = sink
+      or
+      isIndirectCommandArgument(sink, highlight)
+    }
+
+    override predicate isSink(DataFlow::Node sink) { isSinkWithHighlight(sink, _) }
+
+    override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+  }
+}

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

@@ -0,0 +1,58 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * command-injection vulnerabilities, as well as extension points for
+ * adding your own.
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.TaintedPathCustomizations
+
+module ShellCommandInjectionFromEnvironment {
+  /**
+   * A data flow source for command-injection vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node {
+    /** Gets a string that describes the type of this data flow source. */
+    abstract string getSourceType();
+  }
+
+  /**
+   * A data flow sink for command-injection vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A sanitizer for command-injection vulnerabilities.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /** An file name from the local file system, considered as a flow source for command injection. */
+  class FileNameSourceAsSource extends Source {
+    FileNameSourceAsSource() { this instanceof FileNameSource }
+
+    override string getSourceType() { result = "file name" }
+  }
+
+  /** An absolute path from the local file system, considered as a flow source for command injection. */
+  class AbsolutePathSource extends Source {
+    AbsolutePathSource() {
+      exists(ModuleScope ms | this.asExpr() = ms.getVariable("__dirname").getAnAccess())
+      or
+      exists(DataFlow::SourceNode process | process = NodeJSLib::process() |
+        this = process.getAPropertyRead("execPath") or
+        this = process.getAMemberCall("cwd")
+      )
+      or
+      this = any(TaintedPath::ResolvingPathCall c).getOutput()
+    }
+
+    override string getSourceType() { result = "absolute path" }
+  }
+
+  /**
+   * A shell command argument.
+   */
+  class ShellCommandSink extends Sink, DataFlow::ValueNode {
+    ShellCommandSink() { any(SystemCommandExecution sys).isShellInterpreted(this) }
+  }
+}

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

@@ -69,6 +69,7 @@ nodes
 | other.js:19:36:19:38 | cmd |
 | third-party-command-injection.js:5:20:5:26 | command |
 | third-party-command-injection.js:6:21:6:27 | command |
+| tst_shell-command-injection-from-environment.js:4:25:4:61 | ['-rf', ... temp")] |
 edges
 | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:17:13:17:15 | cmd |
 | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:18:17:18:19 | cmd |

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

@@ -54,6 +54,7 @@ nodes
 | command-line-parameter-command-injection.js:27:14:27:57 | `node $ ... ption"` |
 | command-line-parameter-command-injection.js:27:32:27:35 | args |
 | command-line-parameter-command-injection.js:27:32:27:45 | args.join(' ') |
+| tst_shell-command-injection-from-environment.js:4:25:4:61 | ['-rf', ... temp")] |
 edges
 | child_process-test.js:36:7:36:20 | sh | child_process-test.js:39:5:39:5 | sh |
 | child_process-test.js:36:7:36:20 | sh | child_process-test.js:39:14:39:15 | sh |

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

@@ -0,0 +1,46 @@
+nodes
+| child_process-test.js:36:7:36:20 | sh |
+| child_process-test.js:36:12:36:20 | 'cmd.exe' |
+| child_process-test.js:38:7:38:20 | sh |
+| child_process-test.js:38:12:38:20 | '/bin/sh' |
+| child_process-test.js:39:14:39:15 | sh |
+| child_process-test.js:39:18:39:30 | [ flag, cmd ] |
+| child_process-test.js:41:9:41:17 | args |
+| child_process-test.js:41:16:41:17 | [] |
+| child_process-test.js:44:17:44:27 | "/bin/bash" |
+| child_process-test.js:44:30:44:33 | args |
+| child_process-test.js:46:9:46:12 | "sh" |
+| child_process-test.js:46:15:46:18 | args |
+| child_process-test.js:48:9:48:17 | args |
+| child_process-test.js:48:16:48:17 | [] |
+| child_process-test.js:51:17:51:32 | `/bin` + "/bash" |
+| child_process-test.js:51:35:51:38 | args |
+| child_process-test.js:55:14:55:16 | cmd |
+| child_process-test.js:55:19:55:22 | args |
+| child_process-test.js:56:12:56:14 | cmd |
+| child_process-test.js:56:17:56:20 | args |
+| tst_shell-command-injection-from-environment.js:4:25:4:61 | ['-rf', ... temp")] |
+| tst_shell-command-injection-from-environment.js:5:14:5:53 | 'rm -rf ... "temp") |
+| tst_shell-command-injection-from-environment.js:5:26:5:53 | path.jo ... "temp") |
+| tst_shell-command-injection-from-environment.js:5:36:5:44 | __dirname |
+edges
+| child_process-test.js:36:7:36:20 | sh | child_process-test.js:39:5:39:5 | sh |
+| child_process-test.js:36:7:36:20 | sh | child_process-test.js:39:14:39:15 | sh |
+| child_process-test.js:36:12:36:20 | 'cmd.exe' | child_process-test.js:36:7:36:20 | sh |
+| child_process-test.js:38:7:38:20 | sh | child_process-test.js:39:5:39:5 | sh |
+| child_process-test.js:38:7:38:20 | sh | child_process-test.js:39:14:39:15 | sh |
+| child_process-test.js:38:12:38:20 | '/bin/sh' | child_process-test.js:38:7:38:20 | sh |
+| child_process-test.js:39:5:39:5 | sh | child_process-test.js:39:14:39:15 | sh |
+| child_process-test.js:41:9:41:17 | args | child_process-test.js:44:30:44:33 | args |
+| child_process-test.js:41:9:41:17 | args | child_process-test.js:46:15:46:18 | args |
+| child_process-test.js:41:16:41:17 | [] | child_process-test.js:41:9:41:17 | args |
+| child_process-test.js:46:9:46:12 | "sh" | child_process-test.js:55:14:55:16 | cmd |
+| child_process-test.js:46:15:46:18 | args | child_process-test.js:55:19:55:22 | args |
+| child_process-test.js:48:9:48:17 | args | child_process-test.js:51:35:51:38 | args |
+| child_process-test.js:48:16:48:17 | [] | child_process-test.js:48:9:48:17 | args |
+| child_process-test.js:55:14:55:16 | cmd | child_process-test.js:56:12:56:14 | cmd |
+| child_process-test.js:55:19:55:22 | args | child_process-test.js:56:17:56:20 | args |
+| tst_shell-command-injection-from-environment.js:5:26:5:53 | path.jo ... "temp") | tst_shell-command-injection-from-environment.js:5:14:5:53 | 'rm -rf ... "temp") |
+| tst_shell-command-injection-from-environment.js:5:36:5:44 | __dirname | tst_shell-command-injection-from-environment.js:5:26:5:53 | path.jo ... "temp") |
+#select
+| tst_shell-command-injection-from-environment.js:5:14:5:53 | 'rm -rf ... "temp") | tst_shell-command-injection-from-environment.js:5:36:5:44 | __dirname | tst_shell-command-injection-from-environment.js:5:14:5:53 | 'rm -rf ... "temp") | This shell command depends on an uncontrolled $@. | tst_shell-command-injection-from-environment.js:5:36:5:44 | __dirname | absolute path |

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

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

javascript/ql/test/query-tests/Security/CWE-078/tst_shell-command-injection-from-environment.js

@@ -0,0 +1,6 @@
+var cp = require('child_process'),
+    path = require('path');
+(function() {
+	cp.execFileSync('rm',  ['-rf', path.join(__dirname, "temp")]); // GOOD
+	cp.execSync('rm -rf ' + path.join(__dirname, "temp")); // BAD
+});