[SECURITY] Fix Zip Slip Vulnerability

[JLLeitschuh]

Security Vulnerability Fix

This pull request fixes a Zip Slip vulnerability either due to an insufficient, or missing guard when unzipping zip files.

Even if you deem, as the maintainer of this project, this is not necessarily fixing a security vulnerability, it is still, most likely, a valid security hardening.

Preamble

Impact

This issue allows a malicious zip file to potentially break out of the expected destination directory, writing contents into arbitrary locations on the file system. Overwriting certain files/directories could allow an attacker to achieve remote code execution on a target system by exploiting this vulnerability.

Why?

The best description of Zip-Slip can be found in the white paper published by Snyk: Zip Slip Vulnerability

But I had a guard in place, why wasn't it sufficient?

If the changes you see are a change to the guard, not the addition of a new guard, this is probably because this code contains a Zip-Slip vulnerability due to a partial path traversal vulnerability.

To demonstrate this vulnerability, consider "/usr/outnot".startsWith("/usr/out"). The check is bypassed although /outnot is not under the /out directory. It's important to understand that the terminating slash may be removed when using various String representations of the File object. For example, on Linux, println(new File("/var")) will print /var, but println(new File("/var", "/") will print /var/; however, println(new File("/var", "/").getCanonicalPath()) will print /var.

The Fix

Implementing a guard comparing paths with the method java.nio.files.Path#startsWith will adequately protect against this vulnerability.

For example: file.getCanonicalFile().toPath().startsWith(BASE_DIRECTORY) or file.getCanonicalFile().toPath().startsWith(BASE_DIRECTORY_FILE.getCanonicalFile().toPath())

Other Examples

• CVE-2018-1002201 - zeroturnaround/zt-zip
• CVE-2018-1002202 - srikanth-lingala/zip4j
• CVE-2018-8009 - apache/hadoop

: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!

For partial path traversal, consider if user-supplied input could ever flow to this logic. If user-supplied input could reach this conditional, it's insufficient and, as such, most likely a vulnerability.

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. Note: Please include JLLeitschuh Disclosure in the subject of your email so it is not missed.

Detecting this and Future Vulnerabilities

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 handcrafted to bring this security fix to your repository.

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

Why didn't you disclose privately (ie. coordinated disclosure)?

This PR was automatically generated, in-bulk, and sent to this project as well as many others, all at the same time.

This is technically what is called a "Full Disclosure" in vulnerability disclosure, and I agree it's less than ideal. If GitHub offered a way to create private pull requests to submit pull requests, I'd leverage it, but that infrastructure, sadly, doesn't exist yet.

The problem is that, as an open source software security researcher, I (exactly like open source maintainers), I only have so much time in a day. I'm able to find vulnerabilities impacting hundreds, or sometimes thousands of open source projects with tools like GitHub Code Search and CodeQL. The problem is that my knowledge of vulnerabilities doesn't scale very well.

Individualized vulnerability disclosure takes time and care. It's a long and tedious process, and I have a significant amount of experience with it (I have over 50 CVEs to my name). Even tracking down the reporting channel (email, Jira, etc..) can take time and isn't automatable. Unfortunately, when facing problems of this scale, individual reporting doesn't work well either.

Additionally, if I just spam out emails or issues, I'll just overwhelm already over-taxed maintainers, I don't want to do this either.

By creating a pull request, I am aiming to provide maintainers something highly actionable to actually fix the identified vulnerability; a pull request.

There's a larger discussion on this topic that can be found here: https://github.com/JLLeitschuh/security-research/discussions/12

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/16

[TheCurle]

I appreciate the automated process that went into this, but ForgeGradle is effectively ACE As A Modding Toolchain, so this isn't an issue for us whatsoever.

[JLLeitschuh]

I'd still consider merging it for correctness and for proper validation, and to make static code analysis tools happy

[SizableShrimp]

The code in question is only run in rare circumstances and must be initiated by users. The inputs provided by ForgeGradle itself are always autogenerated and don't really have a risk of producing this vulnerability. However, would your static code analysis tools be happy if the line were moved one line lower?

[LexManos]

We typically do not accept these academic/automated/fearmongering type of requests. If we were to accept it, the change would be done by one of our team. As I am morally against legitimizing these bots. As they are essentially spam bots in my opinion. This is not meant as an insult to you or anyone in that project. I just don't like these systems. On top of that, as Shrimp explained there is no possibility of this section of code triggering the vulnerability you described as all the input values are generated by us elsewhere in the codebase. This is the major flaw of these bots, they have zero idea about context.

[JLLeitschuh]

However, would your static code analysis tools be happy if the line were moved one line lower?

Yes. As long as the guard is present in some way between the getEntry and the creation of the output stream, the static code analysis tool would be happy.

We typically do not accept these academic/automated/fearmongering type of requests. If we were to accept it, the change would be done by one of our team. As I am morally against legitimizing these bots. As they are essentially spam bots in my opinion. This is not meant as an insult to you or anyone in that project. I just don't like these systems.

The goal is to be a highly effective way of Scaling security researchers to fix widespread common security vulnerabilities across the open source software ecosystem. The hope is that any fix, will at-minimum be a valid security hardening.

I get that you don't like bots, that's why this pull request was created by my personal account. This way, there's a real person behind the change you can discuss your concerns with.

I also don't want to imply that there's absolutely a security vulnerability in any project I report to. My goal with the message I include is to give maintainers the full information they require to make an assessment about whether a vulnerability reported this way is legitimate or not and decide if vulnerability disclosure is needed or not. In your case, I think your understanding of your own code, and the assessment you've made in that context is appropriate and sufficient.

[marchermans]

I also don't want to imply that there's absolutely a security vulnerability in any project I report to. My goal with the message I include is to give maintainers the full information they require to make an assessment about whether a vulnerability reported this way is legitimate or not and decide if vulnerability disclosure is needed or not. In your case, I think your understanding of your own code, and the assessment you've made in that context are appropriate and sufficient.

I personally find this a weird statement. From the PR title alone your first sentence in that statement is wrong. If you did not want to imply that there are security vulnerabilities in any project you report to, then go about this in a different way. Ask in the community of said project first, they can likely tell you why that code is the way it is and if it is a real vulnerability in the first place.

Again I agree with what others already posted here. If you are serious about security you would learn about the context of the vulnerability and if there are other ways this issue is prevented or handled before you open a ticket which has 2 lines changed, a several-page PR message, without showing how it is actually abused.

[JLLeitschuh]

Ask in the community of said project first, they can likely tell you why that code is the way it is and if it is a real vulnerability in the first place.

The problem I'm facing is a matter of scale. For this vulnerability alone (Zip Slip) I've opened 100 pull requests. This year I've opened over 590 pull requests to fix security vulnerabilities across OSS. Across my OSS development history, I've crated over 5,600 pull requests to fix various security vulnerabilities. To do that asessment on a project-by-project basis can take months. I know this from experience. I did this with the first widespread security vulnerability I attempted to fix. https://infosecwriteups.com/want-to-take-over-the-java-ecosystem-all-you-need-is-a-mitm-1fc329d898fb

[JLLeitschuh]

I personally find this a weird statement. From the PR title alone your first sentence in that statement is wrong. If you did not want to imply that there are security vulnerabilities in any project you report to, then go about this in a different way.

I will reconsider my messaging around this. Thank you for the feedback. The title is intended to catch the attention of the maintainer so that they take a look because the PR may fix a security vulnerability. I lead with the impact so they understand the reason the PR was opened. If I led with the fact this PR was generated, they might dismiss a valid security vulnerability without understanding the potential vulnerability, which would defeat the purpose of the work.

If you have alternative messaging you'd line to suggest, the PR messages I use for this work are all open source and I'd be more than willing to review a pull request that modifies the messaging.

You can find the PR messages inside the repositories: https://github.com/JLLeitschuh/security-research/tree/main/bulk-pr-generation