CVE-2020-8908: Files::createTempDir local information disclosure vulnerability

[JLLeitschuh]

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.

https://github.com/google/guava/issues/4011#issuecomment-765672282

---

Since the fix for this vulnerability is now disclosed by this commit (https://github.com/google/guava/commit/fec0dbc4634006a6162cfd4d0d09c962073ddf40) and it was closed internally by google as 'Intended Functionality' I figure I'll disclose the vulnerability fully.

Vulnerability

File guavaTempDir = com.google.common.io.Files.createTempDir();
System.out.println("Guava Temp Dir: " + guavaTempDir.getName());
runLS(guavaTempDir.getParentFile(), guavaTempDir); // Prints the file permissions -> drwxr-xr-x
File child = new File(guavaTempDir, "guava-child.txt");
child.createNewFile();
runLS(guavaTempDir, child); // Prints the file permissions -> -rw-r--r--

On the flip side, when using java.nio.file.Files, this creates a directory with the correct file permissions.

Path temp = Files.createTempDirectory("random-directory");
System.out.println("Files Temp Dir: " + temp.getFileName());
runLS(temp.toFile().getParentFile(), temp.toFile()); // Prints the file permissions -> drwx------
Path child = temp.resolve("jdk-child.txt");
child.toFile().createNewFile();
runLS(temp.toFile(), child.toFile()); // Prints the file permissions -> -rw-r--r--

Impact

The impact of this vulnerability is that, the file permissions on the file created by com.google.common.io.Files.createTempDir allows 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 /temp directory 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.tmpdir system property to a "safe" directory when starting the JVM.

Resolution

The resolution by the Google team was the following:

The team decided to document the behavior, as well as deprecate the method as other alternatives exist.

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.

[prbprbprb]

although I don't know if there's an effective difference in this situation

Hmm, me either and I think we'd need to look at some real devices rather than the emulators to be sure.

[tony--]

I was curious to search through the source and make the table that @JLLeitschuh suggested, so FWIW...

Android version to git tag mapping from https://source.android.com/setup/start/build-numbers#source-code-tags-and-builds

Android Version	java.io.tmpdir value	git reference
Donut	/sdcard	ref
Eclair	/sdcard	ref
Froyo	/sdcard	ref
Gingerbread	/sdcard	ref
Ice Cream Sandwich	/sdcard	ref
Activity-specific temp directories added	app cache directory	ref
Jelly Bean	app cache directory	ref 1, ref 2
KitKat	app cache directory	ref 1, ref 2
AndroidRuntime.startVm initialization of "java.io.tmpdir" removed	app cache directory	ref
Lollipop	app cache directory	ref
Marshmallow	app cache directory	ref
Nougat	app cache directory	ref
Oreo	app cache directory	ref
Pie	app cache directory	ref
Android10	app cache directory	ref
Android11	app cache directory	ref

It's basically what @cpovirk posted already, but with all the releases and in table form.

With regard to appending a unique suffix to temporary directory names, I remember doing this. IIRC, deleting from /sdcard was slow, so reading in the existing list and finding the next available was more performant than deleting the old one. It seems like there was a blog post comparing a couple approaches.

Something else to keep in mind is that although we can make a list based on reference releases, device vendors sometimes modified the behavior of things like /sdcard to point to some other location or could make java.io.tmpdir point to a different location with their downstream distribution.

[tony--]

So https://github.com/google/cvelist/pull/1 has been merged, but https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-8908 and https://nvd.nist.gov/vuln/detail/CVE-2020-8908 are still unchanged. Any ETA on when we'll see the public-facing advisories for CVE-2020-8908 get updated, @cpovirk?

[nick-someone]

@tony-- The PR to watch is https://github.com/CVEProject/cvelist/pull/713 (which would apply this change to the actual upstream CVE sources). Sounds like there are some minor comments going on there.

[nick-someone]

Two quick updates here:

• It appears that the CVE PR was merged a couple of days ago. The changed description appears to have made its way to https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-8908 and https://nvd.nist.gov/vuln/detail/CVE-2020-8908, so hooray for that!
• From my earlier comment, I made reference to an upcoming open source security response guide. Thanks to the work of @annabellegoth2boss, this is now published! See blog post and the guide on GitHub at https://github.com/google/oss-vulnerability-guide

[melloware]

Now we just need someone to fix this CVE and release a new Guava. 😄

[nick-someone]

@melloware 😄

There continue to be discussions about how best to proceed regarding this method, with some points summarized by https://github.com/google/guava/issues/4011#issuecomment-769975829

Shortly:

• This method would seem to appear to be the type of method we've reserved the "right" to remove (i.e.: It's @Beta and we're intending to remove it for security purposes).
• However well-intended library developers are, it is certainly possible for libraries in the wild to be calling this method (see this conservative GitHub search that tries to exclude test files). We've certainly been burned in the past by developers who try to migrate to a new version of Guava, only to find themselves without a working combination of Guava libraries as their dependents require older versions of Guava while the application wants to use a newer version.
  • We've tried our best to avoid removing methods, even @Beta methods for this reason.

We're discussing what would be in the best interest here - as there are lots of subtle, hidden costs to removing this method. Those costs might be worth paying, of course, but I want to make sure we've got it all accounted for before deciding to remove the method in the next version.

[melloware]

OK thank you. My vote is removal 😄

[marlowa]

@melloware smile

• However well-intended library developers are, it is certainly possible for libraries in the wild to be calling this method (see this conservative GitHub search that tries to exclude test files). We've certainly been burned in the past by developers who try to migrate to a new version of Guava, only to find themselves without a working combination of Guava libraries as their dependents require older versions of Guava while the application wants to use a newer version.

To mitigate this I suggest that the version that has it removed increments the major version number. This would be a tip-off for the API incompatibility.

[melloware]

Any update on removing this so all the Security Services stop reporting Guava as vulnerable?

[yeikel]

Bump as this continues to be an issue in security scans

[JLLeitschuh]

A similar vulnerability exists in the Kotlin Stdlib, they haven't fixed it either. They deprecated the method as well. https://github.com/JLLeitschuh/security-research/security/advisories/GHSA-5w9v-8x7x-rfqm

[jehaineoracle]

Bump. When can we expect the vulnerable code to be removed from Guava so we can pass the security scan?

[FrancoisTrible]

When can we expect a fix ?

[romani]

now Sonatype highlighting this as Threat: https://ossindex.sonatype.org/vulnerability/sonatype-2020-0926?component-type=maven&component-name=com.google.guava%2Fguava&utm_source=ossindex-client&utm_medium=integration&utm_content=1.7.0

[scott--b]

We're discussing what would be in the best interest here - as there are lots of subtle, hidden costs to removing this method. Those costs might be worth paying, of course, but I want to make sure we've got it all accounted for before deciding to remove the method in the next version.

@nick-someone - apologies for reviving a dormant thread, but can you comment on if there was ever a conclusion reached on this discussion? Trying to get a sense on if removal of the impacted method is still something that is being considered or not.

[sjamaan]

FTR, I'm also not happy about the policy to keep this method around - it causes needless churn in projects that indirectly use this library. First one needs to satisfy oneself that this method isn't used and take steps to ensure it will never be used. More likely, people will just suppress the warning, which means we might as well not have a CVE for the bug in the first place.

[norrisjeremy]

Why can't this method simply be changed to not create a world-readable directory? That would actually remediate this vulnerability, instead of sweeping it under the rug with a claim that it is a deprecated method and users shouldn't utilize it.

[melloware]

Why can't this method simply be changed to not create a world-readable directory? That would actually remediate this vulnerability, instead of sweeping it under the rug with a claim that it is a deprecated method and users shouldn't utilize it.

@norrisjeremy I even submitted a PR that did exactly that and Google rejected it for lame reasons about backwards compatibility.

See: https://github.com/google/guava/pull/5324

[norrisjeremy]

@melloware,

Wow, that is indeed incredibly lame. I can't believe Google refuses to remediate a known security vulnerability 2+ years later.

Thanks, Jeremy

[neworld]

@norrisjeremy I even submitted a PR that did exactly that and Google rejected it for lame reasons about backwards compatibility.

Maybe Files::createPrivateTempDir may work? With the deprecation of the old method.

[norrisjeremy]

FYI, the whole reason I landed here is that OWASP Dependency Check is now flagging anything that uses Guava as vulnerable (see https://github.com/jeremylong/DependencyCheck/issues/5526).

My best guess is that the CPE for this vulnerability used to incorrectly mark any Guava versions as >= 30.0 as not being vulnerable, but somebody finally noticed that wasn't actually true, and they adjusted the CPE to start correctly flagging all versions of Guava as vulnerable.

[jumarko]

This has recently failed a couple of automated dependency checks in our CI pipeline. I'm wondering if it can be fixed anytime soon....

[JLLeitschuh]

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

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:

https://github.com/openrewrite/rewrite-migrate-java/blob/f104cb19305fa18b3faa3b929f10765691da8fb9/src/main/java/org/openrewrite/java/migrate/guava/NoGuavaCreateTempDir.java

[norrisjeremy]

Hi @JLLeitschuh,

That's great... But wouldn't it be a whole lot simpler if Google simply stepped up and actually remediated the issue in Guava itself? Doesn't seem like it would be especially difficult to just not create a world readable directory in the first place.

Thanks, Jeremy

[JLLeitschuh]

But wouldn't it be a whole lot simpler if Google simply stepped up and actually remediated the issue in Guava itself?

I mean, absolutely, yes. But that ship may have sailed. I tried to convince them to fix this without any luck 3 years ago. 🤷‍♂️

[arjopoldervaart]

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.

[cpovirk]

@nick-someone was just OOO but will be getting back and may have thoughts.

I gather from the many recent posts (https://github.com/google/guava/issues/4011#issuecomment-1368684825, https://github.com/google/guava/issues/4011#issuecomment-1452177466, https://github.com/google/guava/issues/4011#issuecomment-1452877877, https://github.com/google/guava/issues/4011#issuecomment-1455153956), 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.

[JLLeitschuh]

and we support versions of Android that are old enough not to offer Java 7 APIs

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:

• https://docs.oracle.com/javase/7/docs/api/java/io/File.html#setReadable(boolean,%20boolean)
• https://docs.oracle.com/javase/7/docs/api/java/io/File.html#setExecutable(boolean,%20boolean)
• https://docs.oracle.com/javase/7/docs/api/java/io/File.html#setWritable(boolean,%20boolean)

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

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.

🎉

[cpovirk]

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 setReadable, there would still be a window during which the directory is accessible. And it turns out to be possible (at least under Linux) for someone to open that directory during that window and then use that open file descriptor(?) to read even files that are created after the directory was made unreadable to that user. (I may have a demonstration of this in the form of some snippets of Java lying around somewhere.... I'll see if I can dig it up.) That's not to say that the change wouldn't help, just that it might not be enough to get tools to stop reporting a vulnerability, either.

[cpovirk]

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.

[norrisjeremy]

Why all the consternation for a method that is flagged as @Beta?

Signifies that a public API (public class, method or field) is subject to incompatible changes, or even removal, in a future release.
An API bearing this annotation is exempt from any compatibility guarantees made by its containing library.
Note that the presence of this annotation implies nothing about the quality or performance of the API in question, only the fact that it is not "API-frozen."

[JLLeitschuh]

I found the hacked-up Guava code that I used to demo the race. The end of the file shows a transcript of the race in action.

Nifty! Mind throwing that into a GitHub Gist instead? Would be far more readable. But I appreciate what you're pointing out here 🤔

[cpovirk]

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

[JLLeitschuh]

You've just me to inspired 🐰 🕳️ (rabbit hole), thanks 😆

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 😬. There may be a few other CVEs to come out of this. Thanks! 😆

https://unix.stackexchange.com/questions/631766/can-i-get-unix-file-permissions-to-take-effect-immediately-for-all-processes

[JLLeitschuh]

What if you create the directory in a user-controlled directory, ensure the permissions are set correctly, then move it into the temporary directory?

[cpovirk]

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

[cpovirk]

Release coming tomorrow if all goes well. I would like to think this will now be indisputably fixed: We use java.nio.file to create the file with the right permissions to begin with. The only exception is under old versions of Android: Under those, we check that the version is new enough to have a secure temporary directory, and if it is, we know that it's safe to use java.io. In the extremely unlikely event that someone is running with a brand-new Guava on a truly ancient Ice Cream Sandwich device (which does not have a secure temporary directory), we throw an exception.

[JLLeitschuh]

For reference, this is the commit that fixes this: https://github.com/google/guava/commit/feb83a1c8fd2e7670b244d5afd23cba5aca43284

@cpovirk, will you be updating the CVE to reflect that there is now a fixed version?

[cpovirk]

Sorry, I'm finally coming back as the dust settles from the release....

I think the CVE people have considered this resolved since the deprecation in 30.0. But to state the obvious, we're here because plenty of other people don't consider that "resolution" to be sufficient. Are you advocating for changing the CVE to list all versions before 32.0.0 as vulnerable? Separately, are there other systems that have still been considering Guava to be vulnerable that I should try to get to stop doing that? There's some discussion on that front about the other CVE in #6532.

[JLLeitschuh]

Are you advocating for changing the CVE to list all versions before 32.0.0 as vulnerable?

Yes, I think that would be appropriate.

[cpovirk]

There is probably no downside to doing so at this point, given that we'll be reporting those versions as vulnerable on account of the other CVE, anyway. (That puts us in a better position that the Kotlin runtime, JUnit, or the JDK, whose recent versions wouldn't otherwise show vulnerabilities and who haven't released a version with the vulnerable functionality removed.) I'll see what I can do.

[cpovirk]

I imagine that the change still has to propagate to various systems, but here it is live on cve.org, thanks to our CVE people:

affected from 1.0 before 32.0

Would you like me to edit your original post here with a summary, leave that to you, or write a new summary here or elsewhere (as we did with CVE-2018-10237)?