Zip slip merge conflict fix #231
Conversation
|
Hey @petemoore, I've recently been granted review / approver status. |
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
|
Thanks @coolaj86, I've rebased the commits, and now am just waiting on the CI results... |
| @@ -94,7 +105,7 @@ func (r *Rar) Unarchive(source, destination string) error { | |||
| break | |||
| } | |||
| if err != nil { | |||
| if r.ContinueOnError { | |||
| if r.ContinueOnError || strings.Contains(err.Error(), "illegal file path") { | |||
There was a problem hiding this comment.
I'd prefer that we expose ErrIllegalPath and check against that, however, I'm accepting this as-is and will save that for a later commit.
|
Thanks @petemoore. I'm approving this, merging, and releasing. HOWEVER, I'm not sure that the vulnerability is 100% fixed. From what I can see it doesn't look like you did any symlink checking. Imagine a tar file with these contents: [symlink] demifile =>/etc/passwd
[file] demifile PWNed!I could put |
Thanks @coolaj86. I'm also not sure this fixes everything; it is simply #203 rebased with merge conflicts resolved, but hopefully we're a step closer. That PR was made by someone else, but eventually conflicted with changes on master, so my contribution was just about resolving those conflicts so that somebody could take a look at the changes to see if they were ok (you'll see I'm not the author of any of the commits here). |
|
@petemoore Do you think you could take on #242 ? I'm an "approver" and a "releaser", but I don't actually have direct commit access and the other approvers are inactive... so if you can come up with a solution, I can get it in :) |
|
And thank you very much for your help thus far. 😃 |
This is #203 plus a commit to resolve the merge conflict with master branch.