feat: added check to prevent Zip Slip attacks

thorsten · thorsten · commit d3d0fd8ff54b · 2026-01-15T21:43:47.000+01:00
diff --git a/phpmyfaq/src/phpMyFAQ/Setup/Upgrade.php b/phpmyfaq/src/phpMyFAQ/Setup/Upgrade.php
@@ -223,13 +223,96 @@ public function extractPackage(string $path, callable $progressCallback): bool
         });
 
         if ($zipFile) {
-            $zipArchive->extractTo($this->upgradeDirectory . '/new/');
+            // Secure extraction to prevent Zip Slip vulnerability
+            $extractPath = $this->upgradeDirectory . '/new/';
+            $this->secureExtractZip($zipArchive, $extractPath);
             return $zipArchive->close();
         }
 
         throw new Exception(message: 'Cannot open zipped download package.');
     }
 
+    /**
+     * Securely extracts a ZIP archive, preventing Zip Slip attacks
+     *
+     * @param ZipArchive $zipArchive The ZIP archive to extract
+     * @param string $destination The destination directory
+     * @throws Exception If a malicious path is detected
+     */
+    private function secureExtractZip(ZipArchive $zipArchive, string $destination): void
+    {
+        // Normalize destination path
+        $destination = rtrim(realpath($destination) ?: $destination, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR;
+
+        // Create destination directory if it doesn't exist
+        if (!is_dir($destination)) {
+            mkdir($destination, 0755, true);
+        }
+
+        // Iterate through all entries in the archive
+        for ($i = 0; $i < $zipArchive->numFiles; $i++) {
+            $entry = $zipArchive->getNameIndex($i);
+            if ($entry === false) {
+                continue;
+            }
+
+            // Validate the entry path to prevent directory traversal
+            if (!$this->isPathSafe($entry, $destination)) {
+                $this->configuration->getLogger()->error('Zip Slip attack detected in package', [
+                    'malicious_entry' => $entry,
+                ]);
+                throw new Exception(message: sprintf('Malicious path detected in archive: %s', $entry));
+            }
+
+            // Extract individual file
+            $zipArchive->extractTo($destination, $entry);
+        }
+    }
+
+    /**
+     * Validates that a ZIP entry path is safe and doesn't escape the destination directory
+     *
+     * @param string $entryPath The path from the ZIP entry
+     * @param string $destination The destination directory
+     * @return bool True if path is safe, false otherwise
+     */
+    private function isPathSafe(string $entryPath, string $destination): bool
+    {
+        // Remove any null bytes
+        $entryPath = str_replace("\0", '', $entryPath);
+
+        // Build the full destination path
+        $fullPath = $destination . $entryPath;
+
+        // Resolve the real path (this resolves .. and . sequences)
+        $realPath = realpath(dirname($fullPath));
+        if ($realPath === false) {
+            // Path doesn't exist yet, construct it manually
+            $realPath = realpath($destination) . DIRECTORY_SEPARATOR . dirname($entryPath);
+        }
+
+        // Normalize both paths for comparison
+        $normalizedDestination = rtrim(realpath($destination) ?: $destination, DIRECTORY_SEPARATOR);
+        $normalizedPath = rtrim($realPath, DIRECTORY_SEPARATOR);
+
+        // Check if the resolved path is within the destination directory
+        if (!str_starts_with($normalizedPath, $normalizedDestination)) {
+            return false;
+        }
+
+        // Additional check: reject paths with directory traversal sequences
+        if (preg_match('#(\.\./)|(\.\.)|(\./)|(^/)#', $entryPath)) {
+            return false;
+        }
+
+        // Check for absolute paths (Unix and Windows)
+        if (str_starts_with($entryPath, '/') || preg_match('#^[a-zA-Z]:#', $entryPath)) {
+            return false;
+        }
+
+        return true;
+    }
+
     /**
      * Method to create a temporary backup of the current files
      *
diff --git a/tests/phpMyFAQ/Setup/UpgradeTest.php b/tests/phpMyFAQ/Setup/UpgradeTest.php
@@ -2,6 +2,7 @@
 
 namespace phpMyFAQ\Setup;
 
+use Monolog\Logger;
 use phpMyFAQ\Configuration;
 use phpMyFAQ\Core\Exception;
 use phpMyFAQ\Database\Sqlite3;
@@ -10,14 +11,17 @@
 use PHPUnit\Framework\Attributes\AllowMockObjectsWithoutExpectations;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
+use ReflectionClass;
 use Symfony\Contracts\HttpClient\HttpClientInterface;
 use Symfony\Contracts\HttpClient\ResponseInterface;
+use ZipArchive;
 
 #[AllowMockObjectsWithoutExpectations]
 class UpgradeTest extends TestCase
 {
     private Upgrade $upgrade;
     private HttpClientInterface $httpClientMock;
+    private string $testDir;
 
     protected function setUp(): void
     {
@@ -30,6 +34,43 @@ protected function setUp(): void
         $this->httpClientMock = $this->createMock(HttpClientInterface::class);
         $this->upgrade = new Upgrade(new System(), $configuration, $this->httpClientMock);
         $this->upgrade->setUpgradeDirectory(PMF_CONTENT_DIR . '/upgrades');
+
+        // Setup test directory for Zip Slip tests
+        $this->testDir = sys_get_temp_dir() . '/zip_slip_test_' . uniqid();
+        mkdir($this->testDir);
+        mkdir($this->testDir . '/extract');
+    }
+
+    protected function tearDown(): void
+    {
+        parent::tearDown();
+
+        // Clean up test files
+        if (is_dir($this->testDir)) {
+            $this->recursiveDelete($this->testDir);
+        }
+    }
+
+    private function recursiveDelete(string $dir): void
+    {
+        if (!is_dir($dir)) {
+            return;
+        }
+
+        $items = scandir($dir);
+        foreach ($items as $item) {
+            if ($item === '.' || $item === '..') {
+                continue;
+            }
+
+            $path = $dir . '/' . $item;
+            if (is_dir($path)) {
+                $this->recursiveDelete($path);
+            } else {
+                unlink($path);
+            }
+        }
+        rmdir($dir);
     }
 
     /**
@@ -125,4 +166,153 @@ public function testGetPathForNonNightly(): void
 
         $this->assertEquals('', $this->upgrade->getPath());
     }
+
+    // Zip Slip vulnerability tests
+
+    public function testIsPathSafeRejectsDotDotSlash(): void
+    {
+        $reflection = new ReflectionClass($this->upgrade);
+        $method = $reflection->getMethod('isPathSafe');
+
+        $result = $method->invoke($this->upgrade, '../../../etc/passwd', $this->testDir . '/extract/');
+        $this->assertFalse($result);
+    }
+
+    public function testIsPathSafeRejectsDotDot(): void
+    {
+        $reflection = new ReflectionClass($this->upgrade);
+        $method = $reflection->getMethod('isPathSafe');
+
+        $result = $method->invoke($this->upgrade, 'subdir/../../etc/passwd', $this->testDir . '/extract/');
+        $this->assertFalse($result);
+    }
+
+    public function testIsPathSafeRejectsAbsolutePath(): void
+    {
+        $reflection = new ReflectionClass($this->upgrade);
+        $method = $reflection->getMethod('isPathSafe');
+
+        $result = $method->invoke($this->upgrade, '/etc/passwd', $this->testDir . '/extract/');
+        $this->assertFalse($result);
+    }
+
+    public function testIsPathSafeRejectsWindowsAbsolutePath(): void
+    {
+        $reflection = new ReflectionClass($this->upgrade);
+        $method = $reflection->getMethod('isPathSafe');
+
+        $result = $method->invoke($this->upgrade, 'C:\\Windows\\System32\\evil.exe', $this->testDir . '/extract/');
+        $this->assertFalse($result);
+    }
+
+    public function testIsPathSafeAcceptsSafePath(): void
+    {
+        $reflection = new ReflectionClass($this->upgrade);
+        $method = $reflection->getMethod('isPathSafe');
+
+        $result = $method->invoke($this->upgrade, 'subdir/file.txt', $this->testDir . '/extract/');
+        $this->assertTrue($result);
+    }
+
+    public function testIsPathSafeAcceptsSafeNestedPath(): void
+    {
+        $reflection = new ReflectionClass($this->upgrade);
+        $method = $reflection->getMethod('isPathSafe');
+
+        $result = $method->invoke($this->upgrade, 'a/b/c/d/file.txt', $this->testDir . '/extract/');
+        $this->assertTrue($result);
+    }
+
+    public function testIsPathSafeRemovesNullBytes(): void
+    {
+        $reflection = new ReflectionClass($this->upgrade);
+        $method = $reflection->getMethod('isPathSafe');
+
+        $result = $method->invoke($this->upgrade, "file.txt\0.php", $this->testDir . '/extract/');
+        // Should still be safe after null byte removal
+        $this->assertTrue($result);
+    }
+
+    public function testSecureExtractZipRejectsMaliciousArchive(): void
+    {
+        // Create a malicious ZIP archive
+        $zipPath = $this->testDir . '/malicious.zip';
+        $zip = new ZipArchive();
+        $zip->open($zipPath, ZipArchive::CREATE);
+
+        // Add a file with directory traversal
+        $zip->addFromString('../../../evil.txt', 'malicious content');
+        $zip->close();
+
+        // Try to extract
+        $zip->open($zipPath);
+
+        $reflection = new ReflectionClass($this->upgrade);
+        $method = $reflection->getMethod('secureExtractZip');
+
+        $this->expectException(Exception::class);
+        $this->expectExceptionMessage('Malicious path detected in archive');
+
+        $method->invoke($this->upgrade, $zip, $this->testDir . '/extract/');
+    }
+
+    public function testSecureExtractZipAllowsSafeArchive(): void
+    {
+        // Create a safe ZIP archive
+        $zipPath = $this->testDir . '/safe.zip';
+        $zip = new ZipArchive();
+        $zip->open($zipPath, ZipArchive::CREATE);
+
+        // Add safe files
+        $zip->addFromString('file1.txt', 'content 1');
+        $zip->addFromString('subdir/file2.txt', 'content 2');
+        $zip->close();
+
+        // Extract
+        $zip->open($zipPath);
+
+        $reflection = new ReflectionClass($this->upgrade);
+        $method = $reflection->getMethod('secureExtractZip');
+
+        $method->invoke($this->upgrade, $zip, $this->testDir . '/extract/');
+        $zip->close();
+
+        // Verify files were extracted
+        $this->assertFileExists($this->testDir . '/extract/file1.txt');
+        $this->assertFileExists($this->testDir . '/extract/subdir/file2.txt');
+        $this->assertEquals('content 1', file_get_contents($this->testDir . '/extract/file1.txt'));
+        $this->assertEquals('content 2', file_get_contents($this->testDir . '/extract/subdir/file2.txt'));
+    }
+
+    public function testSecureExtractZipDoesNotEscapeDirectory(): void
+    {
+        // Create a malicious ZIP that tries to escape
+        $zipPath = $this->testDir . '/escape.zip';
+        $zip = new ZipArchive();
+        $zip->open($zipPath, ZipArchive::CREATE);
+
+        // Try various escape techniques
+        $zip->addFromString('../../outside.txt', 'should not be here');
+        $zip->addFromString('a/../../../outside2.txt', 'should not be here');
+        $zip->close();
+
+        // Try to extract
+        $zip->open($zipPath);
+
+        $reflection = new ReflectionClass($this->upgrade);
+        $method = $reflection->getMethod('secureExtractZip');
+
+        try {
+            $method->invoke($this->upgrade, $zip, $this->testDir . '/extract/');
+            $this->fail('Should have thrown an exception');
+        } catch (Exception $e) {
+            $this->assertStringContainsString('Malicious path detected', $e->getMessage());
+        }
+
+        $zip->close();
+
+        // Verify files were NOT created outside the extract directory
+        $this->assertFileDoesNotExist($this->testDir . '/outside.txt');
+        $this->assertFileDoesNotExist($this->testDir . '/outside2.txt');
+    }
 }
