Update qhelp for js/path-injection.

Author: Max Schaefer

javascript/ql/src/Security/CWE-022/TaintedPath.qhelp

@@ -13,40 +13,47 @@ attacker being able to influence behavior by modifying unexpected files.
 
 <recommendation>
 <p>
-Validate user input before using it to construct a file path, either using an off-the-shelf library
-like the <code>sanitize-filename</code> npm package, or by performing custom validation.
+Validate user input before using it to construct a file path.
 </p>
 
 <p>
-Ideally, follow these rules:
+The choice of validation depends on whether you want to allow the user to specify complex paths with multiple components that may span multiple folders, or only simple filenames without a path component.
 </p>
 
-<ul>
-<li>Do not allow more than a single "." character.</li>
-<li>Do not allow directory separators such as "/" or "\" (depending on the file system).</li>
-<li>Do not rely on simply replacing problematic sequences such as "../". For example, after
-applying this filter to ".../...//", the resulting string would still be "../".</li>
-<li>Use a whitelist of known good patterns.</li>
-</ul>
+<p>
+In the former case, a common strategy is to make sure that the constructed file path is contained within a safe root folder.
+First, normalize the path using <code>path.resolve</code> or <code>fs.realpathSync</code> to remove any ".." segments.
+Then check that the normalized path starts with the root folder.
+Note that the normalization step is important, since otherwise even a path that starts with the root folder could be used to access files outside the root folder.
+</p>
+
+<p>
+In the latter case, you can use a library like the <code>sanitize-filename</code> npm package to eliminate any special characters from the file path.
+Note that it is <i>not</i> sufficient to only remove "../" sequences: for example, applying this filter to ".../...//" would still result in the string "../".
+</p>
+
+<p>
+Finally, the simplest (but most restrictive) option is to use an allow list of safe patterns and make sure that the user input matches one of these patterns.
+</p>
 </recommendation>
 
 <example>
 <p>
-In the first example, a file name is read from an HTTP request and then used to access a file.
-However, a malicious user could enter a file name which is an absolute path, such as
-<code>"/etc/passwd"</code>.
+In the first example, a file name is read from an HTTP request and then used to access a file within a root folder.
+However, a malicious user could enter a file name containing "../" segments to navigate outside the root folder and access sensitive files.
 </p>
 
+<sample src="examples/TaintedPath.js" />
+
 <p>
-In the second example, it appears that the user is restricted to opening a file within the
-<code>"user"</code> home directory. However, a malicious user could enter a file name containing
-special characters. For example, the string <code>"../../etc/passwd"</code> will result in the code
-reading the file located at <code>"/home/user/../../etc/passwd"</code>, which is the system's
-password file. This file would then be sent back to the user, giving them access to all the
-system's passwords.
+The second example shows how to fix this.
+First, the file name is resolved relative to a root folder, which has the side effect of normalizing the path and removing any "../" segments.
+Then, <code>fs.realpathSync</code> is used to resolve any symbolic links in the path.
+Finally, we check that the normalized path starts with the root folder's path, which ensures that the file is contained within the root folder.
 </p>
 
-<sample src="examples/TaintedPath.js" />
+<sample src="examples/TaintedPathGood.js" />
+
 </example>
 
 <references>

javascript/ql/src/Security/CWE-022/examples/TaintedPath.js

@@ -1,13 +1,12 @@
-var fs = require('fs'),
-    http = require('http'),
-    url = require('url');
+const fs = require('fs'),
+      http = require('http'),
+      url = require('url');
+
+const ROOT = "/var/www/";
 
 var server = http.createServer(function(req, res) {
-  let path = url.parse(req.url, true).query.path;
+  let filePath = url.parse(req.url, true).query.path;
 
   // BAD: This could read any file on the file system
-  res.write(fs.readFileSync(path));
-
-  // BAD: This could still read any file on the file system
-  res.write(fs.readFileSync("/home/user/" + path));
-});
+  res.write(fs.readFileSync(ROOT + filePath, 'utf8'));
+});

javascript/ql/src/Security/CWE-022/examples/TaintedPathGood.js

@@ -0,0 +1,19 @@
+const fs = require('fs'),
+      http = require('http'),
+      path = require('path'),
+      url = require('url');
+
+const ROOT = "/var/www/";
+
+var server = http.createServer(function(req, res) {
+  let filePath = url.parse(req.url, true).query.path;
+
+  // GOOD: Verify that the file path is under the root directory
+  filePath = fs.realpathSync(path.resolve(ROOT, filePath));
+  if (!filePath.startsWith(ROOT)) {
+    res.statusCode = 403;
+    res.end();
+    return;
+  }
+  res.write(fs.readFileSync(filePath, 'utf8'));
+});