Skip to content
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

SECURITY: vulnerable to zip-slip (possible remote code execution/file overwrite) #232

Closed
aviadatsnyk opened this issue Jun 11, 2018 · 3 comments

Comments

@aviadatsnyk
Copy link

commented Jun 11, 2018

Background

https://github.com/snyk/zip-slip-vulnerability

Steps to reproduce

  1. get a malicious zip file, i.e. https://gist.github.com/grnd/61e3244bfff85cbe025f5bd4c60ba842
  2. unpack with new FastZip().ExtractZip(@"zip-slip-win.zip", @"c:\target", null);
  3. profit

Expected behavior

either the whole extraction should be stopped, or the files that are being extracted outside of the destination folder should not be extracted.

Actual behavior

All files are extracted.

Version of SharpZipLib

All

Obtained from (place an x between the brackets for all that apply)

  • Compiled from source
  • branch: all
  • commit: any
  • Downloaded DLL from GitHub
  • Downloaded DLL from SourceForge
  • Downloaded DLL from _______
  • DLL included as part of
  • Package installed using:
  • NuGet
  • MyGet
  • Chocolatey

Reporters

Yusuke Fujiwara - @yfakariya
Yann McCready, Genetec

@piksel

This comment has been minimized.

Copy link
Member

commented Jun 11, 2018

I am well aware of "Zip Snip" as it's an exploit as old as the zip format.

The behaviour is currently as intended, although one might argue that disallowing parent directory references in paths and symlinks should be done by default since it prevents (some) misuse.

The root issue is still:
If you do not trust the file source, you should not be blindly extracting it in the first place.

@aviadatsnyk

This comment has been minimized.

Copy link
Author

commented Jun 11, 2018

Hey @piksel - indeed this is a feature of the zip format, that has security implications.
As you wrote, it is widely accepted by many zip extraction implementation and libraries that this behaviour should be limited, a partial list of them is in snyk/zip-slip-vulnerability and many others are already limiting this behaviour and are therefore not listed there.

Of course, all of these do not mean this library has to have the same view. However, my opinion is that limiting this dangerous feature is the norm, and so any design choice that is significantly different should be clearly communicated to users. Having corresponded with @christophwille - he suggested I open this issue so users are aware of it.

Personally, I would also suggest re-considering the decision to have this as the intended behaviour, since many users do extract zip files from untrusted sources, and some are using this library to do so. In many cases, this vulnerability could lead to remote code execution. A less extreme decision might be to include this either as a warning or a clarification in the README.

@jfreilly

This comment has been minimized.

Copy link
Member

commented Jun 13, 2018

piksel added a commit to piksel/SharpZipLib that referenced this issue Jun 17, 2018
Use new parameter allowParentTraversal to re-enable past behaviour
Added new explicit exception for invalid names
Fixes icsharpcode#232
shalupov added a commit to JetBrains/SharpZipLib that referenced this issue Jun 24, 2018
Use new parameter allowParentTraversal to re-enable past behaviour
Added new explicit exception for invalid names
Fixes icsharpcode#232
shalupov added a commit to JetBrains/SharpZipLib that referenced this issue Jun 24, 2018
Use new parameter allowParentTraversal to re-enable past behaviour
Added new explicit exception for invalid names
Fixes icsharpcode#232
@piksel piksel closed this in 5376c2d Jul 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.