From 6b66f5cbc51049589371e04ec4b107b99ae4c2ac Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Thu, 11 Jan 2024 11:00:09 +0100 Subject: [PATCH 1/8] check in the TaintedPath qhelp as markdown to get pretty diffs --- .../src/Security/CWE/CWE-022/TaintedPath.md | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 java/ql/src/Security/CWE/CWE-022/TaintedPath.md diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPath.md b/java/ql/src/Security/CWE/CWE-022/TaintedPath.md new file mode 100644 index 000000000000..f1ffb2c85276 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-022/TaintedPath.md @@ -0,0 +1,64 @@ +# 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. + + +## 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. + +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. + +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 "../". + +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". + + +```java +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 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(); + } +} + +``` +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. + + +```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(); + } + } +} + +``` + +## References +* OWASP: [Path Traversal](https://owasp.org/www-community/attacks/Path_Traversal). +* Common Weakness Enumeration: [CWE-22](https://cwe.mitre.org/data/definitions/22.html). +* Common Weakness Enumeration: [CWE-23](https://cwe.mitre.org/data/definitions/23.html). +* Common Weakness Enumeration: [CWE-36](https://cwe.mitre.org/data/definitions/36.html). +* Common Weakness Enumeration: [CWE-73](https://cwe.mitre.org/data/definitions/73.html). From 4958c19c67f2ed9eda3235d1598b7f85314be71f Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Thu, 11 Jan 2024 12:44:49 +0100 Subject: [PATCH 2/8] move the examples for the qhelps into an `example/` folder --- java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp | 4 ++-- java/ql/src/Security/CWE/CWE-022/ZipSlip.qhelp | 4 ++-- .../src/Security/CWE/CWE-022/{ => examples}/TaintedPath.java | 0 .../Security/CWE/CWE-022/{ => examples}/TaintedPathGood.java | 0 .../src/Security/CWE/CWE-022/{ => examples}/ZipSlipBad.java | 0 .../src/Security/CWE/CWE-022/{ => examples}/ZipSlipGood.java | 0 6 files changed, 4 insertions(+), 4 deletions(-) rename java/ql/src/Security/CWE/CWE-022/{ => examples}/TaintedPath.java (100%) rename java/ql/src/Security/CWE/CWE-022/{ => examples}/TaintedPathGood.java (100%) rename java/ql/src/Security/CWE/CWE-022/{ => examples}/ZipSlipBad.java (100%) rename java/ql/src/Security/CWE/CWE-022/{ => examples}/ZipSlipGood.java (100%) diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp b/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp index a68bdad34413..e84efb00b91a 100644 --- a/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp +++ b/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp @@ -38,13 +38,13 @@ the user input matches one of these patterns.

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".

- +

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.

- + diff --git a/java/ql/src/Security/CWE/CWE-022/ZipSlip.qhelp b/java/ql/src/Security/CWE/CWE-022/ZipSlip.qhelp index 2caf0ccc8b0f..885e7ac946a8 100644 --- a/java/ql/src/Security/CWE/CWE-022/ZipSlip.qhelp +++ b/java/ql/src/Security/CWE/CWE-022/ZipSlip.qhelp @@ -45,12 +45,12 @@ the result is within the destination directory. If provided with a zip file cont path like ..\sneaky-file, then this file would be written outside the destination directory.

- +

To fix this vulnerability, we need to verify that the normalized file still has destinationDir as its prefix, and throw an exception if this is not the case.

- + diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPath.java b/java/ql/src/Security/CWE/CWE-022/examples/TaintedPath.java similarity index 100% rename from java/ql/src/Security/CWE/CWE-022/TaintedPath.java rename to java/ql/src/Security/CWE/CWE-022/examples/TaintedPath.java diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPathGood.java b/java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGood.java similarity index 100% rename from java/ql/src/Security/CWE/CWE-022/TaintedPathGood.java rename to java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGood.java diff --git a/java/ql/src/Security/CWE/CWE-022/ZipSlipBad.java b/java/ql/src/Security/CWE/CWE-022/examples/ZipSlipBad.java similarity index 100% rename from java/ql/src/Security/CWE/CWE-022/ZipSlipBad.java rename to java/ql/src/Security/CWE/CWE-022/examples/ZipSlipBad.java diff --git a/java/ql/src/Security/CWE/CWE-022/ZipSlipGood.java b/java/ql/src/Security/CWE/CWE-022/examples/ZipSlipGood.java similarity index 100% rename from java/ql/src/Security/CWE/CWE-022/ZipSlipGood.java rename to java/ql/src/Security/CWE/CWE-022/examples/ZipSlipGood.java From 57e0b3cceb68646995610125634aaf81b0ad2af0 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Fri, 12 Jan 2024 09:28:44 +0100 Subject: [PATCH 3/8] iterate on the java/path-injection qhelp --- .../src/Security/CWE/CWE-022/TaintedPath.md | 53 ++++++++++++++----- .../Security/CWE/CWE-022/TaintedPath.qhelp | 44 +++++++++------ .../CWE/CWE-022/examples/TaintedPathGood.java | 14 ----- .../examples/TaintedPathGoodFolder.java | 19 +++++++ .../examples/TaintedPathGoodNormalize.java | 15 ++++++ 5 files changed, 100 insertions(+), 45 deletions(-) delete mode 100644 java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGood.java create mode 100644 java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGoodFolder.java create mode 100644 java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGoodNormalize.java diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPath.md b/java/ql/src/Security/CWE/CWE-022/TaintedPath.md index f1ffb2c85276..2c851b02efd3 100644 --- a/java/ql/src/Security/CWE/CWE-022/TaintedPath.md +++ b/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,7 +35,7 @@ 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 @@ -43,17 +43,42 @@ 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 diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp b/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp index e84efb00b91a..0be037515850 100644 --- a/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp +++ b/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.

-

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.

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.

@@ -36,15 +38,23 @@ the user input matches one of these patterns.

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".

+such as "/etc/passwd" or "../../../etc/passwd".

-

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. +

- + + +

+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. +

+ + diff --git a/java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGood.java b/java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGood.java deleted file mode 100644 index 37e8be7486c8..000000000000 --- a/java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGood.java +++ /dev/null @@ -1,14 +0,0 @@ -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(); - } - } -} diff --git a/java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGoodFolder.java b/java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGoodFolder.java new file mode 100644 index 000000000000..cd05384b877e --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGoodFolder.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(); + } +} \ No newline at end of file diff --git a/java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGoodNormalize.java b/java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGoodNormalize.java new file mode 100644 index 000000000000..ff0744ccd806 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGoodNormalize.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(); + } +} From 00dadeb3bf6212ed0971d8e7ce0971e109ea4d42 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 23 Jan 2024 12:57:15 +0100 Subject: [PATCH 4/8] delete the markdown file again --- .../src/Security/CWE/CWE-022/TaintedPath.md | 89 ------------------- 1 file changed, 89 deletions(-) delete mode 100644 java/ql/src/Security/CWE/CWE-022/TaintedPath.md diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPath.md b/java/ql/src/Security/CWE/CWE-022/TaintedPath.md deleted file mode 100644 index 2c851b02efd3..000000000000 --- a/java/ql/src/Security/CWE/CWE-022/TaintedPath.md +++ /dev/null @@ -1,89 +0,0 @@ -# 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 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. - -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. - -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. - -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" or "../../../etc/passwd". - - -```java -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 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(); - } -} - -``` -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 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 -* OWASP: [Path Traversal](https://owasp.org/www-community/attacks/Path_Traversal). -* Common Weakness Enumeration: [CWE-22](https://cwe.mitre.org/data/definitions/22.html). -* Common Weakness Enumeration: [CWE-23](https://cwe.mitre.org/data/definitions/23.html). -* Common Weakness Enumeration: [CWE-36](https://cwe.mitre.org/data/definitions/36.html). -* Common Weakness Enumeration: [CWE-73](https://cwe.mitre.org/data/definitions/73.html). From 158ff0da0aff9d20f45e5cfcfa8fe446f00e5548 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 23 Jan 2024 14:46:02 +0100 Subject: [PATCH 5/8] add a trailing slash to the folder check in the QHelp for java/path-injection --- .../Security/CWE/CWE-022/examples/TaintedPathGoodFolder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGoodFolder.java b/java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGoodFolder.java index cd05384b877e..2cc844b0c06e 100644 --- a/java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGoodFolder.java +++ b/java/ql/src/Security/CWE/CWE-022/examples/TaintedPathGoodFolder.java @@ -7,7 +7,7 @@ public void sendUserFileGood(Socket sock, String user) { Path filePath = publicFolder.resolve(filename).normalize().toAbsolutePath(); // GOOD: ensure that the path stays within the public folder - if (!filePath.startsWith(publicFolder)) { + if (!filePath.startsWith(publicFolder + File.separator)) { throw new IllegalArgumentException("Invalid filename"); } BufferedReader fileReader = new BufferedReader(new FileReader(filePath.toString())); From 05a59d2a94d1424c6c0ecb8708efa6bd9eee8c61 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Thu, 25 Jan 2024 11:20:46 +0100 Subject: [PATCH 6/8] apply suggestions from doc review --- .../src/Security/CWE/CWE-022/TaintedPath.qhelp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp b/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp index 0be037515850..0e854370425f 100644 --- a/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp +++ b/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp @@ -7,8 +7,8 @@ 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 be absolute paths or 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 may contain +unexpected special characters such as "..". Such a path could point anywhere on the file system.

@@ -16,12 +16,11 @@ unexpected special characters, such as "..". Such a path may potentially point a

Validate user input before using it to construct a file path.

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. -

+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. -

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. +

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.

@@ -43,14 +42,13 @@ such as "/etc/passwd" or "../../../etc/passwd".

-If the input is just supposed to be a file name, you can check that it doesn't contain any path separators -or ".." sequences. +If the input should only be a file name, you can check that it doesn't contain any path separators or ".." sequences.

-If the input is supposed to be found within a specific directory, you can check that the resolved path +If the input should be within a specific directory, you can check that the resolved path is still contained within that directory.

From 73e3fada44be2685ffe7e0cd5eda6628b8601caa Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Thu, 25 Jan 2024 12:14:10 +0100 Subject: [PATCH 7/8] add missing

--- java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp | 1 + 1 file changed, 1 insertion(+) diff --git a/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp b/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp index 0e854370425f..8fe3dbe3f563 100644 --- a/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp +++ b/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp @@ -18,6 +18,7 @@ unexpected special characters such as "..". Such a path could point anywhere on

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. +

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. From 282632c33b441b59296e302fe77a8d4e3ad8fee9 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 25 Jan 2024 15:11:11 +0100 Subject: [PATCH 8/8] Add new snippets as tests --- .../CWE-022/semmle/tests/TaintedPath.expected | 36 +++++++++++------ .../CWE-022/semmle/tests/TaintedPath.java | 39 ++++++++++++++++++- 2 files changed, 62 insertions(+), 13 deletions(-) diff --git a/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected b/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected index ff2a1aee759c..0e2d90c3709c 100644 --- a/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected +++ b/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected @@ -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 | diff --git a/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java b/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java index 5a5a63ad6adc..a2ae561be588 100644 --- a/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.java +++ b/java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.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(); + } + } }