You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geronimo.apache.org by GitBox <gi...@apache.org> on 2022/11/04 15:00:24 UTC

[GitHub] [geronimo-batchee] JLLeitschuh opened a new pull request, #12: [SECURITY] Fix Zip Slip Vulnerability

JLLeitschuh opened a new pull request, #12:
URL: https://github.com/apache/geronimo-batchee/pull/12

   
   # 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](https://snyk.io/research/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](https://snyk.io/vuln/SNYK-JAVA-ORGZEROTURNAROUND-31681) - zeroturnaround/zt-zip
    - [CVE-2018-1002202](https://snyk.io/vuln/SNYK-JAVA-NETLINGALAZIP4J-31679) - srikanth-lingala/zip4j
    - [CVE-2018-8009](https://nvd.nist.gov/vuln/detail/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](https://docs.github.com/en/code-security/repository-security-advisories/creating-a-repository-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](mailto:report@snyk.io). 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](https://github.com/github/codeql-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](https://github.com/openrewrite/rewrite) [refactoring recipe](https://docs.openrewrite.org/), 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](https://github.com/openrewrite/rewrite-java-security/blob/main/src/main/java/org/openrewrite/java/security/ZipSlip.java)
   
   ## 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](https://moz.com/learn/seo/robotstxt) format for future contributions.
   
   Alternatively, if this project is no longer actively maintained, consider [archiving](https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-archiving-repositories) 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/](https://developercertificate.org/) for more information).
   >
   > \- [Git Commit Signoff documentation](https://developercertificate.org/)
   
   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](https://www.moderne.io/), 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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@geronimo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [geronimo-batchee] fpapon merged pull request #12: [SECURITY] Fix Zip Slip Vulnerability

Posted by GitBox <gi...@apache.org>.
fpapon merged PR #12:
URL: https://github.com/apache/geronimo-batchee/pull/12


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@geronimo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [geronimo-batchee] JLLeitschuh commented on pull request #12: [SECURITY] Fix Zip Slip Vulnerability

Posted by GitBox <gi...@apache.org>.
JLLeitschuh commented on PR #12:
URL: https://github.com/apache/geronimo-batchee/pull/12#issuecomment-1306389721

   Friendly ping: @asfsecurity 
   
   Does this need a CVE assigned to it?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@geronimo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [geronimo-batchee] rmannibucau commented on pull request #12: [SECURITY] Fix Zip Slip Vulnerability

Posted by GitBox <gi...@apache.org>.
rmannibucau commented on PR #12:
URL: https://github.com/apache/geronimo-batchee/pull/12#issuecomment-1319639612

   Well there is no discussion yet - and to be honest "anywhere" does not mean "downloaded from a random source" but literally "put by an user there". So BatchEE does not take into account the zip creation of the zip it extracts but it must be put intentionally by end user there so if it is insecure the user intentionally put an insecure zip, similarly to put an attacker code voluntary in tomcat webapps folder. This is why I'm not sure it needs a CVE, we can create one in low level but not sure it would serve much end users.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@geronimo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [geronimo-batchee] JLLeitschuh commented on pull request #12: [SECURITY] Fix Zip Slip Vulnerability

Posted by GitBox <gi...@apache.org>.
JLLeitschuh commented on PR #12:
URL: https://github.com/apache/geronimo-batchee/pull/12#issuecomment-1303818287

   Hi @rmannibucau,
   
   This pull request, along with 164 similar pull request, was automatically generated at-scale to fix this vulnerability across the OSS java ecosystem. Unfortunately, given the scale of this vulnerability, and the number of projects to be fixed, I don't have the time to craft such a unit test. In general, I 100% agree with you about needing a unit test to verify a change before merging, and I fully support someone stepping in to assist with creating such a test, but I unfortunately don't have the time to do so myself.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@geronimo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [geronimo-batchee] rmannibucau commented on pull request #12: [SECURITY] Fix Zip Slip Vulnerability

Posted by GitBox <gi...@apache.org>.
rmannibucau commented on PR #12:
URL: https://github.com/apache/geronimo-batchee/pull/12#issuecomment-1307129384

   Theorically from anywhere but it should really come from a trusted source (same as webapps/ folder of tomcat if you are more familiar with it).
   
   There is also a pending thread to know if we keep the project or move it to something like "geronimo-"attic so can also change the downstream process, not sure when it will be decided since it lagged due to holidays but we should re-up it to ensure we don't work for nothing too.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@geronimo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [geronimo-batchee] raboof commented on pull request #12: [SECURITY] Fix Zip Slip Vulnerability

Posted by GitBox <gi...@apache.org>.
raboof commented on PR #12:
URL: https://github.com/apache/geronimo-batchee/pull/12#issuecomment-1306969667

   > Does this need a CVE assigned to it?
   
   This is up to the Apache Geronimo team to decide.
   
   It is not clear to me whether the zip files processed here are assumed come from a trusted source or not: without intimate knowledge of the project, it seems this function would be used to unzip archives with the goal of executing its contents. If that's the case, anyone who can produce these archives can perform arbitrary code execution by design, and the 'zip slip' does not provide any elevated privileges. Of course it is still a valuable change from the perspective of hardening, best practices, and avoiding non-security bugs, but without further analysis I don't necessarily see the reason for a CVE here. Perhaps you or the Geronimo team has additional background that would clarify that need, of course.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@geronimo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [geronimo-batchee] rmannibucau commented on pull request #12: [SECURITY] Fix Zip Slip Vulnerability

Posted by GitBox <gi...@apache.org>.
rmannibucau commented on PR #12:
URL: https://github.com/apache/geronimo-batchee/pull/12#issuecomment-1303811303

   Hi, agree that even if it is not exploitable by most env it is good to get it merged. Can you work on a test validating the PR fixes the issue before we integrate it please?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@geronimo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [geronimo-batchee] raboof commented on pull request #12: [SECURITY] Fix Zip Slip Vulnerability

Posted by GitBox <gi...@apache.org>.
raboof commented on PR #12:
URL: https://github.com/apache/geronimo-batchee/pull/12#issuecomment-1319681866

   > "anywhere" does not mean "downloaded from a random source" but literally "put by an user there". So BatchEE does not take into account the zip creation of the zip it extracts but it must be put intentionally by end user there so if it is insecure the user intentionally put an insecure zip, similarly to put an attacker code voluntary in tomcat webapps folder.
   
   That's indeed how I understood your previous comment. Based on this it doesn't look like the zip slip provides any additional privileges to an attacker (as an attacker that can control the zip already has full control, right?). If that's indeed the case I don't see a reason to issue a CVE. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@geronimo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [geronimo-batchee] fpapon commented on pull request #12: [SECURITY] Fix Zip Slip Vulnerability

Posted by GitBox <gi...@apache.org>.
fpapon commented on PR #12:
URL: https://github.com/apache/geronimo-batchee/pull/12#issuecomment-1304860368

   @rmannibucau I pushed your zip test example. I will merge this PR and update the test according to this PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@geronimo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [geronimo-batchee] JLLeitschuh commented on pull request #12: [SECURITY] Fix Zip Slip Vulnerability

Posted by GitBox <gi...@apache.org>.
JLLeitschuh commented on PR #12:
URL: https://github.com/apache/geronimo-batchee/pull/12#issuecomment-1320330093

   Sounds good, thank you all for the thorough assessment of the situation. I really appreciate it!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@geronimo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [geronimo-batchee] rmannibucau commented on pull request #12: [SECURITY] Fix Zip Slip Vulnerability

Posted by GitBox <gi...@apache.org>.
rmannibucau commented on PR #12:
URL: https://github.com/apache/geronimo-batchee/pull/12#issuecomment-1304766910

   @JLLeitschuh would it help if I give you this test (`tools/cli/src/test/java/org/apache/batchee/cli/zip/ZipsTest.java`):
   
   ```
   /*
    * Licensed to the Apache Software Foundation (ASF) under one or more
    * contributor license agreements.  See the NOTICE file distributed with
    * this work for additional information regarding copyright ownership.
    * The ASF licenses this file to You under the Apache License, Version 2.0
    * (the "License"); you may not use this file except in compliance with
    * the License.  You may obtain a copy of the License at
    *
    *     http://www.apache.org/licenses/LICENSE-2.0
    *
    * Unless required by applicable law or agreed to in writing, software
    * distributed under the License is distributed on an "AS IS" BASIS,
    * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    * See the License for the specific language governing permissions and
    * limitations under the License.
    */
   package org.apache.batchee.cli.zip;
   
   import org.junit.Rule;
   import org.junit.Test;
   import org.junit.rules.TemporaryFolder;
   
   import java.io.File;
   import java.io.IOException;
   import java.nio.charset.StandardCharsets;
   import java.nio.file.Files;
   import java.util.zip.ZipEntry;
   import java.util.zip.ZipOutputStream;
   
   import static org.junit.Assert.assertTrue;
   
   public class ZipsTest {
       @Rule
       public final TemporaryFolder temp = new TemporaryFolder();
   
       @Test
       public void zipSlip() throws IOException {
           final File zip = temp.newFile("test.zip");
           final String slipFile = "attack.txt";
   
           try (final ZipOutputStream out = new ZipOutputStream(Files.newOutputStream(zip.toPath()))) {
               out.putNextEntry(new ZipEntry("../" + slipFile));
               out.write("test".getBytes(StandardCharsets.UTF_8));
               out.closeEntry();
           }
   
           final File exploded = temp.newFolder("some/nested/folder");
           Zips.unzip(zip, exploded);
           assertTrue(new File(exploded.getParent(), slipFile).exists());
       }
   }
   ```
   
   You just need to replace the unzip+asserttrue by a `try {unzip(); fail()} catch(final RuntimeException re) { /*ok*/}`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@geronimo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [geronimo-batchee] JLLeitschuh commented on pull request #12: [SECURITY] Fix Zip Slip Vulnerability

Posted by GitBox <gi...@apache.org>.
JLLeitschuh commented on PR #12:
URL: https://github.com/apache/geronimo-batchee/pull/12#issuecomment-1319372016

   If the zip archive could come from anywhere, then it would be appropriate to issue a CVE here. Is there a discussion inside the ASF about this, or does one need to be kicked off?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@geronimo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org