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: prevent extracting archived files outside of target path #59

Merged
merged 1 commit into from Apr 16, 2018

Conversation

Projects
None yet
2 participants
@aviadatsnyk
Copy link

aviadatsnyk commented Apr 16, 2018

Why this PR?

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 creation of evil.txt file in /tmp folder.

fs.createReadStream('zip-slip.zip').pipe(unzipper.Extract({ path: '/tmp/safe' }));

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

@aviadatsnyk aviadatsnyk force-pushed the aviadatsnyk:fix/zip-slip branch 5 times, most recently from aee680d to 5ff422a Apr 16, 2018

@aviadatsnyk aviadatsnyk force-pushed the aviadatsnyk:fix/zip-slip branch from 5ff422a to 5f68901 Apr 16, 2018

@aviadatsnyk

This comment has been minimized.

Copy link

aviadatsnyk commented Apr 16, 2018

Finally got a version that passes for all the node versions, so no more planned changes here.

// destination, or not extract it otherwise.
var extractPath = path.join(opts.path, entry.path);
if (extractPath.indexOf(opts.path) != 0) {
return;

This comment has been minimized.

@ZJONSSON

ZJONSSON Apr 16, 2018

Owner

This should either throw an error or at the very least show a warning.
Also the entry is left hanging here as nothing drains it (possibly use .autodrain)

@ZJONSSON ZJONSSON merged commit 2220ddd into ZJONSSON:master Apr 16, 2018

1 check passed

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

This comment has been minimized.

Copy link
Owner

ZJONSSON commented Apr 16, 2018

published as unzipper@0.8.13. Awesome catch, thank you!!

@ZJONSSON ZJONSSON referenced this pull request Jun 29, 2018

Closed

example request #65

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