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'); +});