From 0efda30ca4181898e17614a195b234954d353f2b Mon Sep 17 00:00:00 2001
From: Andreas Fernandez <a.fernandez@scripting-base.de>
Date: Tue, 17 Dec 2019 10:53:08 +0100
Subject: [PATCH] [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>
---
.../Exception/Archive/ExtractException.php | 25 +++
.../Classes/Service/Archive/ZipService.php | 103 +++++++++++
.../Service/Archive/Fixtures/malicious.zip | Bin 0 -> 668 bytes
.../Service/Archive/Fixtures/my_extension.zip | Bin 0 -> 1104 bytes
.../Service/Archive/ZipServiceTest.php | 163 ++++++++++++++++++
.../Classes/Utility/FileHandlingUtility.php | 39 ++---
.../Classes/Service/LanguagePackService.php | 43 +----
7 files changed, 317 insertions(+), 56 deletions(-)
create mode 100644 typo3/sysext/core/Classes/Exception/Archive/ExtractException.php
create mode 100644 typo3/sysext/core/Classes/Service/Archive/ZipService.php
create mode 100644 typo3/sysext/core/Tests/Functional/Service/Archive/Fixtures/malicious.zip
create mode 100644 typo3/sysext/core/Tests/Functional/Service/Archive/Fixtures/my_extension.zip
create mode 100644 typo3/sysext/core/Tests/Functional/Service/Archive/ZipServiceTest.php
diff --git a/typo3/sysext/core/Classes/Exception/Archive/ExtractException.php b/typo3/sysext/core/Classes/Exception/Archive/ExtractException.php
new file mode 100644
index 000000000000..8febbea3d88f
--- /dev/null
+++ b/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
+{
+}
diff --git a/typo3/sysext/core/Classes/Service/Archive/ZipService.php b/typo3/sysext/core/Classes/Service/Archive/ZipService.php
new file mode 100644
index 000000000000..7484c4e56208
--- /dev/null
+++ b/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
+ );
+ }
+ }
+}
diff --git a/typo3/sysext/core/Tests/Functional/Service/Archive/Fixtures/malicious.zip b/typo3/sysext/core/Tests/Functional/Service/Archive/Fixtures/malicious.zip
new file mode 100644
index 0000000000000000000000000000000000000000..b050224dc77e7e7a718c4b17c88a92e1a77fb0f1
GIT binary patch
literal 668
zcmWIWW@h1H00GB__kOx*hY|#VY!K#VkYUi%(=W--&(SN$C<qPVWMGbL*NII6;?fFk
z21b^zj0_AcB0!}!_CN((K%AMEmam~{%@qJQwbzx;pKGVQCO6nb1_m~usRfC}#pNkr
z15d3Djs;<efiIYW27(Cv)RJU<h%%5-AUil1CV24q=h|K?Sp(!L0j=f(8kJg65}%rz
zoS&BlcVqpg;MjH`Ml&+E&yf3&fk5l`e_hfoI(KEZT6N?_wd)0S9Wh(hIHBc^=-U1F
zG8VCZwAk@@Uv>ZXnBDhMH^2S2N0Wc|EZZr`ydnYV0$mf8bR|Tz6Q;^f$h#J7zscs3
zU&8CD%BjA;UWz(p@TGKS+cNoTt?)Zu#P`VW&;##DF;RX_QTA@!%h^}%<Iz08aj1$j
z(oQLRf=zhahu-9SQ%-SSURpfaUm;^%)YdQT4;7XkkQZ{>d}3aJHzSiAGp=Zn0Q#SS
z0T=)bOBz8elt^NQL=svAAsdJpBM<{YM*UwH1|(te12hXu+(6926(z`yTL?4@J%WH{
Zf+7elass?r*+BL)1K~#?Jr(3T1^`#4v?~Ar
literal 0
HcmV?d00001
diff --git a/typo3/sysext/core/Tests/Functional/Service/Archive/Fixtures/my_extension.zip b/typo3/sysext/core/Tests/Functional/Service/Archive/Fixtures/my_extension.zip
new file mode 100644
index 0000000000000000000000000000000000000000..852ec4c295ad3606952f0510900a0c7c0fc55fca
GIT binary patch
literal 1104
zcmWIWW@Zs#00HK8y#8PYl;B`cU~tY!EG|wh)(;KgWnlj<92^V7r4`%^j4Ush85qDs
z0Nj+)4XCDY0Zj=?EzU13N`{#;PdqqwGZ2GloaU6HnIni|PC#i=PG&O1S&M+i>;_^G
zjnlYBG~-0E8RuLK_30+4ksunYksJ&h4BBYnC5Ow%)ZBuSO1<RbVsI!ri3G>41)7V+
zY-WZ4Z*~q)@H0W<01~bs+f0#cV*|?a0c}gID2Y$aP0r6t(<{g*0Gl2TG(7`|K{T4n
zH*eHGq49tTY{$9BQp_NWKqduvGct)V<4yxmr!c&A1hI%u5eQSUr3r+oZa^j}DFk6I
zwlsn;w-3c!qEigQcx-6~Vf<Ze#uJx<AXXr!Ay|Ba(hv+RX*6I&4jHHqKvoc)qF`|g
mOF_u-4oX2Vu%vMw&@R%_Q-C)sDER=>5F3L9&;v|hPXGXe%h@ad
literal 0
HcmV?d00001
diff --git a/typo3/sysext/core/Tests/Functional/Service/Archive/ZipServiceTest.php b/typo3/sysext/core/Tests/Functional/Service/Archive/ZipServiceTest.php
new file mode 100644
index 000000000000..f16bf57f614e
--- /dev/null
+++ b/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');
+ }
+}
diff --git a/typo3/sysext/extensionmanager/Classes/Utility/FileHandlingUtility.php b/typo3/sysext/extensionmanager/Classes/Utility/FileHandlingUtility.php
index 6ca2c77dfda4..df0f6b701644 100644
--- a/typo3/sysext/extensionmanager/Classes/Utility/FileHandlingUtility.php
+++ b/typo3/sysext/extensionmanager/Classes/Utility/FileHandlingUtility.php
@@ -14,8 +14,12 @@ namespace TYPO3\CMS\Extensionmanager\Utility;
* 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 @@ use TYPO3\CMS\Extensionmanager\Exception\ExtensionManagerException;
* 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 @@ class FileHandlingUtility implements SingletonInterface
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);
}
/**
diff --git a/typo3/sysext/install/Classes/Service/LanguagePackService.php b/typo3/sysext/install/Classes/Service/LanguagePackService.php
index 5d6bcc52ec05..fc2a2a3427eb 100644
--- a/typo3/sysext/install/Classes/Service/LanguagePackService.php
+++ b/typo3/sysext/install/Classes/Service/LanguagePackService.php
@@ -26,6 +26,7 @@ use TYPO3\CMS\Core\Http\Uri;
use TYPO3\CMS\Core\Localization\Locales;
use TYPO3\CMS\Core\Package\PackageManager;
use TYPO3\CMS\Core\Registry;
+use TYPO3\CMS\Core\Service\Archive\ZipService;
use TYPO3\CMS\Core\Utility\ExtensionManagementUtility;
use TYPO3\CMS\Core\Utility\GeneralUtility;
use TYPO3\CMS\Core\Utility\PathUtility;
@@ -361,43 +362,17 @@ class LanguagePackService implements LoggerAwareInterface
*
* @param string $file path to zip file
* @param string $path path to extract to
- * @throws \RuntimeException
*/
protected function unzipTranslationFile(string $file, string $path)
{
- $zip = zip_open($file);
- if (is_resource($zip)) {
- if (!is_dir($path)) {
- GeneralUtility::mkdir_deep($path);
- }
- while (($zipEntry = zip_read($zip)) !== false) {
- $zipEntryName = zip_entry_name($zipEntry);
- if (strpos($zipEntryName, '/') !== false) {
- $zipEntryPathSegments = explode('/', $zipEntryName);
- $fileName = array_pop($zipEntryPathSegments);
- // It is a folder, because the last segment is empty, let's create it
- if (empty($fileName)) {
- GeneralUtility::mkdir_deep($path . implode('/', $zipEntryPathSegments));
- } else {
- $absoluteTargetPath = GeneralUtility::getFileAbsFileName($path . implode('/', $zipEntryPathSegments) . '/' . $fileName);
- if (trim($absoluteTargetPath) !== '') {
- $return = GeneralUtility::writeFile(
- $absoluteTargetPath,
- zip_entry_read($zipEntry, zip_entry_filesize($zipEntry))
- );
- if ($return === false) {
- throw new \RuntimeException('Could not write file ' . $zipEntryName, 1520170845);
- }
- } else {
- throw new \RuntimeException('Could not write file ' . $zipEntryName, 1520170846);
- }
- }
- } else {
- throw new \RuntimeException('Extension directory missing in zip file!', 1520170847);
- }
- }
- } else {
- throw new \RuntimeException('Unable to open zip file ' . $file, 1520170848);
+ if (!is_dir($path)) {
+ GeneralUtility::mkdir_deep($path);
+ }
+
+ $zipService = GeneralUtility::makeInstance(ZipService::class);
+ if ($zipService->verify($file)) {
+ $zipService->extract($file, $path);
}
+ GeneralUtility::fixPermissions($path, true);
}
}
--
GitLab