fix issue #39

Ne-Lexa · Jul 25, 2019 · a84d2f9 · a84d2f9
1 parent 97db60a
commit a84d2f9
Show file tree

Hide file tree

Showing 3 changed files with 73 additions and 12 deletions.
diff --git a/src/PhpZip/Util/FilesUtil.php b/src/PhpZip/Util/FilesUtil.php
@@ -225,4 +225,23 @@ public static function humanSize($size, $unit = null)
         }
         return number_format($size) . " bytes";
     }
+
+    /**
+     * Normalizes zip path.
+     *
+     * @param string $path Zip path
+     * @return string
+     */
+    public static function normalizeZipPath($path)
+    {
+        return implode(
+            '/',
+            array_filter(
+                explode('/', (string)$path),
+                static function ($part) {
+                    return $part !== '.' && $part !== '..';
+                }
+            )
+        );
+    }
 }
diff --git a/src/PhpZip/ZipFile.php b/src/PhpZip/ZipFile.php
@@ -297,13 +297,13 @@ public function matcher()
     public function extractTo($destination, $entries = null)
     {
         if (!file_exists($destination)) {
-            throw new ZipException("Destination " . $destination . " not found");
+            throw new ZipException(sprintf('Destination %s not found', $destination));
         }
         if (!is_dir($destination)) {
-            throw new ZipException("Destination is not directory");
+            throw new ZipException('Destination is not directory');
         }
         if (!is_writable($destination)) {
-            throw new ZipException("Destination is not writable directory");
+            throw new ZipException('Destination is not writable directory');
         }
 
         $zipEntries = $this->zipModel->getEntries();
@@ -315,18 +315,19 @@ public function extractTo($destination, $entries = null)
             if (is_array($entries)) {
                 $entries = array_unique($entries);
                 $flipEntries = array_flip($entries);
-                $zipEntries = array_filter($zipEntries, function (ZipEntry $zipEntry) use ($flipEntries) {
+                $zipEntries = array_filter($zipEntries, static function (ZipEntry $zipEntry) use ($flipEntries) {
                     return isset($flipEntries[$zipEntry->getName()]);
                 });
             }
         }
 
         foreach ($zipEntries as $entry) {
-            $file = $destination . DIRECTORY_SEPARATOR . $entry->getName();
+            $entryName = FilesUtil::normalizeZipPath($entry->getName());
+            $file = $destination . DIRECTORY_SEPARATOR . $entryName;
             if ($entry->isDirectory()) {
                 if (!is_dir($file)) {
-                    if (!mkdir($file, 0755, true)) {
-                        throw new ZipException("Can not create dir " . $file);
+                    if (!mkdir($file, 0755, true) && !is_dir($file)) {
+                        throw new ZipException('Can not create dir ' . $file);
                     }
                     chmod($file, 0755);
                     touch($file, $entry->getTime());
@@ -335,8 +336,8 @@ public function extractTo($destination, $entries = null)
             }
             $dir = dirname($file);
             if (!is_dir($dir)) {
-                if (!mkdir($dir, 0755, true)) {
-                    throw new ZipException("Can not create dir " . $dir);
+                if (!mkdir($dir, 0755, true) && !is_dir($dir)) {
+                    throw new ZipException('Can not create dir ' . $dir);
                 }
                 chmod($dir, 0755);
                 touch($dir, $entry->getTime());
@@ -1210,7 +1211,7 @@ public function saveAsFile($filename)
     {
         $filename = (string)$filename;
 
-        $tempFilename = $filename . '.temp' . uniqid();
+        $tempFilename = $filename . '.temp' . uniqid('', true);
         if (!($handle = @fopen($tempFilename, 'w+b'))) {
             throw new InvalidArgumentException("File " . $tempFilename . ' can not open from write.');
         }
@@ -1256,7 +1257,7 @@ public function outputAsAttachment($outputFilename, $mimeType = null, $attachmen
     {
         $outputFilename = (string)$outputFilename;
 
-        if (empty($mimeType) || !is_string($mimeType) && !empty($outputFilename)) {
+        if (empty($mimeType) || (!is_string($mimeType) && !empty($outputFilename))) {
             $ext = strtolower(pathinfo($outputFilename, PATHINFO_EXTENSION));
 
             if (!empty($ext) && isset(self::$defaultMimeTypes[$ext])) {
@@ -1295,7 +1296,7 @@ public function outputAsResponse(ResponseInterface $response, $outputFilename, $
     {
         $outputFilename = (string)$outputFilename;
 
-        if (empty($mimeType) || !is_string($mimeType) && !empty($outputFilename)) {
+        if (empty($mimeType) || (!is_string($mimeType) && !empty($outputFilename))) {
             $ext = strtolower(pathinfo($outputFilename, PATHINFO_EXTENSION));
 
             if (!empty($ext) && isset(self::$defaultMimeTypes[$ext])) {

diff --git a/tests/PhpZip/ZipSlipVulnerabilityTest.php b/tests/PhpZip/ZipSlipVulnerabilityTest.php
@@ -0,0 +1,41 @@
+<?php
+
+namespace PhpZip;
+
+/**
+ * Class ZipSlipVulnerabilityTest
+ *
+ * @package PhpZip
+ * @see https://github.com/Ne-Lexa/php-zip/issues/39 Issue#31
+ * @see https://snyk.io/research/zip-slip-vulnerability Zip Slip Vulnerability
+ */
+class ZipSlipVulnerabilityTest extends ZipTestCase
+{
+    /**
+     * @throws Exception\ZipException
+     */
+    public function testCreateSlipVulnerabilityFile()
+    {
+        $localFile = '../dir/./../../file.txt';
+        $zipFile = new ZipFile();
+        $zipFile->addFromString($localFile, 'contents');
+        self::assertContains($localFile, $zipFile->getListFiles());
+        $zipFile->close();
+    }
+
+    /**
+     * @throws Exception\ZipException
+     */
+    public function testUnpack()
+    {
+        $this->assertTrue(mkdir($this->outputDirname, 0755, true));
+
+        $zipFile = new ZipFile();
+        $zipFile->addFromString('../dir/./../../file.txt', 'contents');
+        $zipFile->extractTo($this->outputDirname);
+        $zipFile->close();
+
+        $expectedExtractedFile = $this->outputDirname . '/dir/file.txt';
+        self::assertTrue(is_file($expectedExtractedFile));
+    }
+}