Java: CWE-200: Temp directory local information disclosure vulnerability by JLLeitschuh · Pull Request #4388 · github/codeql

JLLeitschuh · 2020-10-02T04:52:57Z

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

aibaars · 2020-10-08T10:32:23Z

java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure1.ql

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


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

Fixed, I think

java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure2.ql

aibaars · 2020-10-08T10:37:48Z

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 · 2020-10-12T15:19:42Z

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

melloware · 2020-10-12T15:26:19Z

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

JLLeitschuh · 2020-10-12T16:20:15Z

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

melloware · 2020-10-12T16:21:45Z

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

JLLeitschuh · 2020-10-12T16:25:12Z

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.

java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure2.ql

aibaars · 2020-10-14T08:25:26Z

java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure2.ql

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


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.

java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure2.ql

JLLeitschuh · 2020-11-10T19:59:18Z

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 · 2021-01-23T19:19:44Z

Test run of this query:

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

JLLeitschuh · 2021-01-23T23:49:00Z

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

aibaars · 2021-01-25T08:31:02Z

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 · 2021-01-25T11:53:56Z

Good point 😂

smowton · 2021-01-27T11:01:18Z

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

JLLeitschuh · 2022-02-08T18:22:42Z

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

…ue) to be safe

smowton · 2022-02-09T11:34:48Z

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

java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql

JLLeitschuh · 2022-02-09T15:06:53Z

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)

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

JLLeitschuh · 2022-02-09T16:05:35Z

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

java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql

java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll

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

JLLeitschuh · 2022-02-10T13:34:46Z

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

smowton · 2022-02-10T13:52:47Z

There's a real test failure to fix

JLLeitschuh · 2022-02-10T15:41:44Z

@smowton indeed! Fixed that. Thanks!

felicitymay

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

java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.qhelp

java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql

felicitymay · 2022-02-10T16:19:26Z

java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql

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


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

felicitymay · 2022-02-10T16:20:41Z

java/ql/src/change-notes/2022-02-04-local-temp-file-or-directory-information-disclosure.md

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


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

java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.qhelp

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

JLLeitschuh · 2022-02-14T21:57:43Z

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

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

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

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

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

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

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

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

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

Choose a reason for hiding this comment

Uh oh!

felicitymay Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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