Skip to content

[SECURITY] Fix Zip Slip Vulnerability #20

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

Open
wants to merge 331 commits into
base: master
Choose a base branch
from

Conversation

JLLeitschuh
Copy link

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

➡️ Vulnerability Disclosure ⬅️

👋 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: JLLeitschuh/security-research#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: JLLeitschuh/security-research#16

trespasserw and others added 30 commits September 27, 2017 16:23
Simplifies CI (the outer IDEA project no longer bundles JUnit and AssertJ .jars).
…esn't contain `con` which is reserved name too.
The option may cause valid constructor parameters to be mislabeled "synthetic" and removed from resulting code.
BartvHelvert and others added 23 commits February 1, 2022 09:46
…asses and lambdas

GitOrigin-RevId: 91681e75d23725104864a6e2258824bc57c63c80
GitOrigin-RevId: 7b8041506897e973a3046eb602e4bca7e54fe8ac
GitOrigin-RevId: 333993a458ecca8fec488fde1058347be06a628f
Moves filtering methods to static methods in `TargetInfo` implementations, cleans up the naming and adds docs.

GitOrigin-RevId: 9a3007f706cb911222b362c3907c5598ec4defa4
GitOrigin-RevId: 36f63468795c06c2fbd0b0f5b76c1f43d7258ae5
…tions

GitOrigin-RevId: 1c1876cdfc35e36cda0160ba265c1bbe235c6bba
Renames `opcode` to `id` and adds better documentation.

GitOrigin-RevId: f9304338846665a72228ade8a0929a823c9ed09e
Adds some documentation and improve naming.

GitOrigin-RevId: 7263dc0bb6b0a17b3d46e540312060bcde02a36b
…eeded

GitOrigin-RevId: 28fea216fe128684fc323fd1dfaa5608d27f0857
GitOrigin-RevId: 580783a580340714dea984ba574d5d35f7996478
Also changes public fields to properties.

GitOrigin-RevId: 7e626222f08b54501dda28f39361275d4dc9c9d5
GitOrigin-RevId: 9bcfbac2895706a5cc6410dcfe43185138988be7
GitOrigin-RevId: f48dba4f4c5108ef93ba18b8d9f7a807a7070ef0
GitOrigin-RevId: 8d921fabb65796df02fe9136ecd5d77d866c9681
GitOrigin-RevId: 5b7d8fbfca9d58fcde762c01e564b16be44d9c1f
…c classes

Fixes type annotation decompilation when the nested type contains references to static classes.

GitOrigin-RevId: e2d1f04975388eb7a5d45976ca5567c9b9e8b3f9
GitOrigin-RevId: 3e717826966a0112943c93eef38c52d122b3bf4b
…ed bytecode

GitOrigin-RevId: fcbbc78a08fa3c0afe7bdad129c0427007f0c3d5
(PR #1957)

GitOrigin-RevId: 4e06322e0a332ddade5d768793e78be2ae1a6ed8
GitOrigin-RevId: de0ec3095d0fb8c0b51b7217f710014f212e9c70
GitOrigin-RevId: 95072f47eac51ddf27d44e65809fd6a6ce6d26c1
(updating direct links and issue navigation templates)

GitOrigin-RevId: 4e3ee1bdd3e620f2ca57533977cedde18377857f
GitOrigin-RevId: a8471e470588d2d29ce8b5c902ba68c64feda8da
@Phoenixcwx
Copy link

Phoenixcwx commented Jul 29, 2022 via email

@JLLeitschuh JLLeitschuh force-pushed the fix/JLL/zip-slip-vulnerability branch 2 times, most recently from b79ee7f to 99de86b Compare July 30, 2022 00:13
This fixes a Zip-Slip vulnerability.

This change does one of two things. This change either

1. Inserts a guard to protect against Zip Slip.
OR
2. Replaces `dir.getCanonicalPath().startsWith(parent.getCanonicalPath())`, which is vulnerable to partial path traversal attacks, with the more secure `dir.getCanonicalFile().toPath().startsWith(parent.getCanonicalFile().toPath())`.

For number 2, 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`.

Weakness: CWE-22: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Severity: High
CVSSS: 7.4
Detection: CodeQL (https://codeql.github.com/codeql-query-help/java/java-zipslip/) & OpenRewrite (https://public.moderne.io/recipes/org.openrewrite.java.security.ZipSlip)

Reported-by: Jonathan Leitschuh <[email protected]>
Signed-off-by: Jonathan Leitschuh <[email protected]>

Bug-tracker: JLLeitschuh/security-research#16

Co-authored-by: Moderne <[email protected]>
@JLLeitschuh JLLeitschuh force-pushed the fix/JLL/zip-slip-vulnerability branch from 99de86b to 639c2ed Compare August 8, 2022 20:43
@BenCat07
Copy link

This is an unoffical mirror of the actual code, a PR here will not achieve anything, sorry to tell you. You may want to message their team Directly over the issue tracker linked in this Projects README

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.