Potential Zip Slip vulnerability on querylog

[nicowaisman]

Describe the problem

Hey cockroach team,
We found a potentially vulnerable function to the Zip Slip vulnerability. The idea of the vulnerability is that by triggering a path transversal on a zip file you are able to write files outside of your folder (https://cwe.mitre.org/data/definitions/29.html)
Here is the vulnerable function:
https://github.com/cockroachdb/cockroach/blob/master/pkg/workload/querylog/querylog.go#L544
We are well aware, that downloads files passed to unzip only occur from a specific URL provided by cockroach, so the risk is really low, but we thought we should let you know about the potential risk.

Regards
Semmle Security Team
Discovered by Max Schaefer

[rolandcrosby]

@nicowaisman Thank you for the report!

@yuzefovich Can you take a look at this? I'm not sure where the input to this function comes from; is there a simple fix that would let us extract the zip files in a safer/more conservative way?

[yuzefovich]

The path to a zip file comes via a flag to the querylog (like querylog --zip=./test.zip ...), but I don't think I see the problem here. The linked page about the vulnerability describes the problem with a context of a "restricted directory," but in my opinion there is no such directory when using querylog. I think that whomever is using the tool should be free to put the zip file anywhere in their filesystem and then should be able to run the tool with any cwd and should be free to specify the relative path to the zip. Granted, I have very little experience with computer security, so I'd really appreciate an additional explanation of the problem. cc @nicowaisman @rolandcrosby

We could simply remove the option to use the zip file as a source of the query log and force the user to extract the archive and then specify the path to the directory containing the log.

[nicowaisman]

The problem really is that if you use a zip file, that has files like with a name like:
../../../../../../../../usr/bin/vi
When the file is unzipped, vi will be replaced. Next time you run vi, you will be running something else.

Again, as it is right now, you need to unzip a harmful file, so the risk is quite reduced, but we want to warn you about a future potential outcome.

[yuzefovich]

I see, thanks for the explanation @nicowaisman. As far as I understand, the same thing will happen if the user extracts the harmful zip file manually, right? Or I guess it might depend on the way the zip is extracted?

Anyway, the querylog tool is meant to be used on the log that is trustworthy, so I think the benefit to the user experience of not having to manually extract the zip outweighs the possible security vulnerability, but I really appreciate you pointing it out.

[nicowaisman]

Well, you don't really need to stop them from unziping, you just need to double check for path traversal.
Here is how other people have fix this problem:
mholt/archiver@d818164
openshift/source-to-image@f5cbcbc
cloudfoundry/archiver@09b5706

Or you could check this article:
https://golangcode.com/unzip-files-in-go/

Regards

[sauyon]

Relevant snippet from the last link, modified for use here:

        // Check for ZipSlip. More Info: http://bit.ly/2MsjAWE
        if !strings.HasPrefix(path, filepath.Clean(dest)+string(os.PathSeparator)) {
            return fmt.Errorf("%s: illegal file path", path)
        }

Possibly adding a message about trusting the zip file in the error.

It's a reasonable sanity check to have. Most unzipping tools don't allow this behavior:

> unzip zip-slip.zip
Archive:  zip-slip.zip
 extracting: good.txt
warning:  skipped "../" path component(s) in ../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../tmp/evil.txt
 extracting: tmp/evil.txt

and disallowing paths such as these should never affect normal use.

[yuzefovich]

@nicowaisman @sauyon Thanks for the input folks! I opened a PR using @sauyon's suggested fix (with a modification of the error message).