From 71bc495b7f89324f96625e2559849b257aec487b Mon Sep 17 00:00:00 2001 From: Michael Reichardt Date: Tue, 26 Mar 2024 20:06:25 +0100 Subject: [PATCH 1/4] Test zip slip and symlinks --- composer.json | 3 +- .../Blueprints/Runner/Step/RmStepRunner.php | 16 +- .../Runner/Step/UnzipStepRunner.php | 4 +- .../Zip/ZipCentralDirectoryEntry.php | 2 +- src/WordPress/Zip/ZipException.php | 7 + src/WordPress/Zip/ZipFileEntry.php | 2 +- src/WordPress/Zip/functions.php | 10 ++ tests/unit/steps/RmStepRunnerTest.php | 170 ++++++++---------- tests/unit/steps/UnzipStepRunnerTest.php | 76 ++++---- tests/unit/zip/ZipFunctionsTest.php | 27 +++ tests/unit/zip/resources/zip-slip-test.zip | Bin 0 -> 201 bytes .../unit/zip/resources/zip-symlinks-test.zip | Bin 0 -> 170 bytes 12 files changed, 171 insertions(+), 146 deletions(-) create mode 100644 src/WordPress/Zip/ZipException.php create mode 100644 tests/unit/zip/ZipFunctionsTest.php create mode 100644 tests/unit/zip/resources/zip-slip-test.zip create mode 100644 tests/unit/zip/resources/zip-symlinks-test.zip diff --git a/composer.json b/composer.json index cb0b96e9..16013aaf 100644 --- a/composer.json +++ b/composer.json @@ -2,7 +2,8 @@ "prefer-stable": true, "require": { "ext-json": "*", - "php": ">=7.0" + "php": ">=7.0", + "ext-zip": "*" }, "require-dev": { "yoast/phpunit-polyfills": "2.0.0" diff --git a/src/WordPress/Blueprints/Runner/Step/RmStepRunner.php b/src/WordPress/Blueprints/Runner/Step/RmStepRunner.php index 63394382..efb75f24 100644 --- a/src/WordPress/Blueprints/Runner/Step/RmStepRunner.php +++ b/src/WordPress/Blueprints/Runner/Step/RmStepRunner.php @@ -12,16 +12,12 @@ class RmStepRunner extends BaseStepRunner { /** * @param RmStep $input */ - public function run( $input ) { - $resolvedPath = $this->getRuntime()->resolvePath( $input->path ); - $fileSystem = new Filesystem(); - if ( false === $fileSystem->exists( $resolvedPath ) ) { - throw new BlueprintException( "Failed to remove \"$resolvedPath\": the directory or file does not exist." ); - } - try { - $fileSystem->remove( $resolvedPath ); - } catch ( IOException $exception ) { - throw new BlueprintException( "Failed to remove the directory or file at \"$resolvedPath\"", 0, $exception ); + public function run( RmStep $input ) { + $resolved_path = $this->getRuntime()->resolvePath( $input->path ); + $filesystem = new Filesystem(); + if ( false === $filesystem->exists( $resolved_path ) ) { + throw new BlueprintException( "Failed to remove \"$resolved_path\": the directory or file does not exist." ); } + $filesystem->remove( $resolved_path ); } } diff --git a/src/WordPress/Blueprints/Runner/Step/UnzipStepRunner.php b/src/WordPress/Blueprints/Runner/Step/UnzipStepRunner.php index 27866085..40695537 100644 --- a/src/WordPress/Blueprints/Runner/Step/UnzipStepRunner.php +++ b/src/WordPress/Blueprints/Runner/Step/UnzipStepRunner.php @@ -16,8 +16,8 @@ class UnzipStepRunner extends BaseStepRunner { * @return void */ public function run( - $input, - $progress_tracker + UnzipStep $input, + Tracker $progress_tracker ) { $progress_tracker->set( 10, 'Unzipping...' ); diff --git a/src/WordPress/Zip/ZipCentralDirectoryEntry.php b/src/WordPress/Zip/ZipCentralDirectoryEntry.php index 3c2f2503..1747e95c 100644 --- a/src/WordPress/Zip/ZipCentralDirectoryEntry.php +++ b/src/WordPress/Zip/ZipCentralDirectoryEntry.php @@ -59,7 +59,7 @@ public function __construct( $this->versionNeeded = $versionNeeded; $this->versionCreated = $versionCreated; $this->firstByteAt = $firstByteAt; - $this->isDirectory = $this->path[- 1] === '/'; + $this->isDirectory = substr( $this->path, -1 ) === '/'; } public function isFileEntry() { diff --git a/src/WordPress/Zip/ZipException.php b/src/WordPress/Zip/ZipException.php new file mode 100644 index 00000000..0eb7a07e --- /dev/null +++ b/src/WordPress/Zip/ZipException.php @@ -0,0 +1,7 @@ +compressionMethod = $compressionMethod; $this->generalPurpose = $generalPurpose; $this->version = $version; - $this->isDirectory = $this->path[- 1] === '/'; + $this->isDirectory = substr( $this->path, -1 ) === '/'; } public function isFileEntry() { diff --git a/src/WordPress/Zip/functions.php b/src/WordPress/Zip/functions.php index 226111df..845365b5 100644 --- a/src/WordPress/Zip/functions.php +++ b/src/WordPress/Zip/functions.php @@ -14,6 +14,16 @@ function zip_extract_to( $fp, $to_path ) { continue; } + // prevent zip slip -> using relative path to access otherwise inaccessible files + if ( false !== strpos( $entry->path ,'..') ) { + throw new ZipException("Relative paths in zips are not allowed."); + } + + // prevent zip with symlinks -> using a symbolic link to access otherwise inaccessible files + if ( is_link( $entry->path ) ) { + throw new ZipException("Semantic links in zips are not allowed."); + } + $path = Path::canonicalize( $to_path . '/' . $entry->path ); $parent = Path::getDirectory( $path ); if ( ! is_dir( $parent ) ) { diff --git a/tests/unit/steps/RmStepRunnerTest.php b/tests/unit/steps/RmStepRunnerTest.php index 2e76d224..1785e5cd 100644 --- a/tests/unit/steps/RmStepRunnerTest.php +++ b/tests/unit/steps/RmStepRunnerTest.php @@ -10,12 +10,11 @@ use WordPress\Blueprints\Runner\Step\RmStepRunner; use WordPress\Blueprints\Runtime\Runtime; -class RmStepRunnerTest extends PHPUnitTestCase -{ +class RmStepRunnerTest extends PHPUnitTestCase { /** * @var string */ - private $documentRoot; + private $document_root; /** * @var Runtime @@ -25,151 +24,140 @@ class RmStepRunnerTest extends PHPUnitTestCase /** * @var RmStepRunner */ - private $step; + private $step_runner; /** * @var Filesystem */ - private $fileSystem; + private $filesystem; /** * @before */ - public function before() - { - $this->documentRoot = Path::makeAbsolute("test", sys_get_temp_dir()); - $this->runtime = new Runtime($this->documentRoot); + public function before() { + $this->document_root = Path::makeAbsolute( "test", sys_get_temp_dir() ); + $this->runtime = new Runtime( $this->document_root ); - $this->step = new RmStepRunner(); - $this->step->setRuntime($this->runtime); + $this->step_runner = new RmStepRunner(); + $this->step_runner->setRuntime( $this->runtime ); - $this->fileSystem = new Filesystem(); + $this->filesystem = new Filesystem(); } /** * @after */ - public function after() - { - $this->fileSystem->remove($this->documentRoot); + public function after() { + $this->filesystem->remove( $this->document_root ); } - public function testRemoveDirectoryWhenUsingAbsolutePath() - { - $absolutePath = $this->runtime->resolvePath("dir"); - $this->fileSystem->mkdir($absolutePath); + public function testRemoveDirectoryWhenUsingAbsolutePath() { + $absolute_path = $this->runtime->resolvePath( "dir" ); + $this->filesystem->mkdir( $absolute_path ); - $input = new RmStep(); - $input->path = $absolutePath; + $step = new RmStep(); + $step->path = $absolute_path; - $this->step->run($input); + $this->step_runner->run( $step ); - $this->assertDirectoryDoesNotExist($absolutePath); + self::assertDirectoryDoesNotExist( $absolute_path ); } - public function testRemoveDirectoryWhenUsingRelativePath() - { - $relativePath = "dir"; - $absolutePath = $this->runtime->resolvePath($relativePath); - $this->fileSystem->mkdir($absolutePath); + public function testRemoveDirectoryWhenUsingRelativePath() { + $relative_path = "dir"; + $absolute_path = $this->runtime->resolvePath( $relative_path ); + $this->filesystem->mkdir( $absolute_path ); - $input = new RmStep(); - $input->path = $relativePath; + $step = new RmStep(); + $step->path = $relative_path; - $this->step->run($input); + $this->step_runner->run( $step ); - $this->assertDirectoryDoesNotExist($absolutePath); + self::assertDirectoryDoesNotExist( $absolute_path ); } - public function testRemoveDirectoryWithSubdirectory() - { - $relativePath = "dir/subdir"; - $absolutePath = $this->runtime->resolvePath($relativePath); - $this->fileSystem->mkdir($absolutePath); + public function testRemoveDirectoryWithSubdirectory() { + $relative_path = "dir/subdir"; + $absolute_path = $this->runtime->resolvePath( $relative_path ); + $this->filesystem->mkdir( $absolute_path ); - $input = new RmStep(); - $input->path = dirname($relativePath); + $step = new RmStep(); + $step->path = dirname( $relative_path ); - $this->step->run($input); + $this->step_runner->run( $step ); - $this->assertDirectoryDoesNotExist($absolutePath); + self::assertDirectoryDoesNotExist( $absolute_path ); } - public function testRemoveDirectoryWithFile() - { - $relativePath = "dir/file.txt"; - $absolutePath = $this->runtime->resolvePath($relativePath); - $this->fileSystem->dumpFile($absolutePath, "test"); + public function testRemoveDirectoryWithFile() { + $relative_path = "dir/file.txt"; + $absolute_pPath = $this->runtime->resolvePath( $relative_path ); + $this->filesystem->dumpFile( $absolute_pPath, "test" ); - $input = new RmStep(); - $input->path = dirname($relativePath); + $step = new RmStep(); + $step->path = dirname( $relative_path ); - $this->step->run($input); + $this->step_runner->run( $step ); - $this->assertDirectoryDoesNotExist(dirname($absolutePath)); + self::assertDirectoryDoesNotExist( dirname( $absolute_pPath ) ); } - public function testRemoveFile() - { - $relativePath = "file.txt"; - $absolutePath = $this->runtime->resolvePath($relativePath); - $this->fileSystem->dumpFile($absolutePath, "test"); + public function testRemoveFile() { + $relative_path = "file.txt"; + $absolute_path = $this->runtime->resolvePath( $relative_path ); + $this->filesystem->dumpFile( $absolute_path, "test" ); - $input = new RmStep(); - $input->path = $relativePath; + $step = new RmStep(); + $step->path = $relative_path; - $this->step->run($input); + $this->step_runner->run( $step ); - $this->assertDirectoryDoesNotExist($absolutePath); + self::assertDirectoryDoesNotExist( $absolute_path ); } - public function testThrowExceptionWhenRemovingNonexistentDirectoryAndUsingRelativePath() - { - $relativePath = "dir"; - $absolutePath = $this->runtime->resolvePath($relativePath); + public function testThrowExceptionWhenRemovingNonexistentDirectoryAndUsingRelativePath() { + $relative_path = "dir"; + $absolute_path = $this->runtime->resolvePath( $relative_path ); - $input = new RmStep(); - $input->path = $relativePath; + $step = new RmStep(); + $step->path = $relative_path; - $this->expectException(BlueprintException::class); - $this->expectExceptionMessage("Failed to remove \"$absolutePath\": the directory or file does not exist."); - $this->step->run($input); + self::expectException( BlueprintException::class ); + self::expectExceptionMessage( "Failed to remove \"$absolute_path\": the directory or file does not exist." ); + $this->step_runner->run( $step ); } - public function testThrowExceptionWhenRemovingNonexistentDirectoryAndUsingAbsolutePath() - { - $absolutePath = "/dir"; + public function testThrowExceptionWhenRemovingNonexistentDirectoryAndUsingAbsolutePath() { + $absolute_path = "/dir"; - $input = new RmStep(); - $input->path = $absolutePath; + $step = new RmStep(); + $step->path = $absolute_path; - $this->expectException(BlueprintException::class); - $this->expectExceptionMessage("Failed to remove \"$absolutePath\": the directory or file does not exist."); - $this->step->run($input); + self::expectException( BlueprintException::class ); + self::expectExceptionMessage( "Failed to remove \"$absolute_path\": the directory or file does not exist." ); + $this->step_runner->run( $step ); } - public function testThrowExceptionWhenRemovingNonexistentFileAndUsingAbsolutePath() - { - $relativePath = "/file.txt"; + public function testThrowExceptionWhenRemovingNonexistentFileAndUsingAbsolutePath() { + $relative_path = "/file.txt"; - $input = new RmStep(); - $input->path = $relativePath; + $step = new RmStep(); + $step->path = $relative_path; - $this->expectException(BlueprintException::class); - $this->expectExceptionMessage("Failed to remove \"$relativePath\": the directory or file does not exist."); - $this->step->run($input); + self::expectException( BlueprintException::class ); + self::expectExceptionMessage( "Failed to remove \"$relative_path\": the directory or file does not exist." ); + $this->step_runner->run( $step ); } - public function testThrowExceptionWhenRemovingNonexistentFileAndUsingRelativePath() - { + public function testThrowExceptionWhenRemovingNonexistentFileAndUsingRelativePath() { $relativePath = "file.txt"; $absolutePath = $this->runtime->resolvePath($relativePath); - $input = new RmStep(); - $input->path = $relativePath; + $step = new RmStep(); + $step->path = $relativePath; - $this->expectException(BlueprintException::class); - $this->expectExceptionMessage("Failed to remove \"$absolutePath\": the directory or file does not exist."); - $this->step->run($input); + self::expectException( BlueprintException::class ); + self::expectExceptionMessage( "Failed to remove \"$absolutePath\": the directory or file does not exist." ); + $this->step_runner->run( $step ); } } diff --git a/tests/unit/steps/UnzipStepRunnerTest.php b/tests/unit/steps/UnzipStepRunnerTest.php index 40635b79..9477fb75 100644 --- a/tests/unit/steps/UnzipStepRunnerTest.php +++ b/tests/unit/steps/UnzipStepRunnerTest.php @@ -25,17 +25,17 @@ class UnzipStepRunnerTest extends PHPUnitTestCase { private $runtime; /** - * @var UnzipStepRunner $step + * @var UnzipStepRunner $step_runner */ - private $step; + private $step_runner; /** * @var Filesystem */ - private $file_system; + private $filesystem; /** - * @var Stub + * @var ResourceManager $resource_manager resource manager mock */ private $resource_manager; @@ -48,51 +48,47 @@ public function before() { $this->resource_manager = $this->createMock( ResourceManager::class ); - $this->step = new UnzipStepRunner(); - $this->step->setRuntime( $this->runtime ); - $this->step->setResourceManager( $this->resource_manager ); + $this->step_runner = new UnzipStepRunner(); + $this->step_runner->setRuntime( $this->runtime ); + $this->step_runner->setResourceManager( $this->resource_manager ); - $this->file_system = new Filesystem(); + $this->filesystem = new Filesystem(); } /** * @after */ public function after() { - $this->file_system->remove( $this->document_root ); + $this->filesystem->remove( $this->document_root ); } - public function test(){ - $this->assertTrue(true); // Placeholder until true test are fixed. + public function testUnzipFileWhenUsingAbsolutePath() { + $zip = __DIR__ . '/resources/test_zip.zip'; + $this->resource_manager->method( 'getStream' ) + ->willReturn( fopen( $zip, 'rb' ) ); + + $step = new UnzipStep(); + $step->setZipFile( $zip ); + $extracted_file_path = $this->runtime->resolvePath( 'dir/test_zip.txt' ); + $step->setExtractToPath( Path::getDirectory( $extracted_file_path ) ); + + $this->step_runner->run( $step, new Tracker() ); + + self::assertFileEquals( __DIR__ . '/resources/test_zip.txt', $extracted_file_path ); } -// public function testUnzipFileWhenUsingAbsolutePath() { -// $zip = __DIR__ . '/resources/test_zip.zip'; -// $this->resource_manager->method( 'getStream' ) -// ->willReturn( fopen( $zip, 'rb' ) ); -// -// $input = new UnzipStep(); -// $input->setZipFile( $zip ); -// $extracted_file_path = $this->runtime->resolvePath( 'dir/test_zip.txt' ); -// $input->setExtractToPath( Path::getDirectory( $extracted_file_path ) ); -// -// $this->step->run( $input, new Tracker() ); -// -// $this->assertFileEquals( __DIR__ . '/resources/test_zip.txt', $extracted_file_path ); -// } -// -// public function testUnzipFileWhenUsingRelativePath() { -// $zip = __DIR__ . '/resources/test_zip.zip'; -// $this->resource_manager->method( 'getStream' ) -// ->willReturn( fopen( $zip, 'rb' ) ); -// -// $input = new UnzipStep(); -// $input->setZipFile( $zip ); -// $input->setExtractToPath( 'dir' ); -// -// $this->step->run( $input, new Tracker() ); -// -// $extracted_file_path = $this->runtime->resolvePath( 'dir/test_zip.txt' ); -// $this->assertFileEquals( __DIR__ . '/resources/test_zip.txt', $extracted_file_path ); -// } + public function testUnzipFileWhenUsingRelativePath() { + $zip = __DIR__ . '/resources/test_zip.zip'; + $this->resource_manager->method( 'getStream' ) + ->willReturn( fopen( $zip, 'rb' ) ); + + $input = new UnzipStep(); + $input->setZipFile( $zip ); + $input->setExtractToPath( 'dir' ); + + $this->step_runner->run( $input, new Tracker() ); + + $extracted_file_path = $this->runtime->resolvePath( 'dir/test_zip.txt' ); + self::assertFileEquals( __DIR__ . '/resources/test_zip.txt', $extracted_file_path ); + } } diff --git a/tests/unit/zip/ZipFunctionsTest.php b/tests/unit/zip/ZipFunctionsTest.php new file mode 100644 index 00000000..a25031a6 --- /dev/null +++ b/tests/unit/zip/ZipFunctionsTest.php @@ -0,0 +1,27 @@ +aa(Lg!^#9;sc8#ycH literal 0 HcmV?d00001 diff --git a/tests/unit/zip/resources/zip-symlinks-test.zip b/tests/unit/zip/resources/zip-symlinks-test.zip new file mode 100644 index 0000000000000000000000000000000000000000..594333895d4db1d4b85df1a40bde964e8123fb1d GIT binary patch literal 170 zcmWIWW@h1H0D;P7H4#&#D-A(B5N2hNVaUiYE-4NT;bdSAJH?t70mP*h+zgB?FPIq^ zz=VEkNwPjzRe(1mlN>WHqa;8!fWiNTAPQ Date: Thu, 28 Mar 2024 22:18:17 +0100 Subject: [PATCH 2/4] Test symlinks --- tests/unit/zip/ZipFunctionsTest.php | 32 ++++++++++++++---- tests/unit/zip/resources/test.zip | Bin 0 -> 240 bytes tests/unit/zip/resources/zip-symlink-test.zip | Bin 0 -> 120 bytes 3 files changed, 25 insertions(+), 7 deletions(-) create mode 100644 tests/unit/zip/resources/test.zip create mode 100644 tests/unit/zip/resources/zip-symlink-test.zip diff --git a/tests/unit/zip/ZipFunctionsTest.php b/tests/unit/zip/ZipFunctionsTest.php index a25031a6..29acf2ad 100644 --- a/tests/unit/zip/ZipFunctionsTest.php +++ b/tests/unit/zip/ZipFunctionsTest.php @@ -11,17 +11,35 @@ public function testThrowsExceptionWhenZipContainsFilesWithRelativePaths() { // zipped file named: "../../../../../../../../tmp/zip-slip-test.txt" $zip = __DIR__ . '/resources/zip-slip-test.zip'; - self::expectException(ZipException::class); - self::expectExceptionMessage("Relative paths in zips are not allowed."); - zip_extract_to(fopen($zip, 'rb'), dirname($zip)); + self::expectException( ZipException::class ); + self::expectExceptionMessage( "Relative paths in zips are not allowed." ); + zip_extract_to( fopen( $zip, 'rb' ), dirname( $zip ) ); } - public function testThrowsExceptionWhenZipContainsFilesWithSymlinks() { + public function testThrowsExceptionWhenZipContainsFilesWithSymlinks1() { + // zipped semantic link + $zip = __DIR__ . '/resources/test.zip'; + + self::expectException( ZipException::class ); + self::expectExceptionMessage( "Relative paths in zips are not allowed." ); + zip_extract_to( fopen( $zip, 'rb' ), dirname( $zip ) ); + } + + public function testThrowsExceptionWhenZipContainsFilesWithSymlinks2() { + // zipped semantic link + $zip = __DIR__ . '/resources/zip-symlink-test.zip'; + + self::expectException( ZipException::class ); + self::expectExceptionMessage( "Relative paths in zips are not allowed." ); + zip_extract_to( fopen( $zip, 'rb' ), dirname( $zip ) ); + } + + public function testThrowsExceptionWhenZipContainsFilesWithSymlinks3() { // zipped semantic link $zip = __DIR__ . '/resources/zip-symlinks-test.zip'; - self::expectException(ZipException::class); - self::expectExceptionMessage("Relative paths in zips are not allowed."); - zip_extract_to(fopen($zip, 'rb'), dirname($zip)); + self::expectException( ZipException::class ); + self::expectExceptionMessage( "Relative paths in zips are not allowed." ); + zip_extract_to( fopen( $zip, 'rb' ), dirname( $zip ) ); } } diff --git a/tests/unit/zip/resources/test.zip b/tests/unit/zip/resources/test.zip new file mode 100644 index 0000000000000000000000000000000000000000..75e070b7244a93f2902c0c84566409e7792bc3fc GIT binary patch literal 240 zcmWIWW@Zs#U|`^2NZ42tF=Io{CLSOUggJn?B(=CCCMmJ#_kaCfMR|@&$sH050dO5z zXgYY2bj0Um=4GR5V`LIxz-<*!FA{)R9N>-6h0QvI79$`NrVDI&fHx}}NSX-<3xRYd Hh{FH?su(c0 literal 0 HcmV?d00001 diff --git a/tests/unit/zip/resources/zip-symlink-test.zip b/tests/unit/zip/resources/zip-symlink-test.zip new file mode 100644 index 0000000000000000000000000000000000000000..5a01069ea1e3a45d3a6d305539f13febacd4334a GIT binary patch literal 120 zcmWIWW@Zs#0D+9PH4$J2l;8%^#g(}^nR(fIB^4zB-i%Bl47imdYemz@$_A2T1VT$7 ItqE2M0Q>b1nE(I) literal 0 HcmV?d00001 From d4ff5fefb4c8d15b457655b4f71c2ded3571935e Mon Sep 17 00:00:00 2001 From: Michael Reichardt Date: Sat, 30 Mar 2024 15:05:51 +0100 Subject: [PATCH 3/4] Add evil zip creation in tests --- composer.json | 6 +- src/WordPress/Zip/functions.php | 4 +- tests/unit/zip/ZipFunctionsTest.php | 61 ++++++++++++------ tests/unit/zip/resources/test.zip | Bin 240 -> 0 bytes tests/unit/zip/resources/zip-slip-test.zip | Bin 201 -> 0 bytes tests/unit/zip/resources/zip-symlink-test.zip | Bin 120 -> 0 bytes .../unit/zip/resources/zip-symlinks-test.zip | Bin 170 -> 0 bytes 7 files changed, 45 insertions(+), 26 deletions(-) delete mode 100644 tests/unit/zip/resources/test.zip delete mode 100644 tests/unit/zip/resources/zip-slip-test.zip delete mode 100644 tests/unit/zip/resources/zip-symlink-test.zip delete mode 100644 tests/unit/zip/resources/zip-symlinks-test.zip diff --git a/composer.json b/composer.json index 16013aaf..7ba219f7 100644 --- a/composer.json +++ b/composer.json @@ -2,11 +2,11 @@ "prefer-stable": true, "require": { "ext-json": "*", - "php": ">=7.0", - "ext-zip": "*" + "php": ">=7.0" }, "require-dev": { - "yoast/phpunit-polyfills": "2.0.0" + "yoast/phpunit-polyfills": "2.0.0", + "ext-zip": "*" }, "config": { "optimize-autoloader": true, diff --git a/src/WordPress/Zip/functions.php b/src/WordPress/Zip/functions.php index 845365b5..a3560fa1 100644 --- a/src/WordPress/Zip/functions.php +++ b/src/WordPress/Zip/functions.php @@ -27,8 +27,8 @@ function zip_extract_to( $fp, $to_path ) { $path = Path::canonicalize( $to_path . '/' . $entry->path ); $parent = Path::getDirectory( $path ); if ( ! is_dir( $parent ) ) { - if(is_file($parent)) { - unlink($parent); + if( is_file( $parent ) ) { + unlink( $parent ); } mkdir( $parent, 0777, true ); } diff --git a/tests/unit/zip/ZipFunctionsTest.php b/tests/unit/zip/ZipFunctionsTest.php index 29acf2ad..cae2cd12 100644 --- a/tests/unit/zip/ZipFunctionsTest.php +++ b/tests/unit/zip/ZipFunctionsTest.php @@ -3,43 +3,62 @@ namespace unit\zip; use PHPUnitTestCase; +use Symfony\Component\Filesystem\Filesystem; use WordPress\Zip\ZipException; +use ZipArchive; use function WordPress\Zip\zip_extract_to; class ZipFunctionsTest extends PHPUnitTestCase { - public function testThrowsExceptionWhenZipContainsFilesWithRelativePaths() { - // zipped file named: "../../../../../../../../tmp/zip-slip-test.txt" - $zip = __DIR__ . '/resources/zip-slip-test.zip'; - self::expectException( ZipException::class ); - self::expectExceptionMessage( "Relative paths in zips are not allowed." ); - zip_extract_to( fopen( $zip, 'rb' ), dirname( $zip ) ); - } + /** + * @var string name of the temporary directory for handling zips in tests + */ + const TMP_DIRECTORY = __DIR__ . "/tmp"; - public function testThrowsExceptionWhenZipContainsFilesWithSymlinks1() { - // zipped semantic link - $zip = __DIR__ . '/resources/test.zip'; + /** + * @var Filesystem + */ + private $filesystem; - self::expectException( ZipException::class ); - self::expectExceptionMessage( "Relative paths in zips are not allowed." ); - zip_extract_to( fopen( $zip, 'rb' ), dirname( $zip ) ); + /** + * @before + */ + public function before() { + $this->filesystem = new Filesystem(); + $this->filesystem->mkdir( self::TMP_DIRECTORY ); } - public function testThrowsExceptionWhenZipContainsFilesWithSymlinks2() { - // zipped semantic link - $zip = __DIR__ . '/resources/zip-symlink-test.zip'; + /** + * @after + */ + public function after(){ + $this->filesystem->remove( self::TMP_DIRECTORY ); + } + + public function testThrowsExceptionWhenZipContainsFilesWithRelativePaths() { + $filename = self::TMP_DIRECTORY . '/zip-slip-test.zip'; + + $zip = new ZipArchive(); + $zip->open( $filename, ZipArchive::CREATE ); + $zip->addFromString( "../../evilfile.txt","zip-slip-test" ); + $zip->close(); self::expectException( ZipException::class ); self::expectExceptionMessage( "Relative paths in zips are not allowed." ); - zip_extract_to( fopen( $zip, 'rb' ), dirname( $zip ) ); + zip_extract_to( fopen( $filename, 'rb' ), dirname( $filename ) ); } - public function testThrowsExceptionWhenZipContainsFilesWithSymlinks3() { - // zipped semantic link - $zip = __DIR__ . '/resources/zip-symlinks-test.zip'; + public function testThrowsExceptionWhenZipContainsFilesWithSymlinks() { + $filename = self::TMP_DIRECTORY . '/zip-symlink-test.zip'; + + $zip = new ZipArchive(); + $zip->open( $filename, ZipArchive::CREATE ); + $symlink = symlink( self::TMP_DIRECTORY, 'evillink.txt' ); + $zip->addFromString( $symlink,"zip-symlink-test" ); + $zip->close(); self::expectException( ZipException::class ); self::expectExceptionMessage( "Relative paths in zips are not allowed." ); - zip_extract_to( fopen( $zip, 'rb' ), dirname( $zip ) ); + zip_extract_to( fopen( $filename, 'rb' ), dirname( $filename ) ); } } diff --git a/tests/unit/zip/resources/test.zip b/tests/unit/zip/resources/test.zip deleted file mode 100644 index 75e070b7244a93f2902c0c84566409e7792bc3fc..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 240 zcmWIWW@Zs#U|`^2NZ42tF=Io{CLSOUggJn?B(=CCCMmJ#_kaCfMR|@&$sH050dO5z zXgYY2bj0Um=4GR5V`LIxz-<*!FA{)R9N>-6h0QvI79$`NrVDI&fHx}}NSX-<3xRYd Hh{FH?su(c0 diff --git a/tests/unit/zip/resources/zip-slip-test.zip b/tests/unit/zip/resources/zip-slip-test.zip deleted file mode 100644 index b8ea6a890d601629263c5b1032be7631a8940106..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 201 zcmWIWW@Zs#0D;WNnuxixkH6prvO!oEi1qaJv7(aP0{yDY0^Q;qAW@Q9T%uP}Q34cD v00}671OmJnne3Tyn+!A@47N3bD6EzM1+m%_;LXYg(!>aa(Lg!^#9;sc8#ycH diff --git a/tests/unit/zip/resources/zip-symlink-test.zip b/tests/unit/zip/resources/zip-symlink-test.zip deleted file mode 100644 index 5a01069ea1e3a45d3a6d305539f13febacd4334a..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 120 zcmWIWW@Zs#0D+9PH4$J2l;8%^#g(}^nR(fIB^4zB-i%Bl47imdYemz@$_A2T1VT$7 ItqE2M0Q>b1nE(I) diff --git a/tests/unit/zip/resources/zip-symlinks-test.zip b/tests/unit/zip/resources/zip-symlinks-test.zip deleted file mode 100644 index 594333895d4db1d4b85df1a40bde964e8123fb1d..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 170 zcmWIWW@h1H0D;P7H4#&#D-A(B5N2hNVaUiYE-4NT;bdSAJH?t70mP*h+zgB?FPIq^ zz=VEkNwPjzRe(1mlN>WHqa;8!fWiNTAPQ Date: Sat, 30 Mar 2024 15:09:35 +0100 Subject: [PATCH 4/4] Add evil zip creation in tests --- src/WordPress/Zip/functions.php | 4 ++-- tests/unit/zip/ZipFunctionsTest.php | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/WordPress/Zip/functions.php b/src/WordPress/Zip/functions.php index a3560fa1..51374391 100644 --- a/src/WordPress/Zip/functions.php +++ b/src/WordPress/Zip/functions.php @@ -16,12 +16,12 @@ function zip_extract_to( $fp, $to_path ) { // prevent zip slip -> using relative path to access otherwise inaccessible files if ( false !== strpos( $entry->path ,'..') ) { - throw new ZipException("Relative paths in zips are not allowed."); + throw new ZipException( "Relative paths in zips are not allowed." ); } // prevent zip with symlinks -> using a symbolic link to access otherwise inaccessible files if ( is_link( $entry->path ) ) { - throw new ZipException("Semantic links in zips are not allowed."); + throw new ZipException( "Semantic links in zips are not allowed." ); } $path = Path::canonicalize( $to_path . '/' . $entry->path ); diff --git a/tests/unit/zip/ZipFunctionsTest.php b/tests/unit/zip/ZipFunctionsTest.php index cae2cd12..3cc4cff9 100644 --- a/tests/unit/zip/ZipFunctionsTest.php +++ b/tests/unit/zip/ZipFunctionsTest.php @@ -54,11 +54,11 @@ public function testThrowsExceptionWhenZipContainsFilesWithSymlinks() { $zip = new ZipArchive(); $zip->open( $filename, ZipArchive::CREATE ); $symlink = symlink( self::TMP_DIRECTORY, 'evillink.txt' ); - $zip->addFromString( $symlink,"zip-symlink-test" ); + $zip->addFile( $symlink,"zip-symlink-test" ); $zip->close(); self::expectException( ZipException::class ); - self::expectExceptionMessage( "Relative paths in zips are not allowed." ); + self::expectExceptionMessage( "Semantic links in zips are not allowed." ); zip_extract_to( fopen( $filename, 'rb' ), dirname( $filename ) ); } }