Also need to strip off leading / characters to sanitize zip entry paths

[lukehutch]

Zip paths are not supposed to start with a slash, but in theory they can -- you can put anything you want in the zip entry name. In most cases, two slashes in a row in a path will just be interpreted as a single slash, but it is highly likely that a great deal of library routines out there that resolve a zip entry path relative to a base path will treat a zip entry path with a leading / as an absolute path, and ignore the base path during path resolution. Therefore, all the info on the ZipSlip repository should also strip off leading / characters.

You also can't just strip off a single leading /, in case there are two leading / characters. So something like the following is needed:

while (zipEntryPath.startsWith("/")) {
    zipEntryPath = zipEntryPath.substring(1);
}

For reference, my fix for this issue and the standard Zip Slip issue in FastClasspathScanner is here:

classgraph/classgraph@93910ad

In an even more esoteric case, on Windows, it's possible that some library routines that resolve a relative path, relative to a base path, may interpret a path starting with a drive designation as an absolute path, e.g. c:/Windows/System32. Probably this should be protected against too, just to make things more complicated...

I remember first hearing about Zip Slip (though it didn't have a name at the time) more than 20 years ago. I'm surprised this has not been more widely known before now, but I'm glad you're working to change that, and get all the broken code fixed!

[grnd]

Hi @lukehutch! Thanks for sharing this!
Can you please share a simplified code sample demonstrating this use-case. I want to make sure I understand it fully, and the reference to fast-classpath-scanner is a bit convoluted.
Thanks!
Danny

[lukehutch]

The use case is relatively simple -- Zip Slip is about malicious paths containing ../ sequences that try to escape the unzip target root directory. I am saying that malicious paths could also simply start with /, which (depending on how paths are resolved relative to a base dir) could actually jump the unzip target directory all the way up to the root directory.

In other words, in a regular zipfile you would have paths like "path/to/file". In a malicious zipfile, you would have malicious paths like path/to/../../../../maliciousfile. But you could also have malicious paths like /absolute/path/to/maliciousfile.

The problem occurs when code that is trying to unzip a zipfile calls a function like resolvePath(unzipTargetDirPath, zipEntryPath), where the logic says something like:

static String resolvePath(String baseDirPath, String relativePath) {
    if (isAbsolutePath(relativePath)) {
        return relativePath;
    } else {
        return baseDirPath + "/" + relativeDirPath;
    }
}

static boolean isAbsolutePath(String path) {
    return path.startsWith("/");
}

Does that help clarify the threat vector?

In fact on Windows, it's probably even possible to start a malicious path in a zip entry with a drive letter like c:/, and have the path be treated as an absolute path, e.g. c:/absolute/path/to/maliciousfile, given the following implementation of isAbsolutePath(path) on Windows (assuming that the zipfile API always returns / as the path separator for zip entries, even if they are stored in the zipfile as \, though I'm not sure if that is valid):

static boolean isAbsoutePath(String path) {
    return path.startsWith("/") || 
        (path.length() > 3 && Character.isLetter(path.charAt(0))
            && path.charAt(1) == ':' && path.charAt(1) == '/');
}

[grnd]

Thanks for clarifying that.
The suggested fix should prevent this vector too.

String canonicalDestinationDirPath = destinationDir.getCanonicalPath();
File destinationfile = new File(destinationDir, e.getName());
String canonicalDestinationFile = destinationfile.getCanonicalPath();
if (!canonicalDestinationFile.startsWith(canonicalDestinationDirPath + File.separator)) {
  throw new ArchiverException("Entry is outside of the target dir: " + e.getName());
}

[lukehutch]

Yes, but path canonicalization can be very time consuming in Java (more than you would expect), and it can actually change the semantics of the path (including the structure of sub-path nesting), if symbolic links are followed during canonicalization. So sometimes you really don't want to do canonicalization. I have come across multiple bugs and slowdowns in different projects in the past caused by canonicalization.

Even though canonicalization does solve the problem, if you can use it, the Zip Slip docs need to at least point out that there are two versions of Zip Slip: one that works with relative paths (the one currently in the docs), and one that works with absolute paths (the one I presented here). That way if canonicalization is not usable, people can at least code defensively to check for this absolute case.