Skip to content
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

Merged
merged 5 commits into from Nov 16, 2023

Conversation

max-schaefer
Copy link
Collaborator

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.

Copy link
Contributor

github-actions bot commented Nov 15, 2023

QHelp previews:

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

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

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

Copy link
Contributor

@atorralba atorralba left a 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.

@max-schaefer
Copy link
Collaborator Author

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.

@atorralba
Copy link
Contributor

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.

@atorralba atorralba added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Nov 15, 2023
subatoi
subatoi previously approved these changes Nov 15, 2023
Copy link
Contributor

@subatoi subatoi left a 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
Copy link
Contributor

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?

Copy link
Collaborator Author

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>
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Comment on lines 37 to 40
<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>
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

@max-schaefer
Copy link
Collaborator Author

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

mattpollard

This comment was marked as outdated.

subatoi
subatoi previously approved these changes Nov 16, 2023
Copy link
Contributor

@subatoi subatoi left a 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 🎉

java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp Outdated Show resolved Hide resolved
java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp Outdated Show resolved Hide resolved
Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>
@atorralba
Copy link
Contributor

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

replaceAll is one of the sanitizers not handled by PathSanitizer.qll. This is what we discover by adding tests for the good and bad recommendations 😅.

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.

@max-schaefer
Copy link
Collaborator Author

I'd recommend either accepting the test results as they are for now, or removing the test case temporarily

👍 I've gone with the latter option.

@max-schaefer max-schaefer merged commit ca33402 into main Nov 16, 2023
14 checks passed
@max-schaefer max-schaefer deleted the max-schaefer/tainted-path-qhelp branch November 16, 2023 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Java ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants