diff --git a/CHANGELOG.md b/CHANGELOG.md index ef6592d..b6dea48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,9 @@ Latest ------ +### Changes +* [#55](https://github.com/cleverage/archive-process-bundle/issues/55) `UnzipTask` (`archive_unzip`): new `max_files` and `max_size` options (default `null`, no limit) to protect the server against "zip bombs": the number of entries and their total uncompressed size are checked before anything is extracted, the task fails with a `RuntimeException` when a limit is exceeded. Update documentation, add tests. + v2.3 ------ diff --git a/docs/reference/tasks/unzip_task.md b/docs/reference/tasks/unzip_task.md index 97b36ed..7e5b763 100644 --- a/docs/reference/tasks/unzip_task.md +++ b/docs/reference/tasks/unzip_task.md @@ -14,7 +14,7 @@ Accepted inputs --------------- `array` or `null`: the input array is merged with the task options, input values taking precedence over the -configured ones. It may only contain the `filename` and/or `destination` keys (any other key throws an +configured ones. It may only contain option keys (`filename`, `destination`, `max_files`, `max_size`; any other key throws an `UndefinedOptionsException`). A `null` input is handled as an empty array, so the options come from the task configuration only. @@ -35,8 +35,10 @@ Options |---------------|----------|:--------:|---------|---------------------------------------------------------------------------------------------------| | `filename` | `string` | **X** | | Path of the zip archive to extract. It must exist and be readable (`\UnexpectedValueException`) | | `destination` | `string` | **X** | | Directory where the archive content is extracted | +| `max_files` | `?int` | | `null` | Maximum number of entries of the archive (`null`: no limit) | +| `max_size` | `?int` | | `null` | Maximum total uncompressed size of the entries, in bytes (`null`: no limit) | -Both options are required, but they can be provided either in the task configuration or in the input. +`filename` and `destination` are required, but they can be provided either in the task configuration or in the input. They are validated when the task is executed, not at process initialization. Examples @@ -84,3 +86,7 @@ Notes * Options are resolved on each execution of the task: when the task receives several inputs (e.g. after an iterable task), each input is extracted with its own `filename` and `destination`. * See the [Import the CSV files of an uploaded archive](../../cookbooks/import_archive.md) cookbook. +* With an archive that is not trusted (received from a partner, uploaded by a user...), set `max_files` and `max_size` + to protect the server against "zip bombs": they are checked from the central directory of the archive before + anything is extracted, and the task fails with a `RuntimeException` when a limit is exceeded. An extraction failing + for another reason (e.g. a write error) may leave the entries already extracted in the destination directory. diff --git a/src/Task/UnzipTask.php b/src/Task/UnzipTask.php index f8872b2..036ed2d 100644 --- a/src/Task/UnzipTask.php +++ b/src/Task/UnzipTask.php @@ -35,7 +35,7 @@ public function execute(ProcessState $state): void throw new UnexpectedValueException(\sprintf('UnzipTask expects an array or null input, %s given', get_debug_type($state->getInput()))); } /** - * @var array{filename: string, destination: string} $options + * @var array{filename: string, destination: string, max_files: ?int, max_size: ?int} $options */ $options = $this->getOptions($state); $filename = $options['filename']; @@ -56,6 +56,14 @@ public function execute(ProcessState $state): void throw new RuntimeException("Unable to open file {$filename} with code {$res}"); } + try { + $this->checkLimits($zipArchive, $filename, $options['max_files'], $options['max_size']); + } catch (RuntimeException $e) { + $zipArchive->close(); + + throw $e; + } + // The PHP warning is replaced by the exception below error_clear_last(); $extracted = @$zipArchive->extractTo($dest); @@ -68,15 +76,46 @@ public function execute(ProcessState $state): void $state->setOutput($dest); } + /** + * Checks the archive against the limits before extracting anything (protection against zip bombs): the number of + * entries and their uncompressed sizes are read from the central directory of the archive. + */ + protected function checkLimits(\ZipArchive $zipArchive, string $filename, ?int $maxFiles, ?int $maxSize): void + { + if (null !== $maxFiles && $zipArchive->numFiles > $maxFiles) { + throw new RuntimeException("Unable to extract file {$filename}: it contains {$zipArchive->numFiles} entries, more than the max_files option ({$maxFiles})"); + } + if (null === $maxSize) { + return; + } + $size = 0; + for ($index = 0; $index < $zipArchive->numFiles; ++$index) { + $stat = $zipArchive->statIndex($index); + if (false === $stat) { + throw new RuntimeException("Unable to extract file {$filename}: the entry {$index} cannot be read"); + } + $size += $stat['size']; + if ($size > $maxSize) { + throw new RuntimeException("Unable to extract file {$filename}: its uncompressed size is more than the max_size option ({$maxSize} bytes)"); + } + } + } + protected function configureOptions(OptionsResolver $resolver): void { $resolver->setRequired(['filename', 'destination']); $resolver->setAllowedTypes('filename', ['string']); $resolver->setAllowedTypes('destination', ['string']); + $resolver->setDefaults([ + 'max_files' => null, + 'max_size' => null, + ]); + $resolver->setAllowedTypes('max_files', ['null', 'int']); + $resolver->setAllowedTypes('max_size', ['null', 'int']); } /** - * @return array{filename: string, destination: string}|null + * @return array{filename: string, destination: string, max_files: ?int, max_size: ?int}|null */ #[\Override] protected function getOptions(ProcessState $state): ?array diff --git a/tests/Task/UnzipTaskTest.php b/tests/Task/UnzipTaskTest.php index 0048948..0adcf33 100644 --- a/tests/Task/UnzipTaskTest.php +++ b/tests/Task/UnzipTaskTest.php @@ -13,6 +13,7 @@ namespace CleverAge\ArchiveProcessBundle\Tests\Task; +use CleverAge\ArchiveProcessBundle\Exception\RuntimeException; use CleverAge\ArchiveProcessBundle\Task\UnzipTask; use CleverAge\ProcessBundle\Configuration\ProcessConfiguration; use CleverAge\ProcessBundle\Configuration\TaskConfiguration; @@ -233,6 +234,54 @@ private function execute(UnzipTask $task, ProcessState $state, mixed $input): mi return $state->getOutput(); } + public function testMaxFilesIsChecked(): void + { + [$task, $state] = $this->createTask([ + 'filename' => $this->dir.'/archive1.zip', + 'destination' => $this->dir.'/destination', + 'max_files' => 0, + ]); + + try { + $this->execute($task, $state, null); + self::fail('The extraction should have failed'); + } catch (RuntimeException $e) { + self::assertSame("Unable to extract file {$this->dir}/archive1.zip: it contains 1 entries, more than the max_files option (0)", $e->getMessage()); + } + self::assertDirectoryDoesNotExist($this->dir.'/destination'); + } + + public function testMaxSizeIsChecked(): void + { + // "content of file1.txt": 20 bytes + [$task, $state] = $this->createTask([ + 'filename' => $this->dir.'/archive1.zip', + 'destination' => $this->dir.'/destination', + 'max_size' => 19, + ]); + + try { + $this->execute($task, $state, null); + self::fail('The extraction should have failed'); + } catch (RuntimeException $e) { + self::assertSame("Unable to extract file {$this->dir}/archive1.zip: its uncompressed size is more than the max_size option (19 bytes)", $e->getMessage()); + } + self::assertDirectoryDoesNotExist($this->dir.'/destination'); + } + + public function testArchiveWithinTheLimitsIsExtracted(): void + { + [$task, $state] = $this->createTask([ + 'filename' => $this->dir.'/archive1.zip', + 'destination' => $this->dir.'/destination', + 'max_files' => 1, + 'max_size' => 20, + ]); + + self::assertSame($this->dir.'/destination', $this->execute($task, $state, null)); + self::assertFileExists($this->dir.'/destination/file1.txt'); + } + private function createArchive(string $filename, string $entry): void { $zip = new \ZipArchive();