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

erik-krogh · 2024-01-23T11:59:46Z

The previous suggestion heavily suggested that paths should be checked to be within a safe folder, even though that is not always relevant.
I added some more examples, and iterated on the text in the QHelp to try and improve it.

See the backref for more context and motivation.

I checked in the markdown render of the QHelp to get a nice diff that's easy to review, and deleted it again in the last commit.

@max-schaefer: Can you have a first look at this?

github-actions · 2024-01-23T12:01:27Z

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

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.

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.

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.
Common Weakness Enumeration: CWE-22.
Common Weakness Enumeration: CWE-23.
Common Weakness Enumeration: CWE-36.
Common Weakness Enumeration: CWE-73.

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

Arbitrary file access during archive extraction ("Zip Slip")

Extracting files from a malicious zip file, or similar type of archive, is at risk of directory traversal attacks if filenames from the archive are not properly validated.

Zip archives contain archive entries representing each file in the archive. These entries include a file path for the entry, but these file paths are not restricted and may contain unexpected special elements such as the directory traversal element (..). If these file paths are used to create a filesystem path, then a file operation may happen in an unexpected location. This can result in sensitive information being revealed or deleted, or an attacker being able to influence behavior by modifying unexpected files.

For example, if a zip file contains a file entry ..\sneaky-file, and the zip file is extracted to the directory c:\output, then naively combining the paths would result in an output file path of c:\output\..\sneaky-file, which would cause the file to be written to c:\sneaky-file.

Recommendation

Ensure that output paths constructed from zip archive entries are validated to prevent writing files to unexpected locations.

The recommended way of writing an output file from a zip archive entry is to verify that the normalized full path of the output file starts with a prefix that matches the destination directory. Path normalization can be done with either java.io.File.getCanonicalFile() or java.nio.file.Path.normalize(). Prefix checking can be done with String.startsWith(..), but it is better to use java.nio.file.Path.startsWith(..), as the latter works on complete path segments.

Another alternative is to validate archive entries against a whitelist of expected files.

Example

In this example, a file path taken from a zip archive item entry is combined with a destination directory. The result is used as the destination file path without verifying that the result is within the destination directory. If provided with a zip file containing an archive path like ..\sneaky-file, then this file would be written outside the destination directory.

void writeZipEntry(ZipEntry entry, File destinationDir) {
    File file = new File(destinationDir, entry.getName());
    FileOutputStream fos = new FileOutputStream(file); // BAD
    // ... write entry to fos ...
}

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.

void writeZipEntry(ZipEntry entry, File destinationDir) {
    File file = new File(destinationDir, entry.getName());
    if (!file.toPath().normalize().startsWith(destinationDir.toPath()))
        throw new Exception("Bad zip entry");
    FileOutputStream fos = new FileOutputStream(file); // OK
    // ... write entry to fos ...
}

References

Snyk: Zip Slip Vulnerability.
OWASP: Path Traversal.
Common Weakness Enumeration: CWE-22.

max-schaefer

LGTM, this is much clearer than what was there before!

max-schaefer · 2024-01-23T13:04:04Z

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

+	Path filePath = publicFolder.resolve(filename).normalize().toAbsolutePath();
+
+	// GOOD: ensure that the path stays within the public folder
+	if (!filePath.startsWith(publicFolder)) {


Is it worth refining this check by adding a path separator after publicFolder?

…njection

atorralba

Thanks for doing this! Looping the docs team in as well.

There's one caveat though: users will take these code snippets as prime examples of what should be done to get rid of alerts, but in TaintedPathGoodFolder.java we actually raise an alert when publicFolder.resolve(filename) is called, because Path.resolve is currently a sink (it shouldn't, but that's another debate).

This effectively means that we can't create a Path from user input without raising an alert, so the Path-related validation methods (like resolve + normalize to then check that the path is within an expected directory) can't be recommended until we sort this out.

To catch this sort of thing, it's good practice that we add any QHelp examples as test cases.

Anyway, I would keep the TaintedPathGoodFolder.java example out of this PR for now.

erik-krogh · 2024-01-25T09:31:21Z

There's one caveat though

I know, and I tried to see if I could rewrite the example to do effectively the same thing, but without constructing any Path objects, but that ended up very ugly (and possibly buggy on Windows).

I want an example of how to sanitize a path that can contain multiple path elements by ensuring that the path is within a safe folder. Because such an example is relevant to the real world.
I want to keep the example, even though it gives a false positive in the analysis, because it is the correct way of fixing that specific scenario.

subatoi

Looks great! I only had minor suggestions and I've left reasoning for all of them, but if you have any questions about any of them then please just ask me. Similarly if you need me to take another look after you've made any further changes. Thanks!

subatoi · 2024-01-25T09:08:50Z

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

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


Suggested change

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.

Minor: main suggestion here is just to use "could" instead of "may potentially" for emphasis and less redundancy (I realise "may potentially" was in the original, already-approved, text). Other than that I've just added some commas to break the sentence up a bit.

In any case, like all my suggestions, it's not urgent.

subatoi · 2024-01-25T09:17:48Z

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

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


Suggested change

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.

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.

Main suggestion is just to make emphasis on "you" (the user).

subatoi · 2024-01-25T09:18:56Z

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

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


Suggested change

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.



Very minor: commas to break the sentence up a bit

subatoi · 2024-01-25T09:23:30Z

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

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


Suggested change

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.

V minor: suggestion to change "just supposed to be" to "should be", to make it slightly less idiomatic (ie "supposed to be" might be tricky for some non-native English speakers)

subatoi · 2024-01-25T09:24:17Z

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

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


Suggested change

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.

If the input should be within a specific directory, you can check that the resolved path

is still contained within that directory.

Same as above

erik-krogh · 2024-01-25T10:21:52Z

Looks great! I only had minor suggestions and I've left reasoning for all of them, but if you have any questions about any of them then please just ask me. Similarly if you need me to take another look after you've made any further changes. Thanks!

Thanks 👍
I've applied all your suggested changes.

Now I think it's up to the Java team what we do about the FP.

github-actions · 2024-01-25T10:22:01Z

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 be absolute paths, or may contain unexpected special characters such as "..". Such a path could 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 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.

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

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 should only be a file name, you can check that it doesn't contain any path separators or ".." sequences.

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 should be within a specific directory, you can check that the resolved path is still contained within that directory.

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();
	}
}

References

OWASP: Path Traversal.
Common Weakness Enumeration: CWE-22.
Common Weakness Enumeration: CWE-23.
Common Weakness Enumeration: CWE-36.
Common Weakness Enumeration: CWE-73.

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

Arbitrary file access during archive extraction ("Zip Slip")

Extracting files from a malicious zip file, or similar type of archive, is at risk of directory traversal attacks if filenames from the archive are not properly validated.

Zip archives contain archive entries representing each file in the archive. These entries include a file path for the entry, but these file paths are not restricted and may contain unexpected special elements such as the directory traversal element (..). If these file paths are used to create a filesystem path, then a file operation may happen in an unexpected location. This can result in sensitive information being revealed or deleted, or an attacker being able to influence behavior by modifying unexpected files.

For example, if a zip file contains a file entry ..\sneaky-file, and the zip file is extracted to the directory c:\output, then naively combining the paths would result in an output file path of c:\output\..\sneaky-file, which would cause the file to be written to c:\sneaky-file.

Recommendation

Ensure that output paths constructed from zip archive entries are validated to prevent writing files to unexpected locations.

The recommended way of writing an output file from a zip archive entry is to verify that the normalized full path of the output file starts with a prefix that matches the destination directory. Path normalization can be done with either java.io.File.getCanonicalFile() or java.nio.file.Path.normalize(). Prefix checking can be done with String.startsWith(..), but it is better to use java.nio.file.Path.startsWith(..), as the latter works on complete path segments.

Another alternative is to validate archive entries against a whitelist of expected files.

Example

In this example, a file path taken from a zip archive item entry is combined with a destination directory. The result is used as the destination file path without verifying that the result is within the destination directory. If provided with a zip file containing an archive path like ..\sneaky-file, then this file would be written outside the destination directory.

void writeZipEntry(ZipEntry entry, File destinationDir) {
    File file = new File(destinationDir, entry.getName());
    FileOutputStream fos = new FileOutputStream(file); // BAD
    // ... write entry to fos ...
}

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.

void writeZipEntry(ZipEntry entry, File destinationDir) {
    File file = new File(destinationDir, entry.getName());
    if (!file.toPath().normalize().startsWith(destinationDir.toPath()))
        throw new Exception("Bad zip entry");
    FileOutputStream fos = new FileOutputStream(file); // OK
    // ... write entry to fos ...
}

References

Snyk: Zip Slip Vulnerability.
OWASP: Path Traversal.
Common Weakness Enumeration: CWE-22.

erik-krogh added 4 commits January 23, 2024 12:56

check in the TaintedPath qhelp as markdown to get pretty diffs

6b66f5c

move the examples for the qhelps into an example/ folder

4958c19

iterate on the java/path-injection qhelp

57e0b3c

delete the markdown file again

00dadeb

github-actions bot added documentation Java labels Jan 23, 2024

max-schaefer previously approved these changes Jan 23, 2024

View reviewed changes

add a trailing slash to the folder check in the QHelp for java/path-i…

158ff0d

…njection

erik-krogh dismissed max-schaefer’s stale review via 158ff0d January 23, 2024 13:46

erik-krogh marked this pull request as ready for review January 23, 2024 13:46

erik-krogh requested a review from a team as a code owner January 23, 2024 13:46

atorralba reviewed Jan 25, 2024

View reviewed changes

atorralba added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Jan 25, 2024

subatoi previously approved these changes Jan 25, 2024

View reviewed changes

subatoi self-assigned this Jan 25, 2024

apply suggestions from doc review

05a59d2

erik-krogh dismissed subatoi’s stale review via 05a59d2 January 25, 2024 10:20

erik-krogh and others added 2 commits January 25, 2024 12:14

add missing 

73e3fad

Add new snippets as tests

282632c

atorralba approved these changes Jan 25, 2024

View reviewed changes

atorralba merged commit 661c5cf into github:main Jan 25, 2024

max-schaefer mentioned this pull request Mar 22, 2024

Go: Update query help for go/path-injection to include example fixes. #16020

Merged

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

		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.

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

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

Uh oh!

Conversation

erik-krogh commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 23, 2024

Uncontrolled data used in path expression

Recommendation

Example

References

Arbitrary file access during archive extraction ("Zip Slip")

Recommendation

Example

References

Uh oh!

max-schaefer left a comment

Choose a reason for hiding this comment

Uh oh!

max-schaefer Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

atorralba left a comment

Choose a reason for hiding this comment

Uh oh!

erik-krogh commented Jan 25, 2024

Uh oh!

subatoi left a comment

Choose a reason for hiding this comment

Uh oh!

subatoi Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

subatoi Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

subatoi Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

subatoi Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

subatoi Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

erik-krogh commented Jan 25, 2024

Uh oh!

github-actions bot commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uncontrolled data used in path expression

Recommendation

Example

References

Arbitrary file access during archive extraction ("Zip Slip")

Recommendation

Example

References

Uh oh!

Uh oh!

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

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

erik-krogh commented Jan 23, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading