Address suggestions from review.

Author: Max Schaefer

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

@@ -8,22 +8,28 @@ can result in sensitive information being revealed or deleted, or an attacker be
 behavior by modifying unexpected files.</p>
 
 <p>Paths that are naively constructed from data controlled by a user may contain unexpected special characters,
-such as "..". Such a path may potentially point to any directory on the file system.</p>
+such as "..". Such a path may potentially point anywhere on the file system.</p>
 
 </overview>
 <recommendation>
 
 <p>Validate user input before using it to construct a file path.</p>
 
-<p>The choice of validation depends on the use case.</p>
+<p>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>
 
-<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, for example by checking that the path starts with the root folder.
-Additionally, you need to ensure that the path does not contain any ".." components, since these would allow
-the path to escape the root folder.</p>
+<p>In the former case, a common strategy is to make sure that the constructed file path is contained within
+a safe root folder, for example by checking that the path starts with the root folder. Additionally,
+you need to ensure that the path does not contain any ".." components, since otherwise
+even a path that starts with the root folder could be used to access files outside the root folder.</p>
 
-<p>A safer (but more restrictive) option is to use an allow list of safe paths and make sure that
-the user input is one of those paths.</p>
+<p>In the latter case, if you want to ensure that the user input is interpreted as a simple filename without
+a path component, you can remove all path separators ("/" or "\") and all ".." sequences from the input
+before using it to construct a 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>
@@ -34,12 +40,9 @@ such as "/etc/passwd".</p>
 
 <sample src="TaintedPath.java" />
 
-<p>Simply checking that the path is under a trusted location (such as the user's home directory) is not enough,
-however, since the path could contain relative components such as "..". For example, the string
-"/home/[user]/../../etc/passwd" starts with the user's home directory, but would still result in the code reading
-the file located at "/etc/passwd".</p>
-
-<p>To fix this, we check that the user-provided path does not contain ".." and starts with the user's home directory.</p>
+<p>Simply checking that the path is under a trusted location (such as a known public folder) is not enough,
+however, since the path could contain relative components such as "..". To fix this, we check that the it does
+not contain ".." and starts with the public folder.</p>
 
 <sample src="TaintedPathGood.java" />
 

java/ql/src/Security/CWE/CWE-022/TaintedPathGood.java

@@ -3,8 +3,8 @@ public void sendUserFileGood(Socket sock, String user) {
 			new InputStreamReader(sock.getInputStream(), "UTF-8"));
 	String filename = filenameReader.readLine();
 	// GOOD: ensure that the file is in a designated folder in the user's home directory
-	if (!filePath.contains("..") && filePath.startsWith("/home/" + user + "/public/")) {
-		BufferedReader fileReader = new BufferedReader(new FileReader(filePath));
+	if (!filename.contains("..") && filename.startsWith("/home/" + user + "/public/")) {
+		BufferedReader fileReader = new BufferedReader(new FileReader(filename));
 		String fileLine = fileReader.readLine();
 		while(fileLine != null) {
 			sock.getOutputStream().write(fileLine.getBytes());

java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java

@@ -21,10 +21,10 @@ public void sendUserFile(Socket sock, String user) throws IOException {
 
     public void sendUserFileGood(Socket sock, String user) throws IOException {
         BufferedReader filenameReader = new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8"));
-        String filePath = filenameReader.readLine();
+        String filename = filenameReader.readLine();
         // GOOD: ensure that the file is in a designated folder in the user's home directory
-        if (!filePath.contains("..") && filePath.startsWith("/home/" + user + "/public/")) {
-            BufferedReader fileReader = new BufferedReader(new FileReader(filePath));
+        if (!filename.contains("..") && filename.startsWith("/home/" + user + "/public/")) {
+            BufferedReader fileReader = new BufferedReader(new FileReader(filename));
             String fileLine = fileReader.readLine();
             while(fileLine != null) {
                 sock.getOutputStream().write(fileLine.getBytes());