[SECURITY] Fix Temporary Directory Hijacking or Information Disclosure Vulnerability

[JLLeitschuh]

Security Vulnerability Fix

This pull request fixes either 1.) Temporary Directory Hijacking Vulnerability, or 2.) Temporary Directory Information Disclosure Vulnerability, which existed in this project.

Preamble

The system temporary directory is shared between all users on most unix-like systems (not MacOS, or Windows). Thus, code interacting with the system temporary directory must be careful about file interactions in this directory, and must ensure that the correct file permissions are set.

This PR was generated because the following chain of calls was detected in this repository in a way that leaves this project vulnerable. File.createTempFile(..) -> file.delete() -> either file.mkdir() or file.mkdirs().

Impact

This vulnerability can have one of two impacts depending upon which vulnerability it is.

1. Temporary Directory Information Disclosure - Information in this directory is visable to other local users, allowing a malicious actor co-resident on the same machine to view potentially sensitive files.
2. Temporary Directory Hijacking Vulnerability - Same impact as 1. above, but also, ther local users can manipulate/add contents to this directory. If code is being executed out of this temporary directory, it can lead to local priviledge escalation.

Temporary Directory Hijacking

This vulnerability exists because the return value from file.mkdir() or file.mkdirs() is not checked to determine if the call succeeded. Say, for example, because another local user created the directory before this process.

File tmpDir = File.createTempFile("temp", ".dir"); // Attacker knows the full path of the directory that will be later created
// delete the file that was created
tmpDir.delete(); // Attacker sees file is deleted and begins a race to create their own directory before the java code.
// and makes a directory of the same name
// SECURITY VULNERABILITY: Race Condition! - Attacker beats java code and now owns this directory
tmpDir.mkdirs(); // This method returns 'false' because it was unable to create the directory. No exception is thrown.
// Attacker can write any new files to this directory that they wish.
// Attacker can read any files created within this directory.

Other Examples

• CVE-2021-20202 - Keycloak/Keycloak
• CVE-2020-27216 - eclipse/jetty.project

Temporary Directory Information Disclosure

This vulnerability exists because, although the return values of file.mkdir() or file.mkdirs() are correctly checked, the permissions of the directory that is created follows the default system uname settings. Thus, the directory is created with everyone-readable permissions. As such, any files/directories written into this directory are viewable by all other local users on the system.

File tmpDir = File.createTempFile("temp", ".dir");
tmpDir.delete();
if (!tmpDir.mkdirs()) { // Guard correctly prevents temporary directory hijacking, but directory contents are everyone-readable.
    throw new IOException("Failed to create temporary directory");
}

Other Examples

• CVE-2020-15250 - junit-team/junit
• CVE-2021-21364 - swagger-api/swagger-codegen
• CVE-2022-24823 - netty/netty
• CVE-2022-24823 - netty/netty

The Fix

The fix has been to convert the logic above to use the following API that was introduced in Java 1.7.

File tmpDir = Files.createTempDirectory("temp dir").toFile();

The API both created the directory securely, ie with a random, non-conflicting name, with directory permissions that only allow the currently executing user to read or write the contents of this directory.

:arrow_right: Vulnerability Disclosure :arrow_left:

:wave: Vulnerability disclosure is a super important part of the vulnerability handling process and should not be skipped! This may be completely new to you, and that's okay, I'm here to assist!

First question, do we need to perform vulnerability disclosure? It depends!

1. Is the vulnerable code only in tests or example code? No disclosure required!
2. Is the vulnerable code in code shipped to your end users? Vulnerability disclosure is probably required!

Vulnerability Disclosure How-To

You have a few options options to perform vulnerability disclosure. However, I'd like to suggest the following 2 options:

1. Request a CVE number from GitHub by creating a repository-level GitHub Security Advisory. This has the advantage that, if you provide sufficient information, GitHub will automatically generate Dependabot alerts for your downstream consumers, resolving this vulnerability more quickly.
2. Reach out to the team at Snyk to assist with CVE issuance. They can be reached at the Snyk's Disclosure Email.

Detecting this and Future Vulnerabilities

This vulnerability was automatically detected by GitHub's LGTM.com using this CodeQL Query.

You can automatically detect future vulnerabilities like this by enabling the free (for open-source) GitHub Action.

I'm not an employee of GitHub, I'm simply an open-source security researcher.

Source

This contribution was automatically generated with an OpenRewrite refactoring recipe, which was lovingly hand crafted to bring this security fix to your repository.

The source code that generated this PR can be found here: UseFilesCreateTempDirectory

Opting-Out

If you'd like to opt-out of future automated security vulnerability fixes like this, please consider adding a file called .github/GH-ROBOTS.txt to your repository with the line:

User-agent: JLLeitschuh/security-research
Disallow: *

This bot will respect the ROBOTS.txt format for future contributions.

Alternatively, if this project is no longer actively maintained, consider archiving the repository.

CLA Requirements

This section is only relevant if your project requires contributors to sign a Contributor License Agreement (CLA) for external contributions.

It is unlikely that I'll be able to directly sign CLAs. However, all contributed commits are already automatically signed-off.

The meaning of a signoff depends on the project, but it typically certifies that committer has the rights to submit this work under the same license and agrees to a Developer Certificate of Origin (see https://developercertificate.org/ for more information).

- Git Commit Signoff documentation

If signing your organization's CLA is a strict-requirement for merging this contribution, please feel free to close this PR.

Sponsorship & Support

This contribution is sponsored by HUMAN Security Inc. and the new Dan Kaminsky Fellowship, a fellowship created to celebrate Dan's memory and legacy by funding open-source work that makes the world a better (and more secure) place.

This PR was generated by Moderne, a free-for-open source SaaS offering that uses format-preserving AST transformations to fix bugs, standardize code style, apply best practices, migrate library versions, and fix common security vulnerabilities at scale.

Tracking

All PR's generated as part of this fix are tracked here: https://github.com/JLLeitschuh/security-research/issues/10

[JLLeitschuh]

Does this fix a valid security vulnerability? Do you need assistance with public vulnerability disclosure?

[ianroberts]

The NameBearerHandle one is theoretically exploitable - it’s code in the desktop GUI to export an application in a form that can be sent to someone else or embedded in a network service, but this code itself is not something that could be activated remotely.

[greenwoodma]

Apologies that I didn't leave a comment when merging the PR. I merged it mostly as the code is cleaner and removes any chance of it being exploited. As @ianroberts points out even if the code in NameBearerHandle could be exploited it wouldn't be remotely exploitable. More importantly I think it would be extremely difficult (I won't say impossible) to exploit this code.

Essentially the code in NameBearerHandle creates a temporary directory, saves a copy of the GATE application (essentially an XML file and some resources) into the folder, and then creates a zip file from the folder contents. If a malicious process wanted to corrupt/modify the GATE application it would need to somehow edit the application file (or one of the resources) after they were written but before the zip file was created. Timing that would be rather tricky, and you'd need to do it in that order. If you wrote a dodgy application file as soon as the directory was created then either GATE would overwrite it, or there would be an IOException if it couldn't write the files. A malicious process could write other random files into the folder but they would never be loaded into GATE when the application was sent to another user or embedded into a network service.

As for leaking files to other local users, the temporary directory was being deleted as soon as the zip file had been created so was very short lived. Given it's unlikely any sensitive data was ever being stored within a GATE application it's difficult to view this as a security issue.

[JLLeitschuh]

Timing that would be rather tricky, and you'd need to do it in that order.

I'd just like to point out that the temporary directory hijacking vulnerability that exists is also hard to achieve.

If you think that there's even a remote possibly that this vulnerability could be exploited, I encourage you to reach out to the sky team to ask for a CVE. If you're adamant that this does not have a security impact, I'll happily agree to your decision here, but if you think there is any risk here, it would be good to disclose it properly.

[greenwoodma]

After careful consideration we’ve come to the conclusion that this isn’t a security issue that warrants disclosure via a CVE. There are two main reasons we have come to this conclusion.

Firstly, exploiting the issue (i.e. injecting malicious code into the zip file containing a GATE application) would be extremely difficult. Simply timing the attack correctly (as discussed above) is only part of the problem.

More importantly, the issue only occurs through a GUI action when using GATE Developer. For complicated historical reasons the code for the GATE Developer GUI and the underlying gate-core API library are tightly intertwined in a single JAR, but the issue you have highlighted does not affect other software that depends on gate-core as a library, only the Developer GUI itself. There is no benefit to issuing things like dependabot alerts to API consumers since the classes within the JAR that downstream consumers can use are not affected and so the CVE would essentially be redundant.

To summarise:

1. The only people who could theoretically be impacted by the issue would be those running the GUI locally on their computer.
2. These users would not be reached automatically by a CVE on the gate-core library.
3. As such we feel a better approach would be to document the issue in the GATE Developer change log along with the relevant details. These will then be visible within the latest SNAPSHOT build of GATE Developer.

[JLLeitschuh]

To be clear, CVEs are not just for Dependabot. They are industry standards that assist all industry security vendors to alert software maintainers and security teams across the industry about a security vulnerability. These also apply to vulnerabilities in end-user software that is not managed by dependency managers.