• d-bugmail puremagic.com (85/85) Jun 06 2018 https://issues.dlang.org/show_bug.cgi?id=18950

d-bugmail puremagic.com writes:

https://issues.dlang.org/show_bug.cgi?id=18950

          Issue ID: 18950
           Summary: Std.zip vulnerable to arbitrary file write
           Product: D
           Version: D2
          Hardware: x86_64
                OS: Linux
            Status: NEW
          Severity: major
          Priority: P1
         Component: phobos
          Assignee: nobody puremagic.com
          Reporter: cpicard openmailbox.org

Created attachment 1702
  --> https://issues.dlang.org/attachment.cgi?id=1702&action=edit
POC ZIP file to test path traversal on a POSIX system

Researchers from Snyk have recently disclosed a wide-spread vulnerability they
call "Zip Slip" which affects many libraries in many languages.

The original blog here: https://snyk.io/research/zip-slip-vulnerability

The issue is two-fold:

- Many ZIP libraries do not check for path traversal in file names (things like
../../../../../../../tmp/test for example.

- Many applications using those libraries decompress the files to the disk
without checking the names either.

Zip files typically cannot contain those names normally, but they can be easily
hand-crafted. This allows an attacker to leverage an application's use of
std.zip to create or overwrite arbitrary files using the application's rights.
This typically results in arbitrary code execution.

Phobos std.zip authorizes such path traversal by default and doesn't provide
any facility to do the right thing easily.

Arguably this is a consummer issue: it is possible to use Phobos right by
checking the name before writting the file, but requiring conciousness of the
issue is asking a lot on the user. As Snyk's research shows, most vulnerable
applications they found were written in Java and they attribute that to the
lack of primitives doing the right thing in Java libraries.

To demonstrate the issue here is a test script written at 99% using the example
from std.zip that is the baseline for a good use of the library. In attachment
you will find a zip file that will try to create an empty file to
../../../../../../../../../../tmp/not-good.

```D
import std.digest.crc;
import std.file;
import std.stdio;
import std.zip;


void main(string[] args) {
   // read a zip file into memory
   auto zip = new ZipArchive(read(args[1]));
   writeln("Archive: ", args[1]);
   writefln("%-10s  %-8s  Name", "Length", "CRC-32");
   // iterate over all zip members
   foreach (name, am; zip.directory)
   {
       // print some data about each member
       writefln("%10s  %08x  %s", am.expandedSize, am.crc32, name);
       assert(am.expandedData.length == 0);
       // decompress the archive member
       zip.expand(am);
       assert(am.expandedData.length == am.expandedSize);
       std.file.write(name, am.expandedData);
   }
}
```

```SH
    sh-4.4$ dmd --version
    DMD64 D Compiler v2.080.0-dirty
    Copyright (C) 1999-2018 by The D Language Foundation, All Rights Reserved
written by Walter Bright
    sh-4.4$ ls /tmp/not-good
    ls: cannot access '/tmp/not-good': No such file or directory
    sh-4.4$ ls
    test.d  test.zip
    sh-4.4$ rdmd test.d test.zip
    Archive: test.zip
    Length      CRC-32    Name
             0  00000000  ../../../../../../../../../../tmp/not-good
    sh-4.4$ ls /tmp/not-good
    /tmp/not-good
```

I think std.zip should do at least one of the following:

- Refuse files with names resulting in path traversal by default (a flag to
enable that behaviour could be used if projects actually use that, but it seems
dubious);

- Provide a function to decompress a file to the disk and not in memory which
also checks for path traversal.

--