New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Java: Improve QHelp for java/path-injection to mention less disruptive fixes.
#14793
Conversation
|
QHelp previews: java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelpUncontrolled data used in path expressionAccessing 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. RecommendationValidate 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. ExampleIn this example, a file name is read from a 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. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for improving our documentation! The suggested changes LGTM, except a comment about tests.
I reckon that the new TaintedPath.java test file is there to test the QHelp recommendations, but have you checked whether the test cases you added aren't already covered by the path sanitizer tests? I'd say they are, but maybe I'm missing something.
I didn't check that. I thought it might be a good idea to include the examples from the query help as verbatim as possible, just to make sure they actually behave the way we expect. If you are concerned about duplication I am, of course, happy to remove the new test. |
|
Well, now that I think of it, although they add a bit of duplication, these tests validate that we are correctly using the path sanitizer library in this query, so I'd say they still add value. Let me request a docs review for the query help changes before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Generally this looks fine but I just had some questions/suggestions—hoping I've not misunderstood 🙏
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have misunderstood but I can't see an example for this option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct; I should put it back.
| ".../...//" 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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement makes sense by itself but the wording around the two options doesn't seem to relate to different use cases. If it's a case of one being safer than the other, but more restrictive, would it make sense to add a sentence to that effect around line 25?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a little bit unsure about how to phrase this. It's not necessarily the case that the second option is safer than the first. If implemented properly, it's just as safe, though it's mildly more complex. In fact, I think it's the better option overall, but I didn't want to completely remove any mention of the other option (which is what was there before).
What I was gesturing towards here (but not clearly enough) is that if you want the user to be able to specify paths with multiple components (i.e., across multiple folders), then you have to use the first option. If you only allow the user to select the filename (but not the entire path), then the second option is simpler.
Let me take another crack at this to see if I can find a better wording.
| <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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just wondering if this text is essential, as lines 22/23 seem to cover the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we can remove that.
|
OK, I've attempted to address the feedback, in two separate commits. The second commit puts back the original example (more or less) about eliminating special characters, but unfortunately I found that CodeQL still flags the fixed code in this case. @atorralba, could you perhaps take a look at this code to see what I'm doing wrong? (I did try simplifying and breaking up the regex replacement on the previous line, but that didn't help.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing those comments so quickly, @max-schaefer! I just left two tiny additional ones, otherwise LGTM 🎉
Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>
143e168
I think we should handle this in a different PR. I'd recommend either accepting the test results as they are for now, or removing the test case temporarily, and then we can create a specific PR to handle that test case + the addition of the missing sanitizer. |
This reverts commit 947b094.
👍 I've gone with the latter option. |
The suggested fix at the moment (ensure the path contains no file separators or metacharacters) is certainly safe, but not applicable in situations where we want to allow paths spanning across multiple folders as long as they stay within a safe root. The query already has extensive modelling for how to safely support this, but the query help didn't mention it.