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

fix: fail when trying to extract outside of dest dir #87

Merged
merged 1 commit into from May 6, 2018

Conversation

Projects
None yet
5 participants
@odinn1984
Copy link

odinn1984 commented May 6, 2018

This PR is meant to fix an arbitrary file write vulnerability, that can be achieved using a specially crafted zip archive, that holds path traversal filenames. When the filename gets concatenated to the target extraction directory, the final path ends up outside of the target folder.

A sample malicious zip file named zip-slip.zip (see this gist) was used, and when running the code below, resulted in the creation of /tmp/evil.txt outside of the intended /tmp/safe target.

File testZip = new File("zip-slip.zip");
File targetDirectory = new File("/tmp/safedir");
    
ZipUnArchiver zu = getZipUnArchiver( testZip );
zu.extract( "", targetDirectory );

There are various possible ways to avoid this issue, some include checking for .. (dot dot) characters in the filename, but the best solution in our opinion is to check if the final target filename, starts with the target folder (after both are resolved to their absolute path).

Stay secure,
Snyk Team

Odinn Odinn
fix: fail when trying to extract outside of dest dir
A well crafted zip file may cause the code to extract outside of the destination dir.
This PR fails when that happens so that no unexpected behaviour happens.

@khmarbaise khmarbaise self-requested a review May 6, 2018

@khmarbaise khmarbaise added this to the 3.6.0 milestone May 6, 2018

@khmarbaise khmarbaise merged commit f8f4233 into codehaus-plexus:master May 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jpederzolli

This comment has been minimized.

Copy link

jpederzolli commented on 58bc24e Jun 4, 2018

This change breaks cases where the maven-dependency-plugin is leveraged to intentionally unpack an archive to an arbitrary directory -- was this intended?

This comment has been minimized.

Copy link
Member

michael-o replied Jun 4, 2018

I consider this not being our task to be checked. Plexus Archiver does sole unpacking. No semantics.

This comment has been minimized.

Copy link
Member

plamentotev replied Jun 6, 2018

I think the purpose of the fix is to not allow files from the archive to be written outside the destination directory. Otherwise a maliciously crafted archive may cause files to be extracted in arbitrary (potentially dangerous) location. @jpederzolli I'm not sure I understand what you mean. Is there a use case where you want to unpack files outside the destination directory? Could you please give an example? Or even better - open an issue so we can better track it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment