From 2a011c99d8f18291e51e11a917a18336e700c47e Mon Sep 17 00:00:00 2001 From: Daniele Barbaro Date: Tue, 8 Sep 2026 15:46:02 +0200 Subject: [PATCH] feat(kmz): add archive limits and use temp_directory extractAllFiles() read an archive with no ceiling on entry count or uncompressed size, so a KMZ declaring a handful of entries that expand into gigabytes filled the disk before anything noticed. Entry names went unchecked too. Both are now validated before a single byte is read, and either limit can be turned off by setting it to 0. The mkdir() return value was ignored, so a destination that could not be created produced a warning and then a confusing failure further down. It now throws with the path that could not be made. temp_directory has been in the config since the first release without anything reading it. extractAllFiles() takes an optional destination and falls back to that key, then to the system temp directory, giving each call a directory of its own. extractAllFiles() also stops throwing bare KmlException for a missing or unreadable archive and uses KmzExtractorException like the rest of the class. That is a narrowing, KmzExtractorException extends KmlException. --- README.md | 16 +- config/kml-parser.php | 20 ++- src/Exceptions/KmzExtractorException.php | 20 +++ src/KmzExtractor.php | 187 +++++++++++++++++++---- tests/KmzHardeningTest.php | 147 ++++++++++++++++++ 5 files changed, 354 insertions(+), 36 deletions(-) create mode 100644 tests/KmzHardeningTest.php diff --git a/README.md b/README.md index ea9858c..8e84d91 100644 --- a/README.md +++ b/README.md @@ -261,7 +261,13 @@ $extractor = new KmzExtractor(); $files = $extractor->extractAllFiles('path/to/file.kmz', 'extraction/directory'); ``` -`extractAllFiles()` returns the list of entry names it wrote. It extracts whatever the archive contains, so point it at a directory you control and treat uploaded archives as untrusted input. +`extractAllFiles()` returns the list of entry names it wrote. Leave the destination out and it writes to a directory of its own under `temp_directory`, or under the system temp directory when that is null: + +```php +$files = $extractor->extractAllFiles('path/to/file.kmz'); +``` + +Archives are checked before anything is read out of them. An archive is rejected when it declares more than `max_archive_entries` entries, when its entries add up to more than `max_uncompressed_size` bytes uncompressed, or when any entry name is absolute or contains `..` and would therefore write outside the destination. Set either limit to `0` to turn it off. ## Error handling @@ -312,8 +318,14 @@ return [ 'http://earth.google.com/kml/2.0', ], - // Reserved for KMZ extraction. Not used yet. + // Where extractAllFiles() writes when given no destination. + // null means the system temp directory. 'temp_directory' => null, + + // Ceilings applied to a KMZ before anything is read out of it. + // Set either to 0 to disable it. + 'max_archive_entries' => 5000, + 'max_uncompressed_size' => 256 * 1024 * 1024, ]; ``` diff --git a/config/kml-parser.php b/config/kml-parser.php index 8ca3448..2177b85 100644 --- a/config/kml-parser.php +++ b/config/kml-parser.php @@ -36,9 +36,25 @@ | Temporary Directory |-------------------------------------------------------------------------- | - | This value determines the temporary directory used for extracting KMZ files. - | If null, the system temp directory will be used. + | Where KmzExtractor::extractAllFiles() writes when the caller names no + | destination. Each call gets its own directory underneath it. If null, + | the system temp directory is used. | */ 'temp_directory' => null, + + /* + |-------------------------------------------------------------------------- + | Archive Limits + |-------------------------------------------------------------------------- + | + | A KMZ is a ZIP, and a ZIP can declare a handful of entries that expand + | into far more than the machine has. An archive breaching either ceiling + | is rejected before anything is read out of it. A real KMZ is a KML plus + | its icons, nowhere near either number. Set one to 0 to disable it. + | + */ + 'max_archive_entries' => 5000, + + 'max_uncompressed_size' => 256 * 1024 * 1024, ]; diff --git a/src/Exceptions/KmzExtractorException.php b/src/Exceptions/KmzExtractorException.php index 39a3ab8..4db981a 100644 --- a/src/Exceptions/KmzExtractorException.php +++ b/src/Exceptions/KmzExtractorException.php @@ -23,4 +23,24 @@ public static function invalidZipFile(string $path): self { return new self("Invalid KMZ file: {$path}"); } + + public static function tooManyEntries(int $count, int $max): self + { + return new self("KMZ archive holds {$count} entries, more than the {$max} allowed"); + } + + public static function archiveTooLarge(int $max): self + { + return new self("KMZ archive expands to more than the {$max} bytes allowed"); + } + + public static function unsafeEntry(string $name): self + { + return new self("KMZ archive holds an entry that would escape the destination: {$name}"); + } + + public static function destinationNotWritable(string $path): self + { + return new self("Unable to create the extraction directory: {$path}"); + } } diff --git a/src/KmzExtractor.php b/src/KmzExtractor.php index ee84dcb..6431ebc 100644 --- a/src/KmzExtractor.php +++ b/src/KmzExtractor.php @@ -2,40 +2,45 @@ namespace PlinCode\KmlParser; -use PlinCode\KmlParser\Exceptions\KmlException; use PlinCode\KmlParser\Exceptions\KmzExtractorException; +use PlinCode\KmlParser\Traits\ReadsPackageConfig; use ZipArchive; class KmzExtractor { + use ReadsPackageConfig; + + /** + * Ceilings applied to an archive before anything is read out of it. + * + * A KMZ is a ZIP, and a ZIP can declare a handful of entries that expand + * into far more than the machine has. These are deliberately generous: a + * real KMZ is a KML plus its icons, nowhere near either limit. + */ + public const DEFAULT_MAX_ENTRIES = 5000; + + public const DEFAULT_MAX_UNCOMPRESSED_SIZE = 268435456; // 256 MB + /** * Extract KML content from a KMZ file + * + * @throws KmzExtractorException */ public function extractKmlContent(string $path): string { - if (! file_exists($path)) { - throw KmzExtractorException::fileNotFound($path); - } - - $zip = new ZipArchive; - if ($zip->open($path) !== true) { - throw KmzExtractorException::invalidZipFile($path); - } + $zip = $this->open($path); try { - $kmlFiles = array_filter( - array_map( - fn (int $i) => $zip->getNameIndex($i), - range(0, $zip->numFiles - 1) - ), - fn (string $filename) => pathinfo($filename, PATHINFO_EXTENSION) === 'kml' - ); - - if (empty($kmlFiles)) { + $this->guardArchive($zip); + + $kmlIndex = $this->firstKmlIndex($zip); + + if ($kmlIndex === null) { throw KmzExtractorException::noKmlFound(); } - $kmlContent = $zip->getFromIndex(array_key_first($kmlFiles)); + $kmlContent = $zip->getFromIndex($kmlIndex); + if ($kmlContent === false) { throw KmzExtractorException::failedToExtract('Failed to read KML file from archive'); } @@ -49,32 +54,150 @@ public function extractKmlContent(string $path): string /** * Extract all files from KMZ archive * - * @throws KmlException If the KMZ file cannot be found or opened + * Without a destination the files go to the configured temp_directory, or + * to the system temp directory, in a directory of their own. + * + * @return array the entry names that were written + * + * @throws KmzExtractorException */ - public function extractAllFiles(string $kmzPath, string $destination): array + public function extractAllFiles(string $kmzPath, ?string $destination = null): array { - if (! file_exists($kmzPath)) { - throw new KmlException("KMZ file not found: {$kmzPath}"); + $destination ??= $this->defaultDestination(); + + $zip = $this->open($kmzPath); + + try { + $this->guardArchive($zip); + + /* + * The warning mkdir() raises carries less than the exception + * thrown below, and an application turning warnings into + * exceptions would otherwise get that one instead of ours. The + * return value is what is acted on, the second is_dir() covers + * another process winning the race. + */ + if (! is_dir($destination) && ! @mkdir($destination, 0755, true) && ! is_dir($destination)) { + throw KmzExtractorException::destinationNotWritable($destination); + } + + if (! $zip->extractTo($destination)) { + throw KmzExtractorException::failedToExtract("Unable to extract the archive into {$destination}"); + } + + return $this->entryNames($zip); + } finally { + $zip->close(); + } + } + + /** + * The directory extractAllFiles() writes to when the caller names none. + */ + public function defaultDestination(): string + { + $configured = $this->packageConfig('kml-parser.temp_directory', null); + $base = is_string($configured) && $configured !== '' ? $configured : sys_get_temp_dir(); + + return rtrim($base, '/\\').DIRECTORY_SEPARATOR.'kml-parser-'.uniqid(); + } + + /** + * @throws KmzExtractorException + */ + protected function open(string $path): ZipArchive + { + if (! file_exists($path)) { + throw KmzExtractorException::fileNotFound($path); } $zip = new ZipArchive; - if ($zip->open($kmzPath) !== true) { - throw new KmlException("Unable to open KMZ file: {$kmzPath}"); + + if ($zip->open($path) !== true) { + throw KmzExtractorException::invalidZipFile($path); } - if (! file_exists($destination)) { - mkdir($destination, 0755, true); + return $zip; + } + + /** + * Reject an archive that is too large to trust before reading anything out + * of it, and reject any entry whose name would escape the destination. + * + * @throws KmzExtractorException + */ + protected function guardArchive(ZipArchive $zip): void + { + $maxEntries = (int) $this->packageConfig('kml-parser.max_archive_entries', self::DEFAULT_MAX_ENTRIES); + $maxSize = (int) $this->packageConfig('kml-parser.max_uncompressed_size', self::DEFAULT_MAX_UNCOMPRESSED_SIZE); + + if ($maxEntries > 0 && $zip->numFiles > $maxEntries) { + throw KmzExtractorException::tooManyEntries($zip->numFiles, $maxEntries); } - $zip->extractTo($destination); + $total = 0; - $extractedFiles = []; for ($i = 0; $i < $zip->numFiles; $i++) { - $extractedFiles[] = $zip->getNameIndex($i); + $stat = $zip->statIndex($i); + + if ($stat === false) { + throw KmzExtractorException::failedToExtract("Unable to read entry {$i} of the archive"); + } + + $this->guardEntryName((string) $stat['name']); + + $total += (int) $stat['size']; + + if ($maxSize > 0 && $total > $maxSize) { + throw KmzExtractorException::archiveTooLarge($maxSize); + } } + } - $zip->close(); + /** + * @throws KmzExtractorException + */ + protected function guardEntryName(string $name): void + { + if (str_starts_with($name, '/') || str_starts_with($name, '\\') || preg_match('#^[A-Za-z]:[\\\\/]#', $name) === 1) { + throw KmzExtractorException::unsafeEntry($name); + } + + foreach (preg_split('#[\\\\/]#', $name) ?: [] as $segment) { + if ($segment === '..') { + throw KmzExtractorException::unsafeEntry($name); + } + } + } + + protected function firstKmlIndex(ZipArchive $zip): ?int + { + for ($i = 0; $i < $zip->numFiles; $i++) { + $name = $zip->getNameIndex($i); + + if ($name !== false && strtolower(pathinfo($name, PATHINFO_EXTENSION)) === 'kml') { + return $i; + } + } + + return null; + } + + /** + * @return array + */ + protected function entryNames(ZipArchive $zip): array + { + $names = []; + + for ($i = 0; $i < $zip->numFiles; $i++) { + $name = $zip->getNameIndex($i); + + if ($name !== false) { + $names[] = $name; + } + } - return $extractedFiles; + return $names; } } diff --git a/tests/KmzHardeningTest.php b/tests/KmzHardeningTest.php new file mode 100644 index 0000000..cb011f6 --- /dev/null +++ b/tests/KmzHardeningTest.php @@ -0,0 +1,147 @@ +open($path, ZipArchive::CREATE); + $build($zip); + $zip->close(); + + return $path; +} + +function validKml(): string +{ + return <<<'XML' + + + + 7.7,45.8,0 + + +XML; +} + +function removeRecursively(string $path): void +{ + if (is_dir($path)) { + foreach (glob($path.'/*') ?: [] as $child) { + removeRecursively($child); + } + + @rmdir($path); + + return; + } + + @unlink($path); +} + +afterEach(function () { + foreach (glob(sys_get_temp_dir().'/kml-parser-test-*') ?: [] as $path) { + removeRecursively($path); + } +}); + +it('rejects an archive with more entries than allowed', function () { + config()->set('kml-parser.max_archive_entries', 3); + + $path = makeArchive(function (ZipArchive $zip) { + $zip->addFromString('doc.kml', validKml()); + foreach (range(1, 5) as $i) { + $zip->addFromString("images/icon-{$i}.png", 'x'); + } + }); + + expect(fn () => (new KmzExtractor)->extractKmlContent($path)) + ->toThrow(KmzExtractorException::class, 'more than the 3 allowed'); +}); + +it('rejects an archive that expands beyond the size limit', function () { + config()->set('kml-parser.max_uncompressed_size', 1024); + + $path = makeArchive(function (ZipArchive $zip) { + $zip->addFromString('doc.kml', validKml()); + $zip->addFromString('big.bin', str_repeat('a', 4096)); + }); + + expect(fn () => (new KmzExtractor)->extractKmlContent($path)) + ->toThrow(KmzExtractorException::class, 'expands to more than the 1024 bytes allowed'); +}); + +it('rejects an entry that would escape the destination', function (string $name) { + $path = makeArchive(function (ZipArchive $zip) use ($name) { + $zip->addFromString('doc.kml', validKml()); + $zip->addFromString($name, 'x'); + }); + + expect(fn () => (new KmzExtractor)->extractAllFiles($path, tempPath('-dir'))) + ->toThrow(KmzExtractorException::class, 'would escape the destination'); +})->with([ + '../escaped.txt', + 'images/../../escaped.txt', + '/etc/passwd', +]); + +it('accepts an ordinary archive', function () { + $path = makeArchive(function (ZipArchive $zip) { + $zip->addFromString('doc.kml', validKml()); + $zip->addFromString('images/icon.png', 'x'); + }); + + expect((new KmzExtractor)->extractKmlContent($path))->toContain('set('kml-parser.max_archive_entries', 0); + config()->set('kml-parser.max_uncompressed_size', 0); + + $path = makeArchive(function (ZipArchive $zip) { + $zip->addFromString('doc.kml', validKml()); + $zip->addFromString('big.bin', str_repeat('a', 4096)); + }); + + expect((new KmzExtractor)->extractKmlContent($path))->toContain('set('kml-parser.temp_directory', $base); + + $path = makeArchive(function (ZipArchive $zip) { + $zip->addFromString('doc.kml', validKml()); + }); + + $extractor = new KmzExtractor; + $files = $extractor->extractAllFiles($path); + + expect($files)->toBe(['doc.kml']) + ->and(glob($base.'/kml-parser-*/doc.kml'))->toHaveCount(1); +}); + +it('falls back to the system temp directory when none is configured', function () { + config()->set('kml-parser.temp_directory', null); + + expect((new KmzExtractor)->defaultDestination())->toStartWith(sys_get_temp_dir().DIRECTORY_SEPARATOR.'kml-parser-'); +}); + +it('reports a destination it cannot create', function () { + $path = makeArchive(fn (ZipArchive $zip) => $zip->addFromString('doc.kml', validKml())); + + $blocker = tempPath('-blocker'); + file_put_contents($blocker, 'not a directory'); + + expect(fn () => (new KmzExtractor)->extractAllFiles($path, $blocker.'/inside')) + ->toThrow(KmzExtractorException::class, 'Unable to create the extraction directory'); +});