diff --git a/CHANGELOG-6.x.md b/CHANGELOG-6.x.md index 794570d..862b1ed 100644 --- a/CHANGELOG-6.x.md +++ b/CHANGELOG-6.x.md @@ -2,6 +2,9 @@ ----- * Fix usage with AssetMapper +* Fix a shell command injection vulnerability in `Wrapper` (image format/paths were not escaped before being passed to the shell) +* Fix a shell command injection vulnerability in `leapt:im:clear` (cache directory path was not escaped before being passed to the shell) +* Fix a regex injection in `ImExtension::convert()` that could break image resizing for `src` attributes containing regex-special characters 6.0.0 ----- diff --git a/src/Command/ClearCommand.php b/src/Command/ClearCommand.php index d9381f0..cc8d6fe 100644 --- a/src/Command/ClearCommand.php +++ b/src/Command/ClearCommand.php @@ -44,7 +44,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int } // removing empty directories - $process = Process::fromShellCommandline('find ' . $cacheDir . ' -type d -empty'); + $process = Process::fromShellCommandline('find ' . escapeshellarg($cacheDir) . ' -type d -empty'); $process->run(); $emptyDirectories = explode("\n", $process->getOutput()); foreach ($emptyDirectories as $directory) { diff --git a/src/Twig/Extension/ImExtension.php b/src/Twig/Extension/ImExtension.php index c20d87e..50eed9f 100644 --- a/src/Twig/Extension/ImExtension.php +++ b/src/Twig/Extension/ImExtension.php @@ -55,7 +55,7 @@ public function convert(string $html): string if (!empty($width) || !empty($height)) { $format = $width . 'x' . $height; - $updatedTagString = preg_replace("| src=[\"']" . $src . "[\"']|", ' src="' . $this->imResize($src, $format) . '"', $img); + $updatedTagString = preg_replace('| src=["\']' . preg_quote($src, '|') . '["\']|', ' src="' . $this->imResize($src, $format) . '"', $img); \assert(null !== $updatedTagString); $html = str_replace($img, $updatedTagString, $html); } diff --git a/src/Wrapper.php b/src/Wrapper.php index df99c1b..0e6d6d7 100644 --- a/src/Wrapper.php +++ b/src/Wrapper.php @@ -83,9 +83,9 @@ private function buildCommand(string $command, string $inputFile, array $attribu if ('' !== $outputFile) { $this->checkDirectory($outputFile); - $commandString = $this->binaryPath . $command . ' ' . $inputFile . $attributesString . ' ' . $outputFile; + $commandString = $this->binaryPath . $command . ' ' . escapeshellarg($inputFile) . $attributesString . ' ' . escapeshellarg($outputFile); } else { - $commandString = $this->binaryPath . $command . $attributesString . ' ' . $inputFile; + $commandString = $this->binaryPath . $command . $attributesString . ' ' . escapeshellarg($inputFile); } $this->validateCommand($commandString); @@ -108,7 +108,7 @@ private function prepareAttributes(array $attributes = []): string } else { $result .= ' -' . $key; if ('' !== $value) { - $result .= ' "' . $value . '"'; + $result .= ' ' . escapeshellarg($value); } } } diff --git a/tests/Command/ClearCommandTest.php b/tests/Command/ClearCommandTest.php new file mode 100644 index 0000000..dbce9ee --- /dev/null +++ b/tests/Command/ClearCommandTest.php @@ -0,0 +1,68 @@ +filesystem = new Filesystem(); + // the space and single quote purposely make sure the "find" shell command used + // internally to prune empty directories properly escapes the cache path + $this->tmpDir = sys_get_temp_dir() . '/leapt-im-bundle-' . uniqid() . " o'clock"; + $this->filesystem->mkdir($this->tmpDir); + } + + protected function tearDown(): void + { + $this->filesystem->remove($this->tmpDir); + } + + public function testClearAllCache(): void + { + $cacheDir = $this->tmpDir . '/public/cache/im'; + $this->filesystem->dumpFile($cacheDir . '/100x100/img.jpg', 'content'); + + $tester = new CommandTester(new ClearCommand($this->createManager())); + $tester->execute([]); + + $this->assertSame(0, $tester->getStatusCode()); + $this->assertFalse($this->filesystem->exists($cacheDir)); + } + + public function testClearCacheOlderThanAgePrunesEmptyDirectories(): void + { + $cacheDir = $this->tmpDir . '/public/cache/im'; + $oldFile = $cacheDir . '/100x100/old.jpg'; + $recentFile = $cacheDir . '/200x200/recent.jpg'; + + $this->filesystem->dumpFile($oldFile, 'content'); + touch($oldFile, strtotime('-10 days')); + $this->filesystem->dumpFile($recentFile, 'content'); + + $tester = new CommandTester(new ClearCommand($this->createManager())); + $tester->execute(['age' => 5]); + + $this->assertSame(0, $tester->getStatusCode()); + $this->assertFalse($this->filesystem->exists($cacheDir . '/100x100'), 'the directory left empty after clearing should be pruned'); + $this->assertTrue($this->filesystem->exists($recentFile), 'files younger than the given age must be kept'); + } + + private function createManager(): Manager + { + return new Manager(new Wrapper(Process::class), $this->tmpDir, 'public', 'cache/im'); + } +} diff --git a/tests/Twig/Extension/ImExtensionTest.php b/tests/Twig/Extension/ImExtensionTest.php index 3c1b131..c03e1a8 100644 --- a/tests/Twig/Extension/ImExtensionTest.php +++ b/tests/Twig/Extension/ImExtensionTest.php @@ -44,6 +44,9 @@ public static function providerConvert(): iterable ['hop hop hip', 'hop hop hip'], ['hop hop hip', 'hop hop hip'], ['hop hop hip', 'hop hop hip'], + // src containing regex-special characters must not break the internal preg_replace + ['hop hop', 'hop hop'], + ['hop hop', 'hop hop'], ]; } diff --git a/tests/WrapperTest.php b/tests/WrapperTest.php index 8bbbc09..40f391a 100644 --- a/tests/WrapperTest.php +++ b/tests/WrapperTest.php @@ -50,14 +50,20 @@ public static function providerPrepareAttributes(): iterable [ 'resize' => '150x150^', ], - ' -resize "150x150^"', + " -resize '150x150^'", ]; yield 'array_config' => [ [ 'resize' => '120x', '' => '+opaque -transparent', ], - ' -resize "120x" +opaque -transparent', + " -resize '120x' +opaque -transparent", + ]; + yield 'shell_metacharacters_are_escaped' => [ + [ + 'thumbnail' => '100x100" ; touch /tmp/pwned ; echo "', + ], + " -thumbnail '100x100\" ; touch /tmp/pwned ; echo \"'", ]; } @@ -95,9 +101,10 @@ public function testBuildCommand(string $command, string $inputFile, array $attr */ public static function providerBuildCommand(): iterable { - yield ['convert', 'somefile', [], 'anotherfile', 'convert somefile anotherfile']; - yield ['mogrify', 'somefile', ['resize' => '450x'], '', 'mogrify -resize "450x" somefile']; - yield ['montage', 'somefile', ['resize' => '450x'], '', 'montage -resize "450x" somefile']; + yield ['convert', 'somefile', [], 'anotherfile', "convert 'somefile' 'anotherfile'"]; + yield ['mogrify', 'somefile', ['resize' => '450x'], '', "mogrify -resize '450x' 'somefile'"]; + yield ['montage', 'somefile', ['resize' => '450x'], '', "montage -resize '450x' 'somefile'"]; + yield ['convert', 'some"file; rm -rf /', [], '', 'convert ' . escapeshellarg('some"file; rm -rf /')]; } /**