Java: CWE-200: Temp directory local information disclosure vulnerability#4388

JLLeitschuh

This vulnerability detects the use of the local temporary directory in ways that potentially expose sensitive information to other users.

Some examples

public class TempDirUsageVulnerable {
    void exampleVulnerable() {
        File temp1 = File.createTempFile("random", ".txt"); // BAD: File has permissions `-rw-r--r--`

        File temp2 = File.createTempFile("random", "file", null); // BAD: File has permissions `-rw-r--r--`

        File systemTempDir = new File(System.getProperty("java.io.tmpdir"));
        File temp3 = File.createTempFile("random", "file", systemTempDir); // BAD: File has permissions `-rw-r--r--`

        File tempDir = com.google.common.io.Files.createTempDir(); // BAD: CVE-2020-8908: Directory has permissions `drwxr-xr-x`

        new File(System.getProperty("java.io.tmpdir"), "/child").mkdir(); // BAD: Directory has permissions `-rw-r--r--`

        File tempDirChildFile = new File(System.getProperty("java.io.tmpdir"), "/child-create-file.txt");
        Files.createFile(tempDirChildFile.toPath()); // BAD: File has permissions `-rw-r--r--`
    }
}

This id is rather generic. Could you make it a bit more specific to this query?

Fixed, I think

aibaars

For reference I think Apache Ant had a similar vulnerability CVE-2020-1945. I think this was addressed by

You might want to check your query on databases built from those commits and their parents.

JLLeitschuh

Here's one that I found that was just disclosed.
GHSA-269g-pwp5-87pp

melloware

@JLLeitschuh So if Junit 4.13.1 can patch this fix why can't Guava?

JLLeitschuh

@melloware 🤷‍♂️ bring it up with the Guava & Google team. There's only so much I can do.

melloware

Can we submit a PR against that ticket for review making the same fix Junit did?

JLLeitschuh

You can always submit a PR, I don't know if the Google team will accept it thought. I'd advise that you ask them if they'd consider accepting a PR on the existing issue:

google/guava#4011

It's probably not that useful to have this discussion here, on this unrelated issue.

You could write:

Suggested change

      
                exists(MethodAccess ma |
          
                  ma.getMethod() instanceof MethodFileSystemFileCreation and
          
                  ma.getQualifier() = this.asExpr()
          
                )
          
                exists(MethodFileSystemFileCreation ma |
          
                  ma.getQualifier() = this.asExpr()
          
                )

This is not quite right. MethodFileSystemFileCreation is a Method not a MethodAccess

Yea, I don't think I can change this to be simpler at all.

JLLeitschuh

There are some outstanding disclosures and CVES from this query. Hence why development is kinda frozen. I'm still leveraging it to report vulns.

JLLeitschuh

Test run of this query:

Method Call: https://lgtm.com/query/3077692672321886364/
Path: https://lgtm.com/query/5276757681085693414/

JLLeitschuh

This PR is now in a "I think I'm happy with it, please review" state. 😃

aibaars

This PR is now in a "I think I'm happy with it, please review" state.

In that case you might want to hit the Ready for review button to change from Draft to Review state ;-)

JLLeitschuh

Good point 😂

smowton

Assigned @aibaars since he has been reviewing this and moved to In Progress state

JLLeitschuh

False positives this is finding (the first path). However, other paths are valid.

https://lgtm.com/query/8810980313419129810/
There is a vulnerability here, but it's being incorrectly identified around the reason.

To simplify, the vulnerability being identified here is because of the following:

new File(System.getProperty("java.io.tmpdir")).mkdirs(); // Not really a vulnerability technically

This doesn't seem like a vulnerability.

However, the following is information disclosure of the file's name:

File tempDir = new File(System.getProperty("java.io.tmpdir"));
tempDir.mkdirs();
File tempFile = new File (tempDir, [USER_INPUT]); // vulnerability
tempFile.createFile(); // vulnerability

smowton

Looks like you're still tweaking this; let me know when you're done?

JLLeitschuh

Looks like you're still tweaking this; let me know when you're done?

I'm wondering if you have any insights into how to fix the problem I described here:

#4388 (comment)

JLLeitschuh

I think I have a fix here that I'm happy with:
49a7367

JLLeitschuh

@smowton I'm happy with this PR as-is

smowton

There's a real test failure to fix

JLLeitschuh

@smowton indeed! Fixed that. Thanks!

felicitymay

Thanks for writing help for your query. A few minor comments, but generally looking good 🙂

I'm wondering if this might be clearer. The query name should be in sentence case.

Suggested change

      
             * @name Temporary Directory Local information disclosure
          
             * @name Local information disclosure in a temporary directory

If we change the name of the query, we'd also need to update it here.

JLLeitschuh

Amazing! Very happy this is finally merged. Only took me ~1.3 years to get it finished 😆

Follow-up that adds OS specific guards: #8032

github-actions Bot added the Java label Oct 2, 2020

JLLeitschuh mentioned this pull request Oct 2, 2020

CVE-2020-8908: Files::createTempDir local information disclosure vulnerability google/guava#4011

Closed

JLLeitschuh force-pushed the feat/JLL/java/CWE-200_temp_directory_local_information_disclosure branch from 7cdf997 to d53d77c Compare October 2, 2020 18:53

aibaars reviewed Oct 8, 2020

View reviewed changes

Comment thread java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure2.ql Outdated Show resolved Hide resolved

JLLeitschuh mentioned this pull request Oct 12, 2020

Java: Track taint through java.io.File constructor and #toURI; URI#toURL #4457

Merged

JLLeitschuh commented Oct 14, 2020

View reviewed changes

Comment thread java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure2.ql Outdated Show resolved Hide resolved

aibaars reviewed Oct 14, 2020

View reviewed changes

JLLeitschuh force-pushed the feat/JLL/java/CWE-200_temp_directory_local_information_disclosure branch from 1b216f8 to b1aeb99 Compare December 8, 2020 20:42

JLLeitschuh mentioned this pull request Jan 2, 2021

Add MethodAccessSystemGetProperty predicate #4898

Merged

smowton self-assigned this Jan 4, 2021

JLLeitschuh force-pushed the feat/JLL/java/CWE-200_temp_directory_local_information_disclosure branch from e53cfd8 to 6deb3d8 Compare January 23, 2021 17:49

github-actions Bot added the documentation label Jan 23, 2021

JLLeitschuh marked this pull request as ready for review January 25, 2021 14:56

JLLeitschuh requested review from a team and felicitymay as code owners January 25, 2021 14:56

smowton assigned aibaars and unassigned smowton Jan 27, 2021

Apply suggestions from code review

7965459

smowton previously approved these changes Feb 8, 2022

View reviewed changes

aschackmull previously approved these changes Feb 8, 2022

View reviewed changes

Fix test requirements, formatting

a6596ea

smowton dismissed stale reviews from aschackmull and themself via a6596ea February 8, 2022 12:02

Consider calls to setReadable(false, false) then setReadable(true, tr… …

7f46640

…ue) to be safe

smowton reviewed Feb 9, 2022

View reviewed changes

Comment thread java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql Outdated Show resolved Hide resolved

JLLeitschuh and others added 2 commits February 9, 2022 10:07

Update java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclo… …

787e3da

…sure.ql Co-authored-by: Chris Smowton <smowton@github.com>

Fix FP from mkdirs call on exact temp directory

49a7367

JLLeitschuh requested a review from smowton February 9, 2022 17:34

smowton reviewed Feb 9, 2022

View reviewed changes

Comment thread java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql Outdated Show resolved Hide resolved

Comment thread java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll Outdated Show resolved Hide resolved

Apply suggestions from code review …

bafcce1

Co-authored-by: Chris Smowton <smowton@github.com>

felicitymay added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Feb 10, 2022

Fix test failure for TempDirLocalInformationDisclosure

eee521e

felicitymay reviewed Feb 10, 2022

View reviewed changes

JLLeitschuh and others added 4 commits February 14, 2022 11:00

Fix implicit 'this' usage

7dee22a

Apply suggestions from code review …

bb580dd

Co-authored-by: Felicity Chapman <felicitymay@github.com>

Apply suggestions from code review …

76964d5

Co-authored-by: Felicity Chapman <felicitymay@github.com>

Review feedback and improve temp dir vulnerable/safe code sugestion

2048aed

smowton approved these changes Feb 14, 2022

View reviewed changes

smowton merged commit 0bf6c83 into github:main Feb 14, 2022

+               * @kind problem
+               * @problem.severity warning
+               * @precision very-high
+               * @id java/local-information-disclosure

+              ---
+              category: newQuery
+              ---
+              * A new query titled "Temporary directory Local information disclosure" (`java/local-temp-file-or-directory-information-disclosure`) has been added.

		@@ -0,0 +1,258 @@
		/**
		* @name Temporary Directory Local information disclosure

	* @name Temporary Directory Local information disclosure
	* @name Local information disclosure in a temporary directory

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: CWE-200: Temp directory local information disclosure vulnerability#4388

Conversation

JLLeitschuh commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aibaars Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh Jan 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aibaars commented Oct 8, 2020

Uh oh!

JLLeitschuh commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melloware commented Oct 12, 2020

Uh oh!

JLLeitschuh commented Oct 12, 2020

Uh oh!

melloware commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JLLeitschuh commented Oct 12, 2020

Uh oh!

Uh oh!

aibaars Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh Jan 23, 2021

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh Jan 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JLLeitschuh commented Nov 10, 2020

Uh oh!

JLLeitschuh commented Jan 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JLLeitschuh commented Jan 23, 2021

Uh oh!

aibaars commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JLLeitschuh commented Jan 25, 2021

Uh oh!

smowton commented Jan 27, 2021

Uh oh!

JLLeitschuh commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smowton commented Feb 9, 2022

Uh oh!

Uh oh!

JLLeitschuh commented Feb 9, 2022

Uh oh!

JLLeitschuh commented Feb 9, 2022

Uh oh!

Uh oh!

Uh oh!

JLLeitschuh commented Feb 10, 2022

Uh oh!

smowton commented Feb 10, 2022

Uh oh!

JLLeitschuh commented Feb 10, 2022

Uh oh!

felicitymay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

felicitymay Feb 10, 2022

JLLeitschuh commented Oct 2, 2020 •
edited
Loading

JLLeitschuh commented Oct 12, 2020 •
edited
Loading

melloware commented Oct 12, 2020 •
edited
Loading

JLLeitschuh commented Jan 23, 2021 •
edited
Loading

aibaars commented Jan 25, 2021 •
edited
Loading

JLLeitschuh commented Feb 8, 2022 •
edited
Loading