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

@@ -13,27 +13,36 @@ such as "..". Such a path may potentially point to any directory on the file sys
 </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 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, 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>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>
 
 </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 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>
+
+<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 (!filePath.contains("..") && filePath.startsWith("/home/" + user + "/public/")) {
+		BufferedReader fileReader = new BufferedReader(new FileReader(filePath));
+		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 filePath = 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));
+            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);
+		}
+	}
 }