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 /')];
}
/**