[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

➡️ 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

[JLLeitschuh]

Hi @krmahadevan & @juherr,

Do you believe this fixed a valid security vulnerability? Do you need assistance with vulnerability disclosure and CVE issuance?

[juherr]

@JLLeitschuh I've no idea if it fixes somethings... but I know it didn't break things 😉

[JLLeitschuh]

What files were being unzipped? Something that could be controlled by a user of TestNG, or was the file always something that could be verified to be safe?

[juherr]

The files are xml that are directly parsed

[JLLeitschuh]

Can you elaborate a bit more here, I don't understand where the XML files come from

[juherr]

It comes from a jar produced by the user and available in the classpath at runtime during the tests run.

[JLLeitschuh]

So, this file could theoretically come from an untrusted source? In that case, I would probably consider this a security vulnerability worth issuing a CVE number for.

Can you detail the scenario under which this logic is executed? I can reach out and have snyk issue a CVE number from there for you.

[ajakk]

So, this file could theoretically come from an untrusted source? In that case, I would probably consider this a security vulnerability worth issuing a CVE number for.

What makes you think "a jar produced by the user" is an untrusted source?

Can you detail the scenario under which this logic is executed? I can reach out and have snyk issue a CVE number from there for you.

This is CVE-2022-4065, issued by VulDB.

[JLLeitschuh]

Vulndb doesn't issue CVE numbers AFAIK.

Someone else must have done this

[JLLeitschuh]

Do you believe the contents of the vulnerability disclosure as it reads currently is accurate?

[ajakk]

They very certainly did, and they are indeed a CNA.

Based on the conversation here, mainly that the file that would exploit this vulnerability comes from the user, I do not agree that this is a 'critical' vulnerability which can be executed remotely, at least not without caveats. Though, correct me if I'm wrong.

[JLLeitschuh]

If it can be exploited remotely, Zip Slip is considered critical in most scenarios because it can be used to achieve remote code execution. Remote code execution is possible because zip slip allows an attacker to write the contents of executable files into executed files/directories. For example, on windows there are files that, if placed in the correct directory, will automatically execute on login.

[JLLeitschuh]

They very certainly did, and they are indeed a CNA.

I stand corrected, and I learned something new this evening.

Just a FYI, this is going to trigger a massive number of PRs from Dependabot, probably tomorrow.

This is why I asked the TestNG maintainers to be involved in the disclosure process multiple times. Because now someone else (Vuln DB) has come along and now controls the messaging going out about this vulnerability. Ideally this information should be coming from the maintainers directly to help provide the most clear information.

[ajakk]

If it can be exploited remotely

Very big 'if'.

If you're worried about coordinated disclosure, why don't you begin by coordinating disclosure yourself rather than apparently automatedly filing PRs to fix vulnerabilities that you can't be sure are actually exploitable?

[ajakk]

In any case, if anyone thinks the CVE is inaccurate, they can ask VulDB to correct it. They are receptive to feedback in my experience. In the worst case, the CVE still informs the public about the vulnerability and the patch for it even if the CVE description is less than accurate, so I'm not that concerned about it.

[JLLeitschuh]

If you're worried about coordinated disclosure, why don't you begin by coordinating disclosure yourself rather than apparently automatedly filing PRs to fix vulnerabilities that you can't be sure are actually exploitable?

Vaild response. Touché. Given the number of cases of this vulnerability across open source (GitHub's CodeQL flags 900+ cases), and I'm attempting to actually fix these vulnerabilities at scale, (I've automatically generated 165 PRs for Zip Slip alone so far) it's unfortunately impractical to attempt to coordinate disclosure at that kind of scale.

I've often thought about attempting to automate the CVE issuance process, but I haven't yet, and I'm beginning to think that having a human in the loop in the general case for CVE issuance is better than attempting full automation.

I see what you're saying, and I welcome suggestions.

[bradh]

It would be useful if a release incorporating this change could be made. I am not sure I see this security fix as a critical issue, but Github has started spamming maintainers of packages that use testng whenever they push. Realise that its a pain to do so, but 7.6.2 would be appreciated.

Thanks for the work on testng, whatever you decide to do.

[krmahadevan]

@bradh -We have done a release candidate build release and some of our early adopters are helping out with vetting it out as we speak. Once I hear back on them the release 7.7.0 will be done.

Please check here #2665 (comment) for more details.

[jeffreytolar]

Is this really a security issue? It appears that this method is only used for handling the -testjar flag. I haven't used -testjar personally, but the docs (https://testng.org/doc/documentation-main.html#running-testng) indicate that "[...] all the test classes found in this jar file will be considered test classes."

If an attacker can feed a malicious jar into -testjar, they've already won. They don't need to craft a special zip file with a bad file header that causes a write to a file on disk that will executed at some random time in the future; they can just include a malicious test in said jar (which testng will happily run immediately).

(There's some discussion above, but not really a definite "this isn't a real issue")

Ref: https://github.com/cbeust/testng/blob/c0e1e772f1fc0ab2142f3a4114a2b8cfe60fa7e1/testng-core/src/main/java/org/testng/TestNG.java#L414-L420; m_jarPath in that context is fed from https://github.com/cbeust/testng/blob/c0e1e772f1fc0ab2142f3a4114a2b8cfe60fa7e1/testng-core/src/main/java/org/testng/TestNG.java#L1641

---

Did some testing with -testjar (using Java 8, since that's what I happen to have installed right now):

<!-- pom.xml -->
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
  xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
  <modelVersion>4.0.0</modelVersion>

  <groupId>test</groupId>
  <artifactId>test</artifactId>
  <version>1.0</version>

  <properties>
    <maven.compiler.source>1.8</maven.compiler.source>
    <maven.compiler.target>1.8</maven.compiler.target>
  </properties>

  <dependencies>
	  <dependency>
		  <groupId>org.testng</groupId>
		  <artifactId>testng</artifactId>
		  <version>7.5</version>
	  </dependency>
  </dependencies>
</project>

// src/main/java/EvilTest.java
import org.testng.annotations.Test;

public class EvilTest {
	@Test public void doEvil() {
		System.out.println("Hello, World!");
	}
}

$ mvn package exec:java -Dexec.mainClass=org.testng.TestNG -Dexec.arguments="-testjar,target/test-1.0.jar"
<snip>
[INFO] --- exec-maven-plugin:3.1.0:java (default-cli) @ test ---
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
Hello, World!

[krmahadevan]

@bradh - Just letting you know. TestNG 7.7.0 which contains this fix, is released to maven central.

[starnowski]

Are there any plans to add a fix to the 7.5 release?

[krmahadevan]

@starnowski - Not sure if we would be doing a patch for the earlier released versions. This is fixed in 7.7.0 Can you please upgrade to that instead ?

[starnowski]

Hmm, currently one of our projects is built by maven and compiled with the java version 8. Fortunately, a project that uses the testng is a separate module that contains only tests and has a dependency on the main code. So I guess I could use a toolchain and change the compilation version to 11. Although I guess my case might not be the only one that uses java 8 in the project and in some cases it might be harder to update the java version.

[starnowski]

Okay, I was able to solve my problem by using the maven toolchain (project pull-request) but I would still considered adding a patch to the testng version that works with java 8.

[cherylking]

Okay, I was able to solve my problem by using the maven toolchain (project pull-request) but I would still considered adding a patch to the testng version that works with java 8.

+1 to this request. We have plugins/tools that are required to support Java 8 still. When I tried to move up to 7.7.0 it broke our Java 8 build/tests. Can this be ported to the 7.5 stream please?

[juherr]

@cherylking I understand but we don't have enough free time or contributors to support many version branches.
But any contribution is welcome and we will enjoy helping if you or your company want to assume the support of a dedicated version branch.

[ljacomet]

Hey folks,

The CVE behind this issue was reported against Gradle build tool. We took the time to analyze it and you can read our findings.

tl;dr:

• Only applicable to TestNG [6.13, 7.6.1]
• Severity feels really inflated, planning to follow up on this. Conclusion in agreement with the comment from @jeffreytolar

[juherr]

Hi @ljacomet
Thanks for the analyze. You confirm what we supposed.

The good part is it forced some users to upgrade 😅
I'm glad to see you plan to upgrade too 👍 gradle/gradle#24930

Could you explain why testng is embedded and not used from the classpath?
I remember an issue where it was a problem (@krmahadevan do you have the issue number in mind?)

[krmahadevan]

@ljacomet - Here's the issue that @juherr is referring to gradle/gradle#10507

This got closed automatically because it turned stale :(

And here's another one that I have created in TestNG, which is also due to Gradle bringing in an old version of TestNG as an embedded dependency

#2766