diff --git a/app/Http/Requests/Photo/UploadPhotoRequest.php b/app/Http/Requests/Photo/UploadPhotoRequest.php index 50627bb519f..2a27dfbe49a 100644 --- a/app/Http/Requests/Photo/UploadPhotoRequest.php +++ b/app/Http/Requests/Photo/UploadPhotoRequest.php @@ -20,6 +20,7 @@ use App\Rules\AlbumIDRule; use App\Rules\DescriptionRule; use App\Rules\ExtensionRule; +use App\Rules\FilenameRule; use App\Rules\FileUuidRule; use App\Rules\TitleRule; use Illuminate\Http\UploadedFile; @@ -56,7 +57,7 @@ public function rules(): array RequestAttribute::ALBUM_ID_ATTRIBUTE => ['present', new AlbumIDRule(true)], RequestAttribute::FILE_LAST_MODIFIED_TIME => 'sometimes|nullable|numeric', RequestAttribute::FILE_ATTRIBUTE => ['required', 'file'], - 'file_name' => 'required|string', + 'file_name' => ['required', new FilenameRule()], 'uuid_name' => ['present', new FileUuidRule()], 'extension' => ['present', new ExtensionRule()], 'chunk_number' => 'required|integer|min:1', diff --git a/app/Jobs/ExtractZip.php b/app/Jobs/ExtractZip.php index 44288eddbcd..0e894b3aaf2 100644 --- a/app/Jobs/ExtractZip.php +++ b/app/Jobs/ExtractZip.php @@ -12,6 +12,7 @@ use App\Contracts\Models\AbstractAlbum; use App\DTO\ImportMode; use App\Enum\JobStatus; +use App\Exceptions\Internal\LycheeLogicException; use App\Exceptions\Internal\ZipBombDetectedException; use App\Exceptions\Internal\ZipExtractionException; use App\Exceptions\ZipInvalidException; @@ -33,6 +34,7 @@ use Illuminate\Support\Facades\Storage; use Illuminate\Support\Str; use function Safe\date; +use function Safe\realpath; use function Safe\unlink; class ExtractZip implements ShouldQueue @@ -52,6 +54,7 @@ class ExtractZip implements ShouldQueue public ?int $file_last_modified_time; private FileExtensionService $file_extension_service; + private ?string $base_extract_dir = null; /** * Create a new job instance. @@ -85,9 +88,11 @@ public function handle(): void $this->history->status = JobStatus::STARTED; $this->history->save(); + $this->base_extract_dir = realpath(Storage::disk('extract-jobs')->path('')); $this->validate_zip(); $path_extracted = Storage::disk('extract-jobs')->path(date('Ymd') . ' ' . $this->getExtractFolderName()); + $this->extract_zip($path_extracted); $config_manager = app(ConfigManager::class); @@ -166,10 +171,17 @@ private function should_import_from_extracted(string $path_extracted): bool * Builds a SafeZipExtractor configured from the "Image Processing" zip-bomb * protection settings (expert settings, in the admin config). * + * @param string $base_extract_dir The base directory where the ZIP file will be extracted. + * This is used to prevent path traversal attacks (zip slip). + * * @return SafeZipExtractor */ - private function makeSafeZipExtractor(): SafeZipExtractor + private function makeSafeZipExtractor(?string $base_extract_dir): SafeZipExtractor { + if ($base_extract_dir === null) { + throw new LycheeLogicException('Base extract directory must be set before creating SafeZipExtractor.'); + } + $config_manager = app(ConfigManager::class); return new SafeZipExtractor( @@ -177,6 +189,7 @@ private function makeSafeZipExtractor(): SafeZipExtractor max_file_size: $config_manager->getValueAsByteSize('zip_bomb_max_file_size'), max_entries: $config_manager->getValueAsInt('zip_bomb_max_entries'), max_ratio: $config_manager->getValueAsInt('zip_bomb_max_ratio'), + base_extract_dir: $base_extract_dir, ); } @@ -248,7 +261,7 @@ private function validate_zip(): void } try { - $is_safe = $this->makeSafeZipExtractor()->inspect($this->file_path); + $is_safe = $this->makeSafeZipExtractor($this->base_extract_dir)->inspect($this->file_path); } catch (\Throwable $e) { Log::channel('jobs')->critical('Zip file ' . $this->file_path . ' could not be inspected: ' . $e->getMessage()); $is_safe = false; @@ -291,7 +304,7 @@ private function validate_zip(): void private function extract_zip(string $path_extracted): void { try { - $this->makeSafeZipExtractor()->extract($this->file_path, $path_extracted); + $this->makeSafeZipExtractor($this->base_extract_dir)->extract($this->file_path, $path_extracted); } catch (ZipBombDetectedException $e) { Log::channel('jobs')->critical('Zip file ' . $this->file_path . ' exceeds the configured zip-bomb protection limits: ' . $e->getMessage()); diff --git a/app/Rules/FilenameRule.php b/app/Rules/FilenameRule.php new file mode 100644 index 00000000000..dfc3996f1ef --- /dev/null +++ b/app/Rules/FilenameRule.php @@ -0,0 +1,60 @@ +contains(['..', '/', '\\'])) { + $fail(':attribute contains invalid characters:.'); + + return; + } + } +} diff --git a/app/Services/Zip/SafeZipExtractor.php b/app/Services/Zip/SafeZipExtractor.php index f602dbc6d45..64d1d1f789b 100644 --- a/app/Services/Zip/SafeZipExtractor.php +++ b/app/Services/Zip/SafeZipExtractor.php @@ -46,6 +46,8 @@ public function __construct( private int $max_ratio, // Read chunk size while streaming. private int $chunk_size = 8192, + // Base extract directory (for path traversal checks). + private string $base_extract_dir = '', ) { } @@ -110,9 +112,21 @@ public function extract(string $zip_path, string $dest_dir): void throw new ZipBombDetectedException('Archive rejected by pre-flight inspection.'); } + $dest_dir = $this->resolveTarget($dest_dir); + if ($dest_dir === null) { + throw new \RuntimeException('Invalid destination directory.'); + } + // Ensure the destination directory is absolute and starts with a slash. + $dest_dir = DIRECTORY_SEPARATOR . ltrim($dest_dir, DIRECTORY_SEPARATOR); + + if ($this->base_extract_dir !== '' && !str_starts_with($dest_dir, $this->base_extract_dir)) { + throw new \RuntimeException('Destination directory is outside the allowed base directory.'); + } + if (!is_dir($dest_dir)) { mkdir($dest_dir, 0755, true); } + $base_real = realpath($dest_dir); $zip = new \ZipArchive(); @@ -200,6 +214,24 @@ public function extract(string $zip_path, string $dest_dir): void * $base_real, or null if the entry tries to escape (zip slip). */ private function resolveSafeTarget(string $base_real, string $entry_name): ?string + { + $target = $this->resolveTarget($entry_name); + if ($target === null) { + // Invalid entry name (empty, null byte, or path traversal). + return null; + } + + return $base_real . DIRECTORY_SEPARATOR . $target; + } + + /** + * Resolve the target path while getting rid of any path traversal attempts. + * + * @param string $entry_name + * + * @return string|null the resolved path relative to the base directory, or null if the entry is invalid or tries to escape + */ + private function resolveTarget(string $entry_name): string|null { if ($entry_name === '' || str_contains($entry_name, "\0")) { return null; @@ -228,13 +260,6 @@ private function resolveSafeTarget(string $base_real, string $entry_name): ?stri return null; } - $target = $base_real . DIRECTORY_SEPARATOR . implode(DIRECTORY_SEPARATOR, $parts); - - // Final belt-and-braces prefix check. - if (!str_starts_with($target, $base_real . DIRECTORY_SEPARATOR)) { - return null; - } - - return $target; + return implode(DIRECTORY_SEPARATOR, $parts); } } diff --git a/tests/Unit/Jobs/ExtractZipTest.php b/tests/Unit/Jobs/ExtractZipTest.php index e73f4ea2fb7..3754457677d 100644 --- a/tests/Unit/Jobs/ExtractZipTest.php +++ b/tests/Unit/Jobs/ExtractZipTest.php @@ -68,6 +68,14 @@ private function makeFile(): ProcessableJobFile private function callValidateZip(ExtractZip $job): void { + // `base_extract_dir` is normally set by handle() before validate_zip() + // runs. Since these tests invoke validate_zip() directly (handle() + // performs a real import that isn't unit-testable), it must be seeded + // here, or makeSafeZipExtractor() throws and validate_zip() silently + // treats the archive as unsafe. + $base_extract_dir = new \ReflectionProperty(ExtractZip::class, 'base_extract_dir'); + $base_extract_dir->setValue($job, sys_get_temp_dir()); + $method = new \ReflectionMethod(ExtractZip::class, 'validate_zip'); $method->invoke($job); } diff --git a/tests/Unit/Rules/FilenameRuleTest.php b/tests/Unit/Rules/FilenameRuleTest.php new file mode 100644 index 00000000000..6bb70ff5ea1 --- /dev/null +++ b/tests/Unit/Rules/FilenameRuleTest.php @@ -0,0 +1,100 @@ +validate('file_name', $value, function () use (&$failed): void { $failed = true; }); + self::assertFalse($failed, 'Expected "' . var_export($value, true) . '" to pass but it failed.'); + } + + private function assertFails(mixed $value, ?string $expected_message = null): void + { + $rule = new FilenameRule(); + $msg = null; + $rule->validate('file_name', $value, function ($message) use (&$msg): void { $msg = $message; }); + self::assertNotNull($msg, 'Expected "' . var_export($value, true) . '" to fail but it passed.'); + if ($expected_message !== null) { + self::assertSame($expected_message, $msg); + } + } + + public function testValidFilenames(): void + { + $this->assertPasses('test.jpg'); + $this->assertPasses('IMG_1234.png'); + $this->assertPasses('.hidden'); + $this->assertPasses('file with spaces.jpg'); + $this->assertPasses('café.jpg'); + $this->assertPasses('a'); + $this->assertPasses('file.tar.gz'); + } + + public function testNonStringValues(): void + { + $this->assertFails(123, ':attribute is not a string.'); + $this->assertFails(1.5, ':attribute is not a string.'); + $this->assertFails(true, ':attribute is not a string.'); + $this->assertFails(null, ':attribute is not a string.'); + $this->assertFails([], ':attribute is not a string.'); + $this->assertFails(['file.jpg'], ':attribute is not a string.'); + } + + public function testContainsDirectory(): void + { + $this->assertFails('foo/bar.jpg', ':attribute contains a directory, give proper filename.'); + $this->assertFails('/etc/passwd', ':attribute contains a directory, give proper filename.'); + $this->assertFails('/file.jpg', ':attribute contains a directory, give proper filename.'); + $this->assertFails('a/b/c.jpg', ':attribute contains a directory, give proper filename.'); + } + + public function testPathTraversalAttempts(): void + { + $this->assertFails('.'); + $this->assertFails('../secret.jpg'); + $this->assertFails('test/Samples/../secret.jpg'); + $this->assertFails('../../etc/passwd'); + $this->assertFails('..\\..\\windows\\win.ini'); + $this->assertFails('foo\\bar.jpg'); + $this->assertFails('foo/../bar.jpg'); + } + + public function testDoubleDotWithoutSeparatorStillFails(): void + { + // ".." alone is not treated as a directory by pathinfo(), but it must + // still be rejected because it is caught by the invalid-characters check. + $this->assertFails('..'); + $this->assertFails('a..b.jpg'); + } + + public function testEmptyStringPasses(): void + { + // pathinfo('', PATHINFO_DIRNAME) === '' and it contains none of the + // forbidden substrings, so the rule itself does not flag it. Emptiness + // should be rejected by a separate `required` rule upstream. + $this->assertPasses(''); + } +}