[SECURITY] Avoid directory traversal on archive extraction

The Extension Manager and Language Pack Manager receive Zip archives as
input from foreign sources and extract them on the disk. However, the
previous approach is considered insecure as the target directory is not
checked per file and directory traversal was possible.

This patch adds a new service class that handles the extraction of Zip
archives via PHP's internal ZipArchive class, which can handle such
cases on its own.

Resolves: #88764
Releases: master, 9.5, 8.7
Security-Commit: a02f19c73211a5f1c0286ab44bee27da9b73f026
Security-Bulletin: TYPO3-CORE-SA-2019-024
Change-Id: I701a577f54410344867b868409a38cc44339f976
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/62718
Tested-by: Oliver Hader <oliver.hader@typo3.org>
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>

typo3/sysext/core/Classes/Exception/Archive/ExtractException.php

@@ -0,0 +1,25 @@
+<?php
+declare(strict_types = 1);
+namespace TYPO3\CMS\Core\Exception\Archive;
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+use TYPO3\CMS\Core\Exception;
+
+/**
+ * An exception thrown when extracting an archive fails
+ */
+class ExtractException extends Exception
+{
+}

typo3/sysext/core/Classes/Service/Archive/ZipService.php

@@ -0,0 +1,103 @@
+<?php
+declare(strict_types = 1);
+namespace TYPO3\CMS\Core\Service\Archive;
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+use TYPO3\CMS\Core\Exception\Archive\ExtractException;
+
+/**
+ * Service that handles zip creation and extraction
+ *
+ * @internal
+ */
+class ZipService
+{
+    /**
+     * Extracts the zip archive to a given directory. This method makes sure a file cannot be placed outside the directory.
+     *
+     * @param string $fileName
+     * @param string $directory
+     * @return bool
+     * @throws ExtractException
+     */
+    public function extract(string $fileName, string $directory): bool
+    {
+        $this->assertDirectoryIsWritable($directory);
+
+        $zip = new \ZipArchive();
+        $state = $zip->open($fileName);
+        if ($state !== true) {
+            throw new ExtractException(
+                sprintf('Unable to open zip file %s, error code %d', $fileName, $state),
+                1565709712
+            );
+        }
+
+        $result = $zip->extractTo($directory);
+        $zip->close();
+        return $result;
+    }
+
+    /**
+     * @param string $fileName
+     * @return bool
+     * @throws ExtractException
+     */
+    public function verify(string $fileName): bool
+    {
+        $zip = new \ZipArchive();
+        $state = $zip->open($fileName);
+        if ($state !== true) {
+            throw new ExtractException(
+                sprintf('Unable to open zip file %s, error code %d', $fileName, $state),
+                1565709713
+            );
+        }
+
+        for ($i = 0; $i < $zip->numFiles; $i++) {
+            $entryName = $zip->getNameIndex($i);
+            if (preg_match('#/(?:\.{2,})+#', $entryName) // Contains any traversal sequence starting with a slash, e.g. /../, /.., /.../
+                || preg_match('#^(?:\.{2,})+/#', $entryName) // Starts with a traversal sequence, e.g. ../, .../
+            ) {
+                throw new ExtractException(
+                    sprintf('Suspicious sequence in zip file %s: %s', $fileName, $entryName),
+                    1565709714
+                );
+            }
+        }
+
+        $zip->close();
+        return true;
+    }
+
+    /**
+     * @param string $directory
+     */
+    private function assertDirectoryIsWritable(string $directory): void
+    {
+        if (!is_dir($directory)) {
+            throw new \RuntimeException(
+                sprintf('Directory %s does not exist', $directory),
+                1565773005
+            );
+        }
+        if (!is_writable($directory)) {
+            throw new \RuntimeException(
+                sprintf('Directory %s is not writable', $directory),
+                1565773006
+            );
+        }
+    }
+}

typo3/sysext/core/Tests/Functional/Service/Archive/ZipServiceTest.php

@@ -0,0 +1,163 @@
+<?php
+declare(strict_types = 1);
+namespace TYPO3\CMS\Core\Tests\Functional\Service\Archive;
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+use org\bovigo\vfs\vfsStream;
+use org\bovigo\vfs\vfsStreamDirectory;
+use TYPO3\CMS\Core\Exception\Archive\ExtractException;
+use TYPO3\CMS\Core\Service\Archive\ZipService;
+use TYPO3\CMS\Core\Utility\GeneralUtility;
+use TYPO3\TestingFramework\Core\Functional\FunctionalTestCase;
+
+class ZipServiceTest extends FunctionalTestCase
+{
+    /**
+     * @var vfsStreamDirectory
+     */
+    private $vfs;
+
+    /**
+     * @var string
+     */
+    private $directory;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $structure = [
+            'typo3conf' => [
+                'ext' => [],
+            ],
+        ];
+        $this->vfs = vfsStream::setup('root', null, $structure);
+        $this->directory = vfsStream::url('root/typo3conf/ext');
+    }
+
+    protected function tearDown(): void
+    {
+        parent::tearDown();
+        unset($this->vfs, $this->directory);
+    }
+
+    /**
+     * @test
+     */
+    public function filesCanNotGetExtractedOutsideTargetDirectory(): void
+    {
+        $extensionDirectory = $this->directory . '/malicious';
+        GeneralUtility::mkdir($extensionDirectory);
+
+        (new ZipService())->extract(
+            __DIR__ . '/Fixtures/malicious.zip',
+            $extensionDirectory
+        );
+        GeneralUtility::fixPermissions($extensionDirectory, true);
+
+        self::assertFileNotExists($extensionDirectory . '/../tool.php');
+        self::assertFileExists($extensionDirectory . '/tool.php');
+        // This is a smoke test to verify PHP's zip library is broken regarding symlinks
+        self::assertFileExists($extensionDirectory . '/passwd');
+        self::assertFalse(is_link($extensionDirectory . '/passwd'));
+    }
+
+    /**
+     * @test
+     */
+    public function fileContentIsExtractedAsExpected(): void
+    {
+        $extensionDirectory = $this->directory . '/my_extension';
+        GeneralUtility::mkdir($extensionDirectory);
+
+        (new ZipService())->extract(
+            __DIR__ . '/Fixtures/my_extension.zip',
+            $extensionDirectory
+        );
+        GeneralUtility::fixPermissions($extensionDirectory, true);
+
+        self::assertDirectoryExists($extensionDirectory . '/Classes');
+        self::assertFileExists($extensionDirectory . '/Resources/Public/Css/empty.css');
+        self::assertFileExists($extensionDirectory . '/ext_emconf.php');
+    }
+
+    /**
+     * @test
+     */
+    public function nonExistentFileThrowsException(): void
+    {
+        $this->expectException(ExtractException::class);
+        $this->expectExceptionCode(1565709712);
+
+        (new ZipService())->extract(
+            'foobar.zip',
+            vfsStream::url('root')
+        );
+    }
+
+    /**
+     * @test
+     */
+    public function nonExistentDirectoryThrowsException(): void
+    {
+        $this->expectException(\RuntimeException::class);
+        $this->expectExceptionCode(1565773005);
+
+        (new ZipService())->extract(
+            __DIR__ . '/Fixtures/my_extension.zip',
+            vfsStream::url('root/non-existent-directory')
+        );
+    }
+
+    /**
+     * @test
+     */
+    public function nonWritableDirectoryThrowsException(): void
+    {
+        $this->expectException(\RuntimeException::class);
+        $this->expectExceptionCode(1565773006);
+
+        $extensionDirectory = $this->directory . '/my_extension';
+        GeneralUtility::mkdir($extensionDirectory);
+        chmod($extensionDirectory, 0000);
+
+        (new ZipService())->extract(
+            __DIR__ . '/Fixtures/my_extension.zip',
+            $extensionDirectory
+        );
+        self::assertFileExists($extensionDirectory . '/Resources/Public/Css/empty.css');
+    }
+
+    /**
+     * @test
+     */
+    public function verifyDetectsValidArchive(): void
+    {
+        self::assertTrue(
+            (new ZipService())->verify(__DIR__ . '/Fixtures/my_extension.zip')
+        );
+    }
+
+    /**
+     * @test
+     */
+    public function verifyDetectsSuspiciousSequences(): void
+    {
+        $this->expectException(ExtractException::class);
+        $this->expectExceptionCode(1565709714);
+
+        (new ZipService())->verify(__DIR__ . '/Fixtures/malicious.zip');
+    }
+}

typo3/sysext/extensionmanager/Classes/Utility/FileHandlingUtility.php

@@ -14,8 +14,12 @@
  * The TYPO3 project - inspiring people to share!
  */
 
+use Psr\Log\LoggerAwareInterface;
+use Psr\Log\LoggerAwareTrait;
 use TYPO3\CMS\Core\Core\Environment;
+use TYPO3\CMS\Core\Exception\Archive\ExtractException;
 use TYPO3\CMS\Core\Localization\LanguageService;
+use TYPO3\CMS\Core\Service\Archive\ZipService;
 use TYPO3\CMS\Core\SingletonInterface;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\Core\Utility\PathUtility;
@@ -26,8 +30,10 @@
  * Utility for dealing with files and folders
  * @internal This class is a specific ExtensionManager implementation and is not part of the Public TYPO3 API.
  */
-class FileHandlingUtility implements SingletonInterface
+class FileHandlingUtility implements SingletonInterface, LoggerAwareInterface
 {
+    use LoggerAwareTrait;
+
     /**
      * @var EmConfUtility
      */
@@ -415,29 +421,18 @@ public function createZipFileFromExtension($extension)
     public function unzipExtensionFromFile($file, $fileName, $pathType = 'Local')
     {
         $extensionDir = $this->makeAndClearExtensionDir($fileName, $pathType);
-        $zip = zip_open($file);
-        if (is_resource($zip)) {
-            while (($zipEntry = zip_read($zip)) !== false) {
-                if (strpos(zip_entry_name($zipEntry), '/') !== false) {
-                    $last = strrpos(zip_entry_name($zipEntry), '/');
-                    $dir = substr(zip_entry_name($zipEntry), 0, $last);
-                    $file = substr(zip_entry_name($zipEntry), strrpos(zip_entry_name($zipEntry), '/') + 1);
-                    if (!is_dir($extensionDir . $dir)) {
-                        GeneralUtility::mkdir_deep($extensionDir . $dir);
-                    }
-                    if (trim($file) !== '') {
-                        $return = GeneralUtility::writeFile($extensionDir . $dir . '/' . $file, zip_entry_read($zipEntry, zip_entry_filesize($zipEntry)));
-                        if ($return === false) {
-                            throw new ExtensionManagerException('Could not write file ' . $this->getRelativePath($file), 1344691048);
-                        }
-                    }
-                } else {
-                    GeneralUtility::writeFile($extensionDir . zip_entry_name($zipEntry), zip_entry_read($zipEntry, zip_entry_filesize($zipEntry)));
-                }
+
+        try {
+            $zipService = GeneralUtility::makeInstance(ZipService::class);
+            if ($zipService->verify($file)) {
+                $zipService->extract($file, $extensionDir);
             }
-        } else {
-            throw new ExtensionManagerException('Unable to open zip file ' . $this->getRelativePath($file), 1344691049);
+        } catch (ExtractException $e) {
+            $this->logger->error('Extracting the extension archive failed', ['exception' => $e]);
+            throw new ExtensionManagerException('Extracting the extension archive failed: ' . $e->getMessage(), 1565777179, $e);
         }
+
+        GeneralUtility::fixPermissions($extensionDir, true);
     }
 
     /**