Guava: Files::createTempDir local information disclosure vulnerability

[JLLeitschuh]

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.

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.

[melloware]

Yuck this code is being reported as a security vulnerability by Sonatype IQ Server as SONATYPE-2020-0926.

RECOMMENDATION
_There is no non-vulnerable upgrade path for this component/package. We recommend investigating alternative components or replacing all usages of the deprecated Files.createTempDir() method with a safer alternative, such as java.nio.file.Files.createTempDirectory() for Java 7+._

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.

[JLLeitschuh]

Can someone from Google make an official statement on whether or not this issue will receive a CVE number or not?

[melloware]

And leaving the deprecated there is OK so people have Javadoc on alternatives but shouldn't it throw UnsupportedOperationException instead of just leaving this vulnerable code in here for someone to use?

[JLLeitschuh]

@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 File.createTempFile method changed between Java 6 and Java 7.

• https://docs.oracle.com/javase/6/docs/api/java/io/File.html#createTempFile(java.lang.String,%20java.lang.String)
• https://docs.oracle.com/javase/7/docs/api/java/io/File.html#createTempFile(java.lang.String,%20java.lang.String)

This new line of documentation reads:

The Files.createTempFile method provides an alternative method to create an empty file in the temporary-file directory. Files created by that method may have more restrictive access permissions to files created by this method and so may be more suited to security-sensitive applications.

[melloware]

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.

[JLLeitschuh]

A few options for you.

1. Push back on Sonatype's analysis of this vulnerability. As a customer, if you think that your security vendor's analysis of a given vulnerability is rating the vuln as too high, then push back. Feel free to include me in the email thread. My email address can be found on my GitHub profile.
2. Push back on management. Here's an ArchUnit test that you can add into your JUnit or TestNG tests that will verify that this method isn't used anywhere, thus preventing its from being used by someone tomorrow.

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)");

[melloware]

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.

[JLLeitschuh]

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.

Agreed.

A few suggestions for you:

1. Create one Gradle build that pulls down all of the JAR artifacts from your different applications, load them all onto the classpath of that Gradle build, and run the check there. Sounds complicated, but it may not be if you publish all your jars to the same internal Sonatype instance.
2. Something that may scale better is GitHub's Code Scanning feature. I'm a OSS Security researcher that contributes to the GitHub Security Lab Bug Bounty program. Your question here actually inspired me to finally write the queries below for my GitHub Security Lab Bug Bounty submission.

The following two CodeQL queries would find all instances of this vulnerability across your codebases.

TempDirsUtil.qll (This is a utility the two queries below depend upon)

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 1

Finds

/**
 * @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 vulnerable

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

With 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: https://github.com/github/codeql/pull/4388

---

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.

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 😆 . Legit, do not do this. GitHub doesn't scale well to having this many forks bound to your account. The second project, where I generated 3,880 pull requests, I realized that it was better for the health of my account if I forked them under organizations instead. I ended up creating 45 GitHub organizations for that project.

• https://github.com/BulkSecurityGeneratorProject
• https://github.com/BulkSecurityGeneratorProject1
• https://github.com/BulkSecurityGeneratorProject2
• ...
• https://github.com/BulkSecurityGeneratorProject45

[melloware]

@JLLeitschuh Than you for all your work! That GitHub bot code will be a huge help!

[semmalimayan]

@JLLeitschuh does the com.google.common.io.Files.createParentDirs() creates with similar vulnerable permissions ?

[JLLeitschuh]

@semmalimayan here's the method you're asking about.

https://github.com/google/guava/blob/fec0dbc4634006a6162cfd4d0d09c962073ddf40/guava/src/com/google/common/io/Files.java#L470-L487

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.

[JLLeitschuh]

A similar vulnerability has been disclosed and patched in JUnit 4. CVE pending.

https://github.com/junit-team/junit4/security/advisories/GHSA-269g-pwp5-87pp

To reiterate my earlier question:

Can someone from Google make an official statement on whether or not this issue will receive a CVE number or not?

CC: @google-admin

[melloware]

Here is the fix Junit did: https://github.com/junit-team/junit4/commit/610155b8c22138329f0723eec22521627dbc52ae#diff-25d902c24283ab8cfbac54dfa101ad31

[melloware]

@JLLeitschuh I submitted a PR for this so we will see. I feel like its totally harmless since the method is deprecated anyway.