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 extraction of archived files outside target path #65

Merged
merged 2 commits into from Apr 17, 2018

Conversation

Projects
None yet
5 participants
@aviadatsnyk
Copy link
Contributor

aviadatsnyk commented Apr 17, 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.

package main

import "log"
import "github.com/mholt/archiver"

func main() {
	err := archiver.Tar.Open("/tmp/evil-tar.tar", "/tmp/safe")
	if err != nil {
		log.Fatal(err)
	}
}

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:master branch from e8249d1 to da87881 Apr 17, 2018

@aviadatsnyk

This comment has been minimized.

Copy link
Contributor

aviadatsnyk commented Apr 17, 2018

I'll add a test next week, but if you can't wait, the idea is to extract the zip-slip.zip malicious zip file, and make sure a file /tmp/evil.txt was not created. I'm not sure how to do it in a way that would work for windows et. al.

@mholt
Copy link
Owner

mholt left a comment

Thanks! I have just one request, that we consolidate all the zip-slip logic into a single function which we then call from the individual extractors.

zip.go Outdated
destpath := filepath.Join(destination, zf.Name)
if !strings.HasPrefix(destpath, destination) {
return fmt.Errorf("%s: illegal file path", zf.Name)
}

This comment has been minimized.

@mholt

mholt Apr 17, 2018

Owner

Do you think this logic could be pulled into its own function so that we don't have to repeat it everywhere? It's simple, I know, but it would be nice to standardize it.

This comment has been minimized.

@aviadatsnyk

aviadatsnyk Apr 17, 2018

Contributor

Took a stab at it. Let me know what you think.
I'm not sure about the name sanitizeExtractPath, so if you have any idea - that could be great.

@mholt

mholt approved these changes Apr 17, 2018

@mholt mholt merged commit e4ef56d into mholt:master Apr 17, 2018

3 checks passed

ci/gopherci/pr Found no issues \ʕ◔ϖ◔ʔ/
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mholt

This comment has been minimized.

Copy link
Owner

mholt commented Apr 17, 2018

Excellent. I'm not too picky about that name, I think it's good. Thank you for contributing this!

A test would definitely be good to have, so if you have the time, I think that'd be important. Thanks ;)

@LiveOverflow

This comment has been minimized.

Copy link

LiveOverflow commented Jun 9, 2018

The fix is insufficient because you can use symlinks in .tar files to get around it.
The following PoC creates a symlink root and then tars it with a file that is referenced through that symlink. Upon unpacking it will first unpack the symlink, which will now point to the root filesystem, and then the second file can use it to write anywhere.

/tmp/zip $ ln -s / root
/tmp/zip $ echo oops > /tmp/ohoh
/tmp/zip $ tar -cf test.tar root/ root/tmp/ohoh
/tmp/zip $ tar -tf test.tar
root
root/tmp/ohoh
/tmp/zip $ rm /tmp/ohoh
/tmp/zip $ cat test.go
package main

import "github.com/mholt/archiver"

func main() {
    archiver.Tar.Open("test.tar", "test_out")
}

/tmp/zip $ ls /tmp/ohoh
ls: /tmp/ohoh: No such file or directory
/tmp/zip $ go run test.go
/tmp/zip $ ls /tmp/ohoh
/tmp/ohoh
/tmp/zip $ cat /tmp/ohoh
oops

I'm not sure what a good fix would be, as symlinks are a feature of .tar files. And it could break existing projects that rely on symlinks. Maybe a flag to enable symlinks would make sense, and have it off by default?

PS: Symlinks are theoretically also supported by .zip files, but archiver doesn't implement it (actually many zip libraries don't support them). So in case this support would be added in the future, this has to be considered as well.

edit: sorry, just noticed that there is already an open issue about that #69

@aviadatsnyk

This comment has been minimized.

Copy link
Contributor

aviadatsnyk commented Jun 10, 2018

@mholt if you need snyk's help with this - we're happy to chip in :D

@mholt

This comment has been minimized.

Copy link
Owner

mholt commented Jun 10, 2018

I would love some help. Been too busy to maintain this lately. Would anyone be able to commit a little time to improve this lib? I will make you a collaborator. :)

aviadatsnyk added a commit to aviadatsnyk/archiver that referenced this pull request Jun 11, 2018

aviadatsnyk added a commit to aviadatsnyk/archiver that referenced this pull request Jun 11, 2018

@aviadatsnyk

This comment has been minimized.

Copy link
Contributor

aviadatsnyk commented Jun 11, 2018

could you please review #70 ?

@paulcarlton

This comment has been minimized.

Copy link

paulcarlton commented Jun 12, 2018

Can you confirm this fix is in golang 10.3?

@paulcarlton

This comment has been minimized.

Copy link

paulcarlton commented Jun 13, 2018

cancel that, this is a separate project

@mattn

This comment has been minimized.

Copy link

mattn commented Jun 13, 2018

How this work with following files?

destination = "/foo/bar"

extracted file = "/foo/bar/../../../../etc/hosts"

I don't make sure but you have to call filepath.Clean() before checking.

@aviadatsnyk

This comment has been minimized.

Copy link
Contributor

aviadatsnyk commented Jun 13, 2018

@mattn the fix for the issue you're describing has already been merged - this is what this PR solved.

@mattn

This comment has been minimized.

Copy link

mattn commented Jun 13, 2018

@aviadatsnyk do you mean this PR solve my comment?

https://github.com/mholt/archiver/pull/65/files

if !strings.HasPrefix(destpath, destination) {
		return fmt.Errorf("%s: illegal file path", filePath)
}

This condition possibly pass insecure paths.

destination = "/foo/bar"
extracted file = "/foo/bar/../../../../etc/hosts"

@aviadatsnyk

This comment has been minimized.

Copy link
Contributor

aviadatsnyk commented Jun 13, 2018

that is what I meant.
see https://play.golang.org/p/9R8ozZaC80X

@mattn

This comment has been minimized.

Copy link

mattn commented Jun 13, 2018

Ah, I missed it.

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