-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Java: CWE-200: Temp directory local information disclosure vulnerability #4388
Java: CWE-200: Temp directory local information disclosure vulnerability #4388
Conversation
7cdf997
to
d53d77c
Compare
* @kind problem | ||
* @problem.severity warning | ||
* @precision very-high | ||
* @id java/local-information-disclosure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This id
is rather generic. Could you make it a bit more specific to this query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, I think
java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure2.ql
Outdated
Show resolved
Hide resolved
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. |
Here's one that I found that was just disclosed. |
@JLLeitschuh So if Junit 4.13.1 can patch this fix why can't Guava? |
@melloware 🤷♂️ bring it up with the Guava & Google team. There's only so much I can do. |
Can we submit a PR against that ticket for review making the same fix Junit did? |
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: It's probably not that useful to have this discussion here, on this unrelated issue. |
java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure2.ql
Outdated
Show resolved
Hide resolved
exists(MethodAccess ma | | ||
ma.getMethod() instanceof MethodFileSystemFileCreation and | ||
ma.getQualifier() = this.asExpr() | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could write:
exists(MethodAccess ma | | |
ma.getMethod() instanceof MethodFileSystemFileCreation and | |
ma.getQualifier() = this.asExpr() | |
) | |
exists(MethodFileSystemFileCreation ma | | |
ma.getQualifier() = this.asExpr() | |
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not quite right. MethodFileSystemFileCreation
is a Method
not a MethodAccess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I don't think I can change this to be simpler at all.
java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure2.ql
Outdated
Show resolved
Hide resolved
There are some outstanding disclosures and CVES from this query. Hence why development is kinda frozen. I'm still leveraging it to report vulns. |
1b216f8
to
b1aeb99
Compare
e53cfd8
to
6deb3d8
Compare
Test run of this query: |
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 |
Good point 😂 |
Assigned @aibaars since he has been reviewing this and moved to In Progress state |
False positives this is finding (the first path). However, other paths are valid.
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 |
Looks like you're still tweaking this; let me know when you're done? |
java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql
Outdated
Show resolved
Hide resolved
I'm wondering if you have any insights into how to fix the problem I described here: |
…sure.ql Co-authored-by: Chris Smowton <smowton@github.com>
I think I have a fix here that I'm happy with: |
java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql
Outdated
Show resolved
Hide resolved
Co-authored-by: Chris Smowton <smowton@github.com>
@smowton I'm happy with this PR as-is |
There's a real test failure to fix |
@smowton indeed! Fixed that. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing help for your query. A few minor comments, but generally looking good 🙂
java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.qhelp
Show resolved
Hide resolved
java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,258 @@ | |||
/** | |||
* @name Temporary Directory Local information disclosure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this might be clearer. The query name should be in sentence case.
* @name Temporary Directory Local information disclosure | |
* @name Local information disclosure in a temporary directory |
--- | ||
category: newQuery | ||
--- | ||
* A new query titled "Temporary directory Local information disclosure" (`java/local-temp-file-or-directory-information-disclosure`) has been added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
Co-authored-by: Felicity Chapman <felicitymay@github.com>
Co-authored-by: Felicity Chapman <felicitymay@github.com>
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 |
This vulnerability detects the use of the local temporary directory in ways that potentially expose sensitive information to other users.
Some examples