Insufficient protection against path traversal in zip files

[cym13]

Putting entries in zip files to overwrite arbitrary files is an old issue. This is fortunately taken in account in dlpdocs when downloading archives from github.

The code in dl.d reads:

    auto archive = new ZipArchive(zipAnswer.content);
    foreach(name, am; archive.directory) {
        if(name.endsWith(".d")) { // || name.endsWith(".di"))
        // FIXME: skip internal things
            auto path = buildSourceFilePath(project, versionTag, name);
            if(path.indexOf("../") != -1) throw new Exception("illegal source filename in zip");
            std.file.mkdirRecurse(path[0 .. path.lastIndexOf("/")]);
            archive.expand(am);
            std.file.write(path, am.expandedData);
        }
    }

This protects against attacks that use relative paths, but not absolute ones such as "/etc/passwd".

I think adding a check to disallow absolute paths would be the simplest option, the best option being not to try detecting an illegal source filename but instead checking that the path effectively ends up in a subdirectory of the target directory.

[adamdruppe]

On Wed, Jun 06, 2018 at 06:18:07AM -0700, Cédric Picard wrote:

This protects against attacks that use relative paths, but not absolute ones such as "/etc/passwd".

hm, true.

Though note that the operating system user account that runs this process has no access to the rest of the system anyway so it should just throw permission denied.

I think adding a check to disallow absolute paths would be the simplest option, the best option being not to try detecting an illegal source filename but instead checking that the path effectively ends up in a subdirectory of the target directory.

But yeah this is probably a good idea anyway.