Merge pull request github#13661 from github/max-schaefer/improve-command-injection-qhelp

JavaScript: Improve query help for js/command-line-injection

Author: max-schaefer

javascript/ql/src/Security/CWE-078/CommandInjection.inc.qhelp

@@ -2,43 +2,65 @@
   "-//Semmle//qhelp//EN"
   "qhelp.dtd">
 <qhelp>
-<overview>
-<p>Code that passes user input directly to
-<code>require('child_process').exec</code>, or some other library
-routine that executes a command, allows the user to execute malicious
-code.</p>
 
+<overview>
+<p>Code that passes untrusted user input directly to
+<code>child_process.exec</code> or similar APIs that execute shell commands
+allows the user to execute malicious code.</p>
 </overview>
-<recommendation>
 
-<p>If possible, use hard-coded string literals to specify the command to run
-or library to load. Instead of passing the user input directly to the
-process or library function, examine the user input and then choose
-among hard-coded string literals.</p>
+<recommendation>
+<p>If possible, use APIs that don't run shell commands and that accept command
+arguments as an array of strings rather than a single concatenated string. This
+is both safer and more portable.</p>
 
-<p>If the applicable libraries or commands cannot be determined at
-compile time, then add code to verify that the user input string is
-safe before using it.</p>
+<p>If given arguments as a single string, avoid simply splitting the string on 
+whitespace. Arguments may contain quoted whitespace, causing them to split into 
+multiple arguments. Use a library like <code>shell-quote</code> to parse the string 
+into an array of arguments instead.</p>
 
+<p>If this approach is not viable, then add code to verify that the user input
+string is safe before using it.</p>
 </recommendation>
-<example>
 
-<p>The following example shows code that takes a shell script that can be changed
-maliciously by a user, and passes it straight to <code>child_process.exec</code>
-without examining it first.</p>
+<example>
+<p>The following example shows code that extracts a filename from an HTTP query
+parameter that may contain untrusted data, and then embeds it into a shell
+command to count its lines without examining it first:</p>
 
 <sample src="examples/command-injection.js" />
 
+<p>A malicious user can take advantage of this code by executing arbitrary shell commands. For example, by providing a filename like <code>foo.txt; rm -rf .</code>, the user can first count the lines in <code>foo.txt</code> and subsequently delete all files in the current directory. </p>
+
+<p>To avoid this catastrophic behavior, use an API such as
+<code>child_process.execFileSync</code> that does not spawn a shell by
+default:</p>
+
+<sample src="examples/command-injection_fixed.js" />
+
+<p>If you want to allow the user to specify other options to <code>wc</code>,
+you can use a library like <code>shell-quote</code> to parse the user input into
+an array of arguments without risking command injection:</p>
+
+<sample src="examples/command-injection_shellquote.js" />
+
+<p>Alternatively, the original example can be made safe by checking the filename
+against an allowlist of safe characters before using it:</p>
+
+<sample src="examples/command-injection_allowlist.js" />
 </example>
-<references>
 
+<references>
 <li>
 OWASP:
 <a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
 </li>
-
+<li>
+npm:
+<a href="https://www.npmjs.com/package/shell-quote">shell-quote</a>.
+</li>
 <!--  LocalWords:  CWE untrusted unsanitized Runtime
  -->
-
 </references>
+
 </qhelp>

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

@@ -27,27 +27,25 @@
 
 		<p>
 
-			If possible, use hard-coded string literals to specify the
-			command to run or library to load. Instead of forwarding the
-			command-line arguments to the process, examine the command-line
-			arguments and then choose among hard-coded string literals.
+			If possible, use APIs that don't run shell commands and accept
+			command arguments as an array of strings rather than a single
+			concatenated string. This is both safer and more portable.
 
 		</p>
 
 		<p>
 
-			If the applicable libraries or commands cannot be determined
-			at compile time, then add code to verify that each forwarded
-			command-line argument is properly escaped before using it.
+If given arguments as a single string, avoid simply splitting the string on 
+whitespace. Arguments may contain quoted whitespace, causing them to split into 
+multiple arguments. Use a library like <code>shell-quote</code> to parse the string 
+into an array of arguments instead.
 
 		</p>
 
 		<p>
 
-			If the forwarded command-line arguments are part of the
-			arguments of the system command, prefer a library routine that handles
-			the arguments as an array of strings rather than a single concatenated
-			string. This prevents the unexpected evaluation of special characters.
+			If this approach is not viable, then add code to verify that each
+			forwarded command-line argument is properly escaped before using it.
 
 		</p>
 
@@ -91,6 +89,17 @@
 
 		<sample src="examples/indirect-command-injection_fixed.js" />
 
+		<p>
+
+			If you want to allow the user to specify other options to
+			<code>node</code>, you can use a library like
+			<code>shell-quote</code> to parse the user input into an array of
+			arguments without risking command injection:
+
+		</p>
+
+		<sample src="examples/indirect-command-injection_shellquote.js" />
+
 	</example>
 
 	<references>
@@ -100,6 +109,11 @@
 			<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
 		</li>
 
+		<li>
+			npm:
+			<a href="https://www.npmjs.com/package/shell-quote">shell-quote</a>.
+		</li>
+
 	</references>
 
 </qhelp>

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

@@ -27,9 +27,22 @@
 	</p>
 
 	<p>
-		Alternatively, if the shell command must be constructed
-		dynamically, then add code to ensure that special characters 
-		do not alter the shell command unexpectedly.
+
+		If given arguments as a single string, avoid simply splitting the string
+		on whitespace. Arguments may contain quoted whitespace, causing them to
+		split into multiple arguments. Use a library like
+		<code>shell-quote</code> to parse the string into an array of arguments
+		instead.
+
+	</p>
+
+	<p>
+
+		Alternatively, if the command must be interpreted by a shell (for
+		example because it includes I/O redirections), you can use
+		<code>shell-quote</code> to escape any special characters in the input
+		before embedding it in the command.
+
 	</p>
 
 </recommendation>
@@ -63,6 +76,27 @@
 
 	<sample src="examples/unsafe-shell-command-construction_fixed.js" />
 
+	<p>
+
+		As another example, consider the following code which is similar to the
+		preceding example, but pipes the output of <code>wget</code> into <code>wc -l</code>
+		to count the number of lines in the downloaded file.
+
+	</p>
+
+	<sample src="examples/unsafe-shell-command-construction_pipe.js" />
+
+	<p>
+
+		In this case, using <code>child_process.execFile</code> is not an option
+		because the shell is needed to interpret the pipe operator. Instead, you
+		can use <code>shell-quote</code> to escape the input before embedding it
+		in the command:
+
+	</p>
+
+	<sample src="examples/unsafe-shell-command-construction_pipe_fixed.js" />
+
 </example>
 <references>
 
@@ -71,5 +105,10 @@
 		<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
 	</li>
 
+	<li>
+		npm:
+		<a href="https://www.npmjs.com/package/shell-quote">shell-quote</a>.
+	</li>
+
 </references>
 </qhelp>

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

@@ -3,7 +3,7 @@ var cp = require("child_process"),
     url = require('url');
 
 var server = http.createServer(function(req, res) {
-    let cmd = url.parse(req.url, true).query.path;
+    let file = url.parse(req.url, true).query.path;
 
-    cp.exec(cmd); // BAD
+    cp.execSync(`wc -l ${file}`); // BAD
 });

javascript/ql/src/Security/CWE-078/examples/command-injection_allowlist.js

@@ -0,0 +1,12 @@
+var cp = require("child_process"),
+    http = require('http'),
+    url = require('url');
+
+var server = http.createServer(function(req, res) {
+    let file = url.parse(req.url, true).query.path;
+
+    // only allow safe characters in file name
+    if (file.match(/^[\w\.\-\/]+$/)) {
+        cp.execSync(`wc -l ${file}`); // GOOD
+    }
+});

javascript/ql/src/Security/CWE-078/examples/command-injection_fixed.js

@@ -0,0 +1,9 @@
+var cp = require("child_process"),
+    http = require('http'),
+    url = require('url');
+
+var server = http.createServer(function(req, res) {
+    let file = url.parse(req.url, true).query.path;
+
+    cp.execFileSync('wc', ['-l', file]); // GOOD
+});

javascript/ql/src/Security/CWE-078/examples/command-injection_shellquote.js

@@ -0,0 +1,10 @@
+var cp = require("child_process"),
+    http = require('http'),
+    url = require('url'),
+    shellQuote = require('shell-quote');
+
+var server = http.createServer(function(req, res) {
+    let options = url.parse(req.url, true).query.options;
+
+    cp.execFileSync('wc', shellQuote.parse(options)); // GOOD
+});

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

@@ -2,4 +2,4 @@ var cp = require("child_process");
 
 const args = process.argv.slice(2);
 const script = path.join(__dirname, 'bin', 'main.js');
-cp.execSync(`node ${script} ${args.join(' ')}"`); // BAD
+cp.execSync(`node ${script} ${args.join(' ')}`); // BAD

javascript/ql/src/Security/CWE-078/examples/indirect-command-injection_shellquote.js

@@ -0,0 +1,11 @@
+var cp = require("child_process"),
+    shellQuote = require("shell-quote");
+
+const args = process.argv.slice(2);
+let nodeOpts = '';
+if (args[0] === '--node-opts') {
+    nodeOpts = args[1];
+    args.splice(0, 2);
+}
+const script = path.join(__dirname, 'bin', 'main.js');
+cp.execFileSync('node', shellQuote.parse(nodeOpts).concat(script).concat(args)); // GOOD

javascript/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction.js

@@ -1,5 +1,5 @@
 var cp = require("child_process");
 
 module.exports = function download(path, callback) {
-  cp.execSync("wget " + path, callback);
+  cp.exec("wget " + path, callback);
 }