Prevent arbitrary file overwrite via path traversal [CVE-2019-10743] #169
Conversation
|
Thanks, but this will need test cases to prove it works before it will be considered, and the CI tests will need to pass as well. |
the extraction process now continues with other files by default even if there is a dot-dot filename, which is of course skipped. The test evilarchives have been generated with following script: https://github.com/giuliocomi/evilarchiver Command: $ mkdir safedir $ touch security_test.txt safedir/safefile $ python2 evilarchiver.py -e security_test.txt -n ../evilfile -s safedir/safefile
|
I have added some test cases in archiver_test.go.
|
giuliocomi
changed the title
|
I want to use this package in krew.dev to simplify archive download and unpack but this security issue is a show stopper. If the walker was exposing path information, it could be done where it's used. Is it possible to give a bump to this PR? Happy to help. Until PR is in, I will go with header inspection. |
| return fmt.Errorf("illegal file path: %s", filename) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
It seems this functionality is duplicated, better to extract to a separate function and call it.
Might be better to define it on struct{} and inline into formats.
|
@ferhatelmas and @giuliocomi. I have reopened this in #203, rebased against master and with a few fixes to pass lint build. Keen to get this in as I need the CVE closed for another project I am working on Embedded-Postgres Let me know thoughts? Also big thanks to @mholt for all the great work in this project already 👍 |
|
Hi @giuliocomi, I've recently been given the almighty approver privileges and I'd like to get this pushed through. I'm familiar with this CVE and how dangerous it is, and I've actually fixed it myself in a node library a while back. I've reviewed the changes and they look great. Could you possibly rebase this on the current master? If so I'll approve and merge it right away, and I'll include it in the upcoming release. |
|
Merged via #231 |
This PR is meant to patch the current version 3.1.1 against Zip Slip attacks (https://github.com/snyk/zip-slip-vulnerability).
This issue and patch are part of the vulnerability disclosure program of Snyk (more info here https://snyk.io/docs/security/).
Brief summary: In April 2018 a pull request (#65) was made by Snyk's security researcher and was integrated in version 2.1. After the refactoring of the whole project in version 3.0 (November 2018) the security fix was undone, reintroducing therefore the vulnerability till now.
A security note in the current README (fea250a#diff-04c6e90faac2675aa89e2176d2eec7d8) suggests the 'users' to implement their own security control with Walkers but this approach might be error prone and introduce a new kind of security issue - TOCTOU race conditions - that may allow to bypass the proposed path traversal prevention with Walkers.
Furthermore, I strongly believe that a security check must be available by design and be transparent in its usage.
Special thanks to Karen Yavine Shemesh of Snyk's security team for her responsiveness and coordination of this disclosure.