iterate on the java/path-injection qhelp

Author: erik-krogh

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

@@ -1,23 +1,23 @@
 # Uncontrolled data used in path expression
 Accessing paths controlled by users can allow an attacker to access unexpected resources. This can result in sensitive information being revealed or deleted, or an attacker being able to influence behavior by modifying unexpected files.
 
-Paths that are naively constructed from data controlled by a user may contain unexpected special characters, such as "..". Such a path may potentially point anywhere on the file system.
+Paths that are naively constructed from data controlled by a user may be absolute paths or contain unexpected special characters, such as "..". Such a path may potentially point anywhere on the file system.
 
 
 ## Recommendation
 Validate user input before using it to construct a file path.
 
-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.
+Common validation methods include checking that the normalized path is relative and does not contain any ".." components, or that the path is contained within a safe folder. The validation method to use depends on how the path is used in the application and whether the path is supposed to be a single path component.
 
-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.
+If the path is supposed to be a single path component (such as a file name) you can check for the existence of any path separators ("/" or "\\") or ".." sequences in the input, and reject the input if any are found.
 
-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 *not* sufficient to only remove "../" sequences: for example, applying this filter to ".../...//" would still result in the string "../".
+Note that removing "../" sequences is *not* sufficient, since the input could still contain a path separator followed by "..". For example, the input ".../...//" would still result in the string "../" if only "../" sequences are removed.
 
 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.
 
 
 ## Example
-In this example, a file name is read from a `java.net.Socket` 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".
+In this example, a file name is read from a `java.net.Socket` 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" or "../../../etc/passwd".
 
 
 ```java
@@ -35,25 +35,50 @@ public void sendUserFile(Socket sock, String user) {
 }
 
 ```
-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.
+If the input is just supposed to be a file name, you can check that it doesn't contain any path separators or ".." sequences.
 
 
 ```java
 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();
-		}
+	// GOOD: ensure that the filename has no path separators or parent directory references
+	if (filename.contains("..") || filename.contains("/") || filename.contains("\\")) {
+		throw new IllegalArgumentException("Invalid filename");
+	}
+	BufferedReader fileReader = new BufferedReader(new FileReader(filename));
+	String fileLine = fileReader.readLine();
+	while(fileLine != null) {
+		sock.getOutputStream().write(fileLine.getBytes());
+		fileLine = fileReader.readLine();
 	}	
 }
 
+```
+If the input is supposed to be found within a specific directory, you can check that the resolved path is still contained within that directory.
+
+
+```java
+public void sendUserFileGood(Socket sock, String user) {
+	BufferedReader filenameReader = new BufferedReader(
+			new InputStreamReader(sock.getInputStream(), "UTF-8"));
+	String filename = filenameReader.readLine();
+
+	Path publicFolder = Paths.get("/home/" + user + "/public").normalize().toAbsolutePath();
+	Path filePath = publicFolder.resolve(filename).normalize().toAbsolutePath();
+
+	// GOOD: ensure that the path stays within the public folder
+	if (!filePath.startsWith(publicFolder)) {
+		throw new IllegalArgumentException("Invalid filename");
+	}
+	BufferedReader fileReader = new BufferedReader(new FileReader(filePath.toString()));
+	String fileLine = fileReader.readLine();
+	while(fileLine != null) {
+		sock.getOutputStream().write(fileLine.getBytes());
+		fileLine = fileReader.readLine();
+	}
+}
 ```
 
 ## References

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

@@ -7,26 +7,28 @@
 can result in sensitive information being revealed or deleted, or an attacker being able to influence
 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 anywhere on the file system.</p>
+<p>Paths that are naively constructed from data controlled by a user may be absolute paths or contain
+unexpected special characters, 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 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>Common validation methods include checking that the normalized path is relative and does not contain
+any ".." components, or that the path is contained within a safe folder. The validation method to use depends
+on how the path is used in the application and whether the path is supposed to be a single 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>If the path is supposed to be a single path component (such as a file name) you can check for the existence
+of any path separators ("/" or "\") or ".." sequences in the input, and reject the input if any are found.
+</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>
+Note that removing "../" sequences is <i>not</i> sufficient, since the input could still contain a path separator
+followed by "..". For example, the input ".../...//" would still result in the string "../" if only "../" sequences
+are removed.
+</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>
@@ -36,15 +38,23 @@ the user input matches one of these patterns.</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>
+such as "/etc/passwd" or "../../../etc/passwd".</p>
 
 <sample src="examples/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>
+<p>
+If the input is just supposed to be a file name, you can check that it doesn't contain any path separators
+or ".." sequences.
+</p>
 
-<sample src="examples/TaintedPathGood.java" />
+<sample src="examples/TaintedPathGoodNormalize.java" />
+
+<p>
+If the input is supposed to be found within a specific directory, you can check that the resolved path 
+is still contained within that directory.
+</p>
+
+<sample src="examples/TaintedPathGoodFolder.java" />
 
 </example>
 <references>

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

@@ -0,0 +1,19 @@
+public void sendUserFileGood(Socket sock, String user) {
+	BufferedReader filenameReader = new BufferedReader(
+			new InputStreamReader(sock.getInputStream(), "UTF-8"));
+	String filename = filenameReader.readLine();
+
+	Path publicFolder = Paths.get("/home/" + user + "/public").normalize().toAbsolutePath();
+	Path filePath = publicFolder.resolve(filename).normalize().toAbsolutePath();
+
+	// GOOD: ensure that the path stays within the public folder
+	if (!filePath.startsWith(publicFolder)) {
+		throw new IllegalArgumentException("Invalid filename");
+	}
+	BufferedReader fileReader = new BufferedReader(new FileReader(filePath.toString()));
+	String fileLine = fileReader.readLine();
+	while(fileLine != null) {
+		sock.getOutputStream().write(fileLine.getBytes());
+		fileLine = fileReader.readLine();
+	}
+}

java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGoodFolder.java

@@ -0,0 +1,15 @@
+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 filename has no path separators or parent directory references
+	if (filename.contains("..") || filename.contains("/") || filename.contains("\\")) {
+		throw new IllegalArgumentException("Invalid filename");
+	}
+	BufferedReader fileReader = new BufferedReader(new FileReader(filename));
+	String fileLine = fileReader.readLine();
+	while(fileLine != null) {
+		sock.getOutputStream().write(fileLine.getBytes());
+		fileLine = fileReader.readLine();
+	}	
+}