Update qhelp for py/path-injection.

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

@@ -13,40 +13,43 @@ 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 paths constructed from untrusted user input before using them to access files.
+</p>
+
+<p>The choice of validation depends on the use case.</p>
+
+<p>
+If you want to allow paths spanning multiple folders, 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>
-Ideally, follow these rules:
+More restrictive options include using a library like the <code>sanitize-filename</code> npm package to
+eliminate any special characters from the file path, or restricting the path to an allow list of safe paths.
+These options are safe, but can only be used in particular circumstances.
 </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>
 </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>.
-</p>
-
-<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.
+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, the file name is resolved relative to a root folder, which has the side effect
+of normalizing the path and removing any "../" segments. We then 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/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'));
+});

javascript/ql/src/Security/CWE-327/tst.ql

@@ -0,0 +1,3 @@
+from int i, int s
+where i in [0..10] and s = sum([i, i+1]) and i % 2 = 0
+select s

javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected

@@ -1535,6 +1535,76 @@ nodes
 | TaintedPath.js:214:35:214:38 | path |
 | TaintedPath.js:214:35:214:38 | path |
 | TaintedPath.js:214:35:214:38 | path |
+| examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:28:8:34 | req.url |
+| examples/TaintedPath.js:8:28:8:34 | req.url |
+| examples/TaintedPath.js:8:28:8:34 | req.url |
+| examples/TaintedPath.js:8:28:8:34 | req.url |
+| examples/TaintedPath.js:8:28:8:34 | req.url |
+| examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath |
 | express.js:8:20:8:32 | req.query.bar |
 | express.js:8:20:8:32 | req.query.bar |
 | express.js:8:20:8:32 | req.query.bar |
@@ -6635,6 +6705,102 @@ edges
 | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
 | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
 | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:211:14:211:37 | url.par ... , true) |
+| examples/TaintedPath.js:8:7:8:52 | filePath | examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath | examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath | examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath | examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath | examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath | examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath | examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath | examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath | examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath | examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath | examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:8:7:8:52 | filePath | examples/TaintedPath.js:11:36:11:43 | filePath |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) | examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) | examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) | examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) | examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) | examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) | examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) | examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) | examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) | examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) | examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) | examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:41 | url.par ... , true) | examples/TaintedPath.js:8:18:8:47 | url.par ... ).query |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query | examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query | examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query | examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query | examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query | examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query | examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query | examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query | examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query | examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query | examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query | examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:47 | url.par ... ).query | examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path | examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path | examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path | examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path | examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path | examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path | examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path | examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path | examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path | examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path | examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path | examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:18:8:52 | url.par ... ry.path | examples/TaintedPath.js:8:7:8:52 | filePath |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:8:18:8:41 | url.par ... , true) |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
+| examples/TaintedPath.js:11:36:11:43 | filePath | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath |
 | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar |
 | handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
 | handlebars.js:10:51:10:58 | filePath | handlebars.js:11:32:11:39 | filePath |
@@ -10345,6 +10511,7 @@ edges
 | TaintedPath.js:212:31:212:34 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:212:31:212:34 | path | This path depends on a $@. | TaintedPath.js:211:24:211:30 | req.url | user-provided value |
 | TaintedPath.js:213:45:213:48 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:213:45:213:48 | path | This path depends on a $@. | TaintedPath.js:211:24:211:30 | req.url | user-provided value |
 | TaintedPath.js:214:35:214:38 | path | TaintedPath.js:211:24:211:30 | req.url | TaintedPath.js:214:35:214:38 | path | This path depends on a $@. | TaintedPath.js:211:24:211:30 | req.url | user-provided value |
+| examples/TaintedPath.js:11:29:11:43 | ROOT + filePath | examples/TaintedPath.js:8:28:8:34 | req.url | examples/TaintedPath.js:11:29:11:43 | ROOT + filePath | This path depends on a $@. | examples/TaintedPath.js:8:28:8:34 | req.url | user-provided value |
 | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar | express.js:8:20:8:32 | req.query.bar | This path depends on a $@. | express.js:8:20:8:32 | req.query.bar | user-provided value |
 | handlebars.js:11:32:11:39 | filePath | handlebars.js:29:46:29:60 | req.params.path | handlebars.js:11:32:11:39 | filePath | This path depends on a $@. | handlebars.js:29:46:29:60 | req.params.path | user-provided value |
 | handlebars.js:15:25:15:32 | filePath | handlebars.js:43:15:43:29 | req.params.path | handlebars.js:15:25:15:32 | filePath | This path depends on a $@. | handlebars.js:43:15:43:29 | req.params.path | user-provided value |

javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/examples/TaintedPath.js

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

javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/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'));
+});

python/ql/src/Security/CWE-022/PathInjection.qhelp

@@ -13,21 +13,24 @@ 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 function
-like <code>werkzeug.utils.secure_filename</code>, or by performing custom validation.
+Validate paths constructed from untrusted user input before using them to access files.
+</p>
+
+<p>The choice of validation depends on the use case.</p>
+
+<p>
+If you want to allow paths spanning multiple folders, 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>os.path.normpath</code>
+or <code>os.path.realpath</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>
-Ideally, follow these rules:
+More restrictive options include using a library function like <code>werkzeug.utils.secure_filename</code> to
+eliminate any special characters from the file path, or restricting the path to an allow list of safe paths.
+These options are safe, but can only be used in particular circumstances.
 </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 an allowlist of known good patterns.</li>
-</ul>
 </recommendation>
 
 <example>