Address more review feedback.

Author: Max Schaefer

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

@@ -10,22 +10,18 @@ allows the user to execute malicious code.</p>
 </overview>
 
 <recommendation>
-<p>If possible, use hard-coded string literals to specify the command to run.
-Instead of interpreting the user input directly as a shell command, examine the
-user input and then choose among hard-coded string literals.</p>
+<p>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 until runtime,
-then add code to verify that the user input string is safe before using it.</p>
-
-<p>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>In the latter case, if you are given the arguments as a single string, note
-that it is not safe to simply split the string on whitespace, since an argument
-may contain quoted whitespace which would cause it to be split into multiple
-arguments. Instead, use a library such as <code>shell-quote</code> to parse the
-string into an array of arguments.</p>
+<p>If you are given the arguments as a single string, note that it is not safe
+to simply split the string on whitespace, since an argument may contain quoted
+whitespace which would cause it to be split into multiple arguments. Instead,
+use a library such as <code>shell-quote</code> to parse the string into an array
+of arguments.</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>
@@ -45,6 +41,12 @@ directory.</p>
 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" />
 </example>
 
 <references>

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

@@ -27,38 +27,27 @@
 
 		<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 you are given the arguments as a single string, note that it is
+			not safe to simply split the string on whitespace, since an argument
+			may contain quoted whitespace which would cause it to be split into
+			multiple arguments. Instead, use a library such as
+			<code>shell-quote</code> to parse the string into an array of
+			arguments.
 
 		</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.
-
-		</p>
-
-		<p>
-
-			In the latter case, if you are given the arguments as a single
-			string, note that it is not safe to simply split the string on
-			whitespace, since an argument may contain quoted whitespace which
-			would cause it to be split into multiple arguments. Instead, use a
-			library such as <code>shell-quote</code> to parse the string into an
-			array of arguments.
+			If this approach is not viable, then add code to verify that each
+			forwarded command-line argument is properly escaped before using it.
 
 		</p>
 
@@ -102,6 +91,17 @@
 
 		<sample src="examples/indirect-command-injection_fixed.js" />
 
+		<p>
+
+			If we want to allow the user to specify other options to
+			<code>node</code>, we 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>

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

@@ -73,6 +73,18 @@
 
 	<sample src="examples/unsafe-shell-command-construction_fixed.js" />
 
+	<p>
+
+		If you want to allow the user to specify other options to
+		<code>wget</code> as a string, we 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/unsafe-shell-command-construction_shellquote.js" />
+
 </example>
 <references>
 

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

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

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

@@ -1,10 +1,9 @@
 var cp = require("child_process"),
     http = require('http'),
-    path = require('path'),
     url = require('url');
 
 var server = http.createServer(function(req, res) {
-    let file = path.basename(url.parse(req.url, true).query.path);
+    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_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);
 }

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

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

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

@@ -0,0 +1,6 @@
+var cp = require("child_process"),
+    shellQuote = require('shell-quote');
+
+module.exports = function download(path, options, callback) {
+  cp.execFile("wget", shellQuote.parse(options).concat(path), callback);
+}