Merge pull request #14793 from github/max-schaefer/tainted-path-qhelp

Java: Improve QHelp for `java/path-injection` to mention less disruptive fixes.

Author: max-schaefer

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

@@ -2,23 +2,11 @@ public void sendUserFile(Socket sock, String user) {
 	BufferedReader filenameReader = new BufferedReader(
 			new InputStreamReader(sock.getInputStream(), "UTF-8"));
 	String filename = filenameReader.readLine();
-	// BAD: read from a file using a path controlled by the user
-	BufferedReader fileReader = new BufferedReader(
-			new FileReader("/home/" + user + "/" + filename));
+	// BAD: read from a file without checking its path
+	BufferedReader fileReader = new BufferedReader(new FileReader(filename));
 	String fileLine = fileReader.readLine();
 	while(fileLine != null) {
 		sock.getOutputStream().write(fileLine.getBytes());
 		fileLine = fileReader.readLine();
 	}
 }
-
-public void sendUserFileFixed(Socket sock, String user) {
-	// ...
-	
-	// GOOD: remove all dots and directory delimiters from the filename before using
-	String filename = filenameReader.readLine().replaceAll("\\.", "").replaceAll("/", "");
-	BufferedReader fileReader = new BufferedReader(
-			new FileReader("/home/" + user + "/" + filename));
-
-	// ...
-}

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

@@ -8,32 +8,44 @@ 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. Ideally, follow these rules:</p>
+<p>Validate user input before using it to construct a file path.</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>Ideally use a whitelist of known good patterns.</li>
-</ul>
+<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>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>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>
 
-<p>In this example, a file name is read from a <code>java.net.Socket</code> and then used to access a file in the
-user's home directory and send it back over the socket. However, a malicious user could enter a file name which contains special 
-characters. For example, the string "../../etc/passwd" will result in the code reading the file located at 
-"/home/[user]/../../etc/passwd", 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.</p>
+<p>In this example, a file name is read from a <code>java.net.Socket</code> and then used to access a file
+and send it back over the socket. However, a malicious user could enter a file name anywhere on the file system,
+such as "/etc/passwd".</p>
 
 <sample src="TaintedPath.java" />
 
+<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, check that it does
+not contain ".." and starts with the public folder.</p>
+
+<sample src="TaintedPathGood.java" />
+
 </example>
 <references>
 

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

@@ -0,0 +1,14 @@
+public void sendUserFileGood(Socket sock, String user) {
+	BufferedReader filenameReader = new BufferedReader(
+			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 (!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());
+			fileLine = fileReader.readLine();
+		}
+	}	
+}

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

@@ -1,4 +1,9 @@
 edges
+| TaintedPath.java:11:38:11:110 | new BufferedReader(...) : BufferedReader | TaintedPath.java:12:24:12:37 | filenameReader : BufferedReader |
+| TaintedPath.java:11:57:11:109 | new InputStreamReader(...) : InputStreamReader | TaintedPath.java:11:38:11:110 | new BufferedReader(...) : BufferedReader |
+| TaintedPath.java:11:79:11:99 | getInputStream(...) : InputStream | TaintedPath.java:11:57:11:109 | new InputStreamReader(...) : InputStreamReader |
+| TaintedPath.java:12:24:12:37 | filenameReader : BufferedReader | TaintedPath.java:12:24:12:48 | readLine(...) : String |
+| TaintedPath.java:12:24:12:48 | readLine(...) : String | TaintedPath.java:14:68:14:75 | filename |
 | Test.java:19:18:19:38 | getHostName(...) : String | Test.java:24:20:24:23 | temp |
 | Test.java:19:18:19:38 | getHostName(...) : String | Test.java:27:21:27:24 | temp |
 | Test.java:19:18:19:38 | getHostName(...) : String | Test.java:30:44:30:47 | temp |
@@ -184,6 +189,12 @@ edges
 | mad/Test.java:221:26:221:33 | source(...) : String | mad/Test.java:221:19:221:33 | (...)... |
 | mad/Test.java:226:29:226:36 | source(...) : String | mad/Test.java:226:20:226:36 | (...)... |
 nodes
+| TaintedPath.java:11:38:11:110 | new BufferedReader(...) : BufferedReader | semmle.label | new BufferedReader(...) : BufferedReader |
+| TaintedPath.java:11:57:11:109 | new InputStreamReader(...) : InputStreamReader | semmle.label | new InputStreamReader(...) : InputStreamReader |
+| TaintedPath.java:11:79:11:99 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
+| TaintedPath.java:12:24:12:37 | filenameReader : BufferedReader | semmle.label | filenameReader : BufferedReader |
+| TaintedPath.java:12:24:12:48 | readLine(...) : String | semmle.label | readLine(...) : String |
+| TaintedPath.java:14:68:14:75 | filename | semmle.label | filename |
 | Test.java:19:18:19:38 | getHostName(...) : String | semmle.label | getHostName(...) : String |
 | Test.java:24:20:24:23 | temp | semmle.label | temp |
 | Test.java:27:21:27:24 | temp | semmle.label | temp |
@@ -375,6 +386,7 @@ nodes
 | mad/Test.java:226:29:226:36 | source(...) : String | semmle.label | source(...) : String |
 subpaths
 #select
+| TaintedPath.java:14:53:14:76 | new FileReader(...) | TaintedPath.java:11:79:11:99 | getInputStream(...) : InputStream | TaintedPath.java:14:68:14:75 | filename | This path depends on a $@. | TaintedPath.java:11:79:11:99 | getInputStream(...) | user-provided value |
 | Test.java:24:11:24:24 | new File(...) | Test.java:19:18:19:38 | getHostName(...) : String | Test.java:24:20:24:23 | temp | This path depends on a $@. | Test.java:19:18:19:38 | getHostName(...) | user-provided value |
 | Test.java:27:11:27:25 | get(...) | Test.java:19:18:19:38 | getHostName(...) : String | Test.java:27:21:27:24 | temp | This path depends on a $@. | Test.java:19:18:19:38 | getHostName(...) | user-provided value |
 | Test.java:30:11:30:48 | getPath(...) | Test.java:19:18:19:38 | getHostName(...) : String | Test.java:30:44:30:47 | temp | This path depends on a $@. | Test.java:19:18:19:38 | getHostName(...) | user-provided value |

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

@@ -0,0 +1,35 @@
+import java.io.BufferedReader;
+import java.io.FileReader;
+import java.io.InputStreamReader;
+import java.net.Socket;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.io.IOException;
+
+public class TaintedPath {
+    public void sendUserFile(Socket sock, String user) throws IOException {
+	    BufferedReader filenameReader = new BufferedReader(new InputStreamReader(sock.getInputStream(), "UTF-8"));
+	    String filename = filenameReader.readLine();
+	    // BAD: read from a file without checking its path
+    	BufferedReader fileReader = new BufferedReader(new FileReader(filename));
+        String fileLine = fileReader.readLine();
+        while(fileLine != null) {
+        	sock.getOutputStream().write(fileLine.getBytes());
+        	fileLine = fileReader.readLine();
+        }
+    }
+
+    public void sendUserFileGood(Socket sock, String user) throws IOException {
+        BufferedReader filenameReader = new BufferedReader(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 (!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());
+                fileLine = fileReader.readLine();
+            }
+        }
+    }
+}

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

@@ -101,4 +101,12 @@ void doGet5(InetAddress address)
 		new File(new URI(null, null, null, 0, t, null, null));
 	}
 
+	void doGet6(String root, InetAddress address)
+	throws IOException{
+		String temp = address.getHostName();
+		// GOOD: Use `contains` and `startsWith` to check if the path is safe
+		if (!temp.contains("..") && temp.startsWith(root + "/")) {
+			File file = new File(temp);
+		}
+	}
 }