-
Notifications
You must be signed in to change notification settings - Fork 653
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[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: 2c303d5c52645e5711b2a68c6a459ea5b5099239 Security-Bulletin: TYPO3-CORE-SA-2019-024 Change-Id: Ifa6612e2912552ad13364c31b614248ba8d316d6 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/62699 Tested-by: Oliver Hader <oliver.hader@typo3.org> Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
- Loading branch information
1 parent
96b122b
commit 037b6c2
Showing
8 changed files
with
349 additions
and
93 deletions.
There are no files selected for viewing
25 changes: 25 additions & 0 deletions
25
typo3/sysext/core/Classes/Exception/Archive/ExtractException.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
{ | ||
} |
103 changes: 103 additions & 0 deletions
103
typo3/sysext/core/Classes/Service/Archive/ZipService.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) | ||
{ | ||
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 | ||
); | ||
} | ||
} | ||
} |
Binary file added
BIN
+668 Bytes
typo3/sysext/core/Tests/Functional/Service/Archive/Fixtures/malicious.zip
Binary file not shown.
Binary file added
BIN
+1.08 KB
typo3/sysext/core/Tests/Functional/Service/Archive/Fixtures/my_extension.zip
Binary file not shown.
163 changes: 163 additions & 0 deletions
163
typo3/sysext/core/Tests/Functional/Service/Archive/ZipServiceTest.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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() | ||
{ | ||
parent::setUp(); | ||
|
||
$structure = [ | ||
'typo3conf' => [ | ||
'ext' => [], | ||
], | ||
]; | ||
$this->vfs = vfsStream::setup('root', null, $structure); | ||
$this->directory = vfsStream::url('root/typo3conf/ext'); | ||
} | ||
|
||
protected function tearDown() | ||
{ | ||
parent::tearDown(); | ||
unset($this->vfs, $this->directory); | ||
} | ||
|
||
/** | ||
* @test | ||
*/ | ||
public function filesCanNotGetExtractedOutsideTargetDirectory() | ||
{ | ||
$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() | ||
{ | ||
$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() | ||
{ | ||
$this->expectException(ExtractException::class); | ||
$this->expectExceptionCode(1565709712); | ||
|
||
(new ZipService())->extract( | ||
'foobar.zip', | ||
vfsStream::url('root') | ||
); | ||
} | ||
|
||
/** | ||
* @test | ||
*/ | ||
public function nonExistentDirectoryThrowsException() | ||
{ | ||
$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() | ||
{ | ||
$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() | ||
{ | ||
self::assertTrue( | ||
(new ZipService())->verify(__DIR__ . '/Fixtures/my_extension.zip') | ||
); | ||
} | ||
|
||
/** | ||
* @test | ||
*/ | ||
public function verifyDetectsSuspiciousSequences() | ||
{ | ||
$this->expectException(ExtractException::class); | ||
$this->expectExceptionCode(1565709714); | ||
|
||
(new ZipService())->verify(__DIR__ . '/Fixtures/malicious.zip'); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.