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
CVE-2020-8908: Files::createTempDir local information disclosure vulnerability #4011
Comments
|
Yuck this code is being reported as a security vulnerability by Sonatype IQ Server as SONATYPE-2020-0926. RECOMMENDATION I know its deprecated by why not just rip this code out? It will be forever reported as a vulnerability by apps like OWASP and Sonatype. |
|
Can someone from Google make an official statement on whether or not this issue will receive a CVE number or not? |
|
And leaving the deprecated there is OK so people have Javadoc on alternatives but shouldn't it throw |
|
@melloware, this is indeed a security vulnerability, however, given that the severity of this vulnerability is quite low, I think that the way that Google and the Guava team has handled this vulnerability with a documented depreciation is appropriate. It's important to note that the JDK actually contains a method with a very similar vulnerability. You'll notice that the documentation on
This new line of documentation reads:
|
|
I understand but its listed in Sonatype with a score of 7 which means my management team freaks out we are using a JAR with a vulnerability. Even if you prove to management you aren't using that feature it doesn't mean someone tomorrow in the org couldn't accidentally start using it. So that is why I am PRO on removal vs documentation. When you work at a very security sensitive client who receives daily reports that include this type of alert. |
|
A few options for you.
https://github.com/TNG/ArchUnit @ArchTest
public static final ArchRule forbid_calls_to_guava_Files_createTempDir =
classes()
.should(not(callMethod(com.google.common.io.Files.class, "createTempDir")))
.because("Files::createTempDir contains a local information disclosure vulnerability (https://github.com/google/guava/issues/4011)"); |
|
Cool I didn't know about ArchUnit!!! It won't help much we have over 50 in production applications using it so we would have to add this test to each one and all future applications. As for your email when I click on your profile GitHub gives me the "Unicorn page is taking too long to load!"??? you must have a lot in your profile. |
Agreed. A few suggestions for you:
The following two CodeQL queries would find all instances of this vulnerability across your codebases.
import java
import semmle.code.java.dataflow.FlowSources
class MethodAccessSystemGetProperty extends MethodAccess {
MethodAccessSystemGetProperty() {
getMethod() instanceof MethodSystemGetProperty
}
predicate hasPropertyName(string propertyName) {
this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = propertyName
}
}
class MethodAccessSystemGetPropertyTempDir extends MethodAccessSystemGetProperty {
MethodAccessSystemGetPropertyTempDir() { this.hasPropertyName("java.io.tmpdir") }
}
/**
* Find dataflow from the temp directory system property to the `File` constructor.
* Examples:
* - `new File(System.getProperty("java.io.tmpdir"))`
* - `new File(new File(System.getProperty("java.io.tmpdir")), "/child")`
*/
private predicate isTaintedFileCreation(Expr expSource, Expr exprDest) {
exists(ConstructorCall construtorCall |
construtorCall.getConstructedType() instanceof TypeFile and
construtorCall.getArgument(0) = expSource and
construtorCall = exprDest
)
}
private class TaintFollowingFileMethod extends Method {
TaintFollowingFileMethod() {
getDeclaringType() instanceof TypeFile and
(
hasName("getAbsoluteFile") or
hasName("getCanonicalFile")
)
}
}
private predicate isTaintFollowingFileTransformation(Expr expSource, Expr exprDest) {
exists(MethodAccess fileMethodAccess |
fileMethodAccess.getMethod() instanceof TaintFollowingFileMethod and
fileMethodAccess.getQualifier() = expSource and
fileMethodAccess = exprDest
)
}
predicate isAdditionalFileTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
isTaintedFileCreation(node1.asExpr(), node2.asExpr()) or
isTaintFollowingFileTransformation(node1.asExpr(), node2.asExpr())
}Query 1Finds /**
* @name Temporary Directory Local information disclosure
* @description Detect local information disclosure via the java temporary directory
* @kind problem
* @problem.severity warning
* @precision very-high
* @id java/local-information-disclosure
* @tags security
* external/cwe/cwe-200
*/
import TempDirUtils
/**
* All `java.io.File::createTempFile` methods.
*/
class MethodFileCreateTempFile extends Method {
MethodFileCreateTempFile() {
this.getDeclaringType() instanceof TypeFile and
this.hasName("createTempFile")
}
}
class TempDirSystemGetPropertyToAnyConfig extends TaintTracking::Configuration {
TempDirSystemGetPropertyToAnyConfig() { this = "TempDirSystemGetPropertyToAnyConfig" }
override predicate isSource(DataFlow::Node source) {
source.asExpr() instanceof MethodAccessSystemGetPropertyTempDir
}
override predicate isSink(DataFlow::Node source) { any() }
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
isAdditionalFileTaintStep(node1, node2)
}
}
abstract class MethodAccessInsecureFileCreation extends MethodAccess { }
/**
* Insecure calls to `java.io.File::createTempFile`.
*/
class MethodAccessInsecureFileCreateTempFile extends MethodAccessInsecureFileCreation {
MethodAccessInsecureFileCreateTempFile() {
this.getMethod() instanceof MethodFileCreateTempFile and
(
this.getNumArgument() = 2 or
getArgument(2) instanceof NullLiteral or
// There exists a flow from the 'java.io.tmpdir' system property to this argument
exists(TempDirSystemGetPropertyToAnyConfig config |
config.hasFlowTo(DataFlow::exprNode(getArgument(2)))
)
)
}
}
class MethodGuavaFilesCreateTempFile extends Method {
MethodGuavaFilesCreateTempFile() {
getDeclaringType().hasQualifiedName("com.google.common.io", "Files") and
hasName("createTempDir")
}
}
class MethodAccessInsecureGuavaFilesCreateTempFile extends MethodAccessInsecureFileCreation {
MethodAccessInsecureGuavaFilesCreateTempFile() {
getMethod() instanceof MethodGuavaFilesCreateTempFile
}
}
from MethodAccessInsecureFileCreation methodAccess
select methodAccess,
"Local information disclosure vulnerability due to use of file or directory readable by other local users."Example of this query finding vulns against other Google projects: https://lgtm.com/query/7917272935407723538/ The above query will find method calls like this: File.createTempFile("biz", "baz", null); // Flagged vulnerable
File.createTempFile("biz", "baz"); // Flagged vulnerable
com.google.common.io.Files.createTempDir(); // Flagged vulnerable
File tempDirChild = new File(new File(System.getProperty("java.io.tmpdir")), "/child"); // Not Flagged
File.createTempFile("random", "file", tempDirChild); // Flagged vulnerableQuery 2:/**
* @name Temporary Directory Local information disclosure
* @description Detect local information disclosure via the java temporary directory
* @kind path-problem
* @problem.severity warning
* @precision very-high
* @id java/local-information-disclosure
* @tags security
* external/cwe/cwe-200
*/
import TempDirUtils
import DataFlow::PathGraph
private class MethodFileSystemCreation extends Method {
MethodFileSystemCreation() {
getDeclaringType() instanceof TypeFile and
(
hasName("mkdir") or
hasName("createNewFile")
)
}
}
private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Configuration {
TempDirSystemGetPropertyToCreateConfig() { this = "TempDirSystemGetPropertyToAnyConfig" }
override predicate isSource(DataFlow::Node source) {
source.asExpr() instanceof MethodAccessSystemGetPropertyTempDir
}
override predicate isSink(DataFlow::Node sink) {
exists (MethodAccess ma |
ma.getMethod() instanceof MethodFileSystemCreation and
ma.getQualifier() = sink.asExpr()
)
}
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
isAdditionalFileTaintStep(node1, node2)
}
}
from DataFlow::PathNode source, DataFlow::PathNode sink, TempDirSystemGetPropertyToCreateConfig conf
where conf.hasFlowPath(source, sink)
select source.getNode(), source, sink,
"Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users.", source.getNode(),
"system temp directory"Example of this query finding vulns against other Google projects: https://lgtm.com/query/548722881855915017/ The above query will find instances of this vulnerability by doing dataflow analysis to find where uses of the system property flow to a file creation location. File tempDirChild = new File(System.getProperty("java.io.tmpdir"), "/child"); // Not flagged
tempDirChild.mkdir(); // Flagged vulnerable
tempDirChild.createNewFile(); // Flagged vulnerableWith the GitHub Code Scanning feature, once my queries are merged, you'll automatically get alerts about these vulnerabilities in your code. The pull request can be found here: github/codeql#4388
You sometimes have to reload a few times. I have over 1,596 forks against my profile. I have a bot that I use to automate the generation of thousands of security-fix pull requests across GitHub projects. The first project I did this for, I forked all the projects under my personal account, I have since learned this is a mistake |
|
@JLLeitschuh Than you for all your work! That GitHub bot code will be a huge help! |
|
@JLLeitschuh does the com.google.common.io.Files.createParentDirs() creates with similar vulnerable permissions ? |
|
@semmalimayan here's the method you're asking about. guava/guava/src/com/google/common/io/Files.java Lines 470 to 487 in fec0dbc
The true answer here is "it depends". For example, this would be vulnerable: File tempDirChild = new File(System.getProperty("java.io.tmpdir"), "/child/text_file.txt");
com.google.common.io.Files.createParentDirs(tempDirChild); // Anything in the 'child' directory would have a local information disclosure vulnerability
tempDirChild.createNewFile(); // This file would be visible to other users, putting sensitive information in this file would be an information disclosure vulnerability.That being said, I think that at that point, this is probably more likely a user-error vulnerability, less a guava vulnerability. |
|
A similar vulnerability has been disclosed and patched in JUnit 4. CVE pending. To reiterate my earlier question:
CC: @google-admin |
|
Here is the fix Junit did: junit-team/junit4@610155b#diff-25d902c24283ab8cfbac54dfa101ad31 |
Open GitHub opened this alert 12 days ago Bump junit from 4.11 to 4.13.1 in /SortDoublingRatios dependencies 1 junit:junit vulnerability found in SortDoublingRatios/pom.xml 12 days ago Remediation Upgrade junit:junit to version 4.13.1 or later. For example: <dependency> <groupId>junit</groupId> <artifactId>junit</artifactId> <version>[4.13.1,)</version> </dependency> Always verify the validity and compatibility of suggestions with your codebase. Details GHSA-269g-pwp5-87pp low severity Vulnerable versions: >= 4.7, < 4.13.1 Patched version: 4.13.1 Vulnerability The JUnit4 test rule TemporaryFolder contains a local information disclosure vulnerability. Example of vulnerable code: public static class HasTempFolder { @rule public TemporaryFolder folder = new TemporaryFolder(); @test public void testUsingTempFolder() throws IOException { folder.getRoot(); // Previous file permissions: `drwxr-xr-x`; After fix:`drwx------` File createdFile= folder.newFile("myfile.txt"); // unchanged/irrelevant file permissions File createdFolder= folder.newFolder("subfolder"); // unchanged/irrelevant file permissions // ... } } Impact On Unix like systems, the system's temporary directory is shared between all users on that system. Because of this, when files and directories are written into this directory they are, by default, readable by other users on that same system. This vulnerability does not allow other users to overwrite the contents of these directories or files. This is purely an information disclosure vulnerability. When analyzing the impact of this vulnerability, here are the important questions to ask: Do the JUnit tests write sensitive information, like API keys or passwords, into the temporary folder? If yes, this vulnerability impacts you, but only if you also answer 'yes' to question 2. If no, this vulnerability does not impact you. Do the JUnit tests ever execute in an environment where the OS has other untrusted users. This may apply in CI/CD environments but normally won't be 'yes' for personal developer machines. If yes, and you answered 'yes' to question 1, this vulnerability impacts you. If no, this vulnerability does not impact you. Patches Because certain JDK file system APIs were only added in JDK 1.7, this this fix is dependent upon the version of the JDK you are using. Java 1.7 and higher users: this vulnerability is fixed in 4.13.1. Java 1.6 and lower users: no patch is available, you must use the workaround below. Workarounds If you are unable to patch, or are stuck running on Java 1.6, specifying the java.io.tmpdir system environment variable to a directory that is exclusively owned by the executing user will fix this vulnerability. References CWE-200: Exposure of Sensitive Information to an Unauthorized Actor Fix commit junit-team/junit4@610155b Similar Vulnerabilities Google Guava - google/guava#4011 Apache Ant - https://nvd.nist.gov/vuln/detail/CVE-2020-1945 JetBrains Kotlin Compiler - https://nvd.nist.gov/vuln/detail/CVE-2020-15824 For more information If you have any questions or comments about this advisory, please pen an issue in junit-team/junit4.
Relevant info: google/guava#4011. We never used it but better prevent this in the future.
|
It's my intention to run a campaign to bulk generate PRs across all OSS to fix this vulnerability (by replacing the method's use with the new java std lib That should significantly decrease the likelihood that this vulnerability will impact you via a transitive dependency. If you're curious and want to do the same across your corporate repositories, this is the OpenRewrite recipe to automate making this change: |
|
Hi @JLLeitschuh, That's great... Thanks, |
I mean, absolutely, yes. But that ship may have sailed. I tried to convince them to fix this without any luck 3 years ago. |
|
If it has been 3 years since the code was deprecated, now seems to be a good time to remove it. We also use the OWASP dependency checker and it is now breaking all the builds that pull in guava. We hardly ever use the library directly so we would need to inspect the library that pulls it in to see if we can safely ignore the CVE. This is not something I would like to spend my time on. Just blindly ignoring it is also not an option. I would support a new attempt to convince Google to remove the code. |
|
@nick-someone was just OOO but will be getting back and may have thoughts. I gather from the many recent posts (#4011 (comment), #4011 (comment), #4011 (comment), #4011 (comment)), as well as from https://stackoverflow.com/q/75615883/28465, that more tools have begun reporting this lately. And, as suggested just above, anyone who actually wants the current functionality has been getting deprecation warnings since version 30.0 (released almost 2.5 years ago), so even a project that is slow to upgrade has probably heard of this by now (perhaps because of, you know, the build breakages...). A quick recap, since I can't remember if we gave a good summary of the situation 3 years ago: We can't change the behavior to be secure because Java didn't provide a secure API for this until Java 7, and we support versions of Android that are old enough not to offer Java 7 APIs. That leaves us with a set of options that all can cause problems for someone: remove the method (and break existing users), make the method work only under JVMs and new versions of Android (and make it do no nothing (and log?) or throw under old versions of Android), or deprecate the method in place (and have some number of tools report warnings). Our call at the time was that deprecating was the least costly way forward. But as various tools move to consider deprecation to be insufficient, and as existing users of the method have more time to find alternatives, that shifts the relative costs and benefits. I still don't know exactly what that will mean for the future, but I think enough has changed that it makes sense for us to give this some focused attention again. |
You can use reflection to secure Java 7+ version usage though, correct? Also, regardless of version, the following two methods have been available since Java 6:
I think you can call each on the file in sequence like this to lock it down before returning it: // Make the file readable by no user
tempFile.setReadable(false, false);
// Then only make it readable to the owner
tempFile.setReadable(true, true);Of course, the better solution on java 7+ is to explicitly use the posix permissions setting APIs offered on
|
|
Right, we could use reflection. However, the code has to do something under Android versions under which the methods aren't available, so we'd be back to either breaking the method under those versions or leaving it insecure under those versions, and I don't think I can predict whether the latter would be enough to get tools to stop reporting a vulnerability. As for |
|
I found the hacked-up Guava code that I used to demo the race [edited to change to a Gist]. The end of the file shows a transcript of the race in action. |
|
Why all the consternation for a method that is flagged as |
Nifty! Mind throwing that into a GitHub Gist instead? Would be far more readable. But I appreciate what you're pointing out here |
|
Ah, yes, that would be smarter: https://gist.github.com/cpovirk/3aa1e9accb44c35f967175b237c6e1fb Don't worry, though: It's still plenty unreadable, since the methods are named things like "deleteDirectoryContentsSecure" (from the code I copied to create the demo) :) |
|
You've just me to inspired I had some previous cases of this vulnerability in some other projects that I had previously believed were safe, but actually aren't because of this information |
|
What if you create the directory in a user-controlled directory, ensure the permissions are set correctly, then move it into the temporary directory? |
|
That sounds to me like it ought to work, but I am not at all an expert in this area. Certainly I am always more comfortable pointing out a specific problem with an approach than I am with claiming that an approach is definitely safe :) |
Guava has vulnerability related to permissions on a temp directory (google/guava#4011) but we don't use that method. This wasn't flagged by the previous version of dependency-check-maven, as it'd been assumed fixed in Guava.
Guava has vulnerability related to permissions on a temp directory (google/guava#4011) but we don't use that method. This wasn't flagged by the previous version of dependency-check-maven, as it'd been assumed fixed in Guava.
- The vulnerable method is deprecated in Guava, but isn't removed. It's necessary to suppress this CVE. See google/guava#4011
Guava has vulnerability related to permissions on a temp directory (google/guava#4011) but we don't use that method. This wasn't flagged by the previous version of dependency-check-maven, as it'd been assumed fixed in Guava.

IMPORTANT NOTE
Updating to Guava 30.0 does not fix this security vulnerability. The method is merely deprecated. There currently exits no fix for this vulnerability.
#4011 (comment)
Since the fix for this vulnerability is now disclosed by this commit (fec0dbc) and it was closed internally by google as 'Intended Functionality' I figure I'll disclose the vulnerability fully.
Vulnerability
On the flip side, when using
java.nio.file.Files, this creates a directory with the correct file permissions.Impact
The impact of this vulnerability is that, the file permissions on the file created by
com.google.common.io.Files.createTempDirallows an attacker running a malicious program co-resident on the same machine can steal secrets stored in this directory. This is because by default on unix-like operating systems the/tempdirectory is shared between all users, so if the correct file permissions aren't set by the directory/file creator, the file becomes readable by all other users on that system.Workaround
This vulnerability can be fixed by explicitly setting the
java.io.tmpdirsystem property to a "safe" directory when starting the JVM.Resolution
The resolution by the Google team was the following:
This completely makes sense to me, and I think is appropriate. The open question that exists in my mind is whether or not this issue warrants a CVE number issued.
The text was updated successfully, but these errors were encountered: