Merge pull request #15409 from erik-krogh/path-java

Java: Improve the QHelp for `java/path-injection`.

Author: atorralba

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 may contain
+unexpected special characters such as "..". Such a path could 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 checking that the path is contained within a safe folder. The method you should use depends 
+on how the path is used in the application, and whether the path should 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 should 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,22 @@ 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="TaintedPath.java" />
+<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 should only be a file name, you can check that it doesn't contain any path separators or ".." sequences.
+</p>
 
-<sample src="TaintedPathGood.java" />
+<sample src="examples/TaintedPathGoodNormalize.java" />
+
+<p>
+If the input should be 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/TaintedPathGood.java

@@ -45,12 +45,12 @@ the result is within the destination directory. If provided with a zip file cont
 path like <code>..\sneaky-file</code>, then this file would be written outside the destination
 directory.</p>
 
-<sample src="ZipSlipBad.java" />
+<sample src="examples/ZipSlipBad.java" />
 
 <p>To fix this vulnerability, we need to verify that the normalized <code>file</code> still has
 <code>destinationDir</code> as its prefix, and throw an exception if this is not the case.</p>
 
-<sample src="ZipSlipGood.java" />
+<sample src="examples/ZipSlipGood.java" />
 
 </example>
 <references>

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

@@ -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 + File.separator)) {
+		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/TaintedPath.java renamed to java/ql/src/Security/CWE/CWE-022/examples/TaintedPath.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();
+	}	
+}

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

@@ -1,9 +1,14 @@
 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 |
+| TaintedPath.java:12:38:12:110 | new BufferedReader(...) : BufferedReader | TaintedPath.java:13:24:13:37 | filenameReader : BufferedReader |
+| TaintedPath.java:12:57:12:109 | new InputStreamReader(...) : InputStreamReader | TaintedPath.java:12:38:12:110 | new BufferedReader(...) : BufferedReader |
+| TaintedPath.java:12:79:12:99 | getInputStream(...) : InputStream | TaintedPath.java:12:57:12:109 | new InputStreamReader(...) : InputStreamReader |
+| TaintedPath.java:13:24:13:37 | filenameReader : BufferedReader | TaintedPath.java:13:24:13:48 | readLine(...) : String |
+| TaintedPath.java:13:24:13:48 | readLine(...) : String | TaintedPath.java:15:68:15:75 | filename |
+| TaintedPath.java:38:41:39:70 | new BufferedReader(...) : BufferedReader | TaintedPath.java:40:27:40:40 | filenameReader : BufferedReader |
+| TaintedPath.java:39:17:39:69 | new InputStreamReader(...) : InputStreamReader | TaintedPath.java:38:41:39:70 | new BufferedReader(...) : BufferedReader |
+| TaintedPath.java:39:39:39:59 | getInputStream(...) : InputStream | TaintedPath.java:39:17:39:69 | new InputStreamReader(...) : InputStreamReader |
+| TaintedPath.java:40:27:40:40 | filenameReader : BufferedReader | TaintedPath.java:40:27:40:51 | readLine(...) : String |
+| TaintedPath.java:40:27:40:51 | readLine(...) : String | TaintedPath.java:43:46:43:53 | 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 |
@@ -189,12 +194,18 @@ 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 |
+| TaintedPath.java:12:38:12:110 | new BufferedReader(...) : BufferedReader | semmle.label | new BufferedReader(...) : BufferedReader |
+| TaintedPath.java:12:57:12:109 | new InputStreamReader(...) : InputStreamReader | semmle.label | new InputStreamReader(...) : InputStreamReader |
+| TaintedPath.java:12:79:12:99 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
+| TaintedPath.java:13:24:13:37 | filenameReader : BufferedReader | semmle.label | filenameReader : BufferedReader |
+| TaintedPath.java:13:24:13:48 | readLine(...) : String | semmle.label | readLine(...) : String |
+| TaintedPath.java:15:68:15:75 | filename | semmle.label | filename |
+| TaintedPath.java:38:41:39:70 | new BufferedReader(...) : BufferedReader | semmle.label | new BufferedReader(...) : BufferedReader |
+| TaintedPath.java:39:17:39:69 | new InputStreamReader(...) : InputStreamReader | semmle.label | new InputStreamReader(...) : InputStreamReader |
+| TaintedPath.java:39:39:39:59 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
+| TaintedPath.java:40:27:40:40 | filenameReader : BufferedReader | semmle.label | filenameReader : BufferedReader |
+| TaintedPath.java:40:27:40:51 | readLine(...) : String | semmle.label | readLine(...) : String |
+| TaintedPath.java:43:46:43:53 | 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 |
@@ -386,7 +397,8 @@ 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 |
+| TaintedPath.java:15:53:15:76 | new FileReader(...) | TaintedPath.java:12:79:12:99 | getInputStream(...) : InputStream | TaintedPath.java:15:68:15:75 | filename | This path depends on a $@. | TaintedPath.java:12:79:12:99 | getInputStream(...) | user-provided value |
+| TaintedPath.java:43:25:43:54 | resolve(...) | TaintedPath.java:39:39:39:59 | getInputStream(...) : InputStream | TaintedPath.java:43:46:43:53 | filename | This path depends on a $@. | TaintedPath.java:39:39:39:59 | 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/src/Security/CWE/CWE-022/examples/TaintedPathGoodNormalize.java

@@ -1,10 +1,11 @@
 import java.io.BufferedReader;
+import java.io.File;
 import java.io.FileReader;
+import java.io.IOException;
 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 {
@@ -32,4 +33,40 @@ public void sendUserFileGood(Socket sock, String user) throws IOException {
             }
         }
     }
+
+    public void sendUserFileGood2(Socket sock, String user) throws Exception {
+        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(); // FP until the path-injection sinks are reworked
+    
+        // GOOD: ensure that the path stays within the public folder
+        if (!filePath.startsWith(publicFolder + File.separator)) {
+            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();
+        }
+    }
+
+    public void sendUserFileGood3(Socket sock, String user) throws Exception {
+        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();
+        }	
+    }
 }