Fix shell command injection in Wrapper and ClearCommand, regex injection in ImExtension

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
 -----

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) {

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

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

tests/Command/ClearCommandTest.php

@@ -0,0 +1,68 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Leapt\ImBundle\Tests\Command;
+
+use Leapt\ImBundle\Command\ClearCommand;
+use Leapt\ImBundle\Manager;
+use Leapt\ImBundle\Tests\Mock\Process;
+use Leapt\ImBundle\Wrapper;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\Console\Tester\CommandTester;
+use Symfony\Component\Filesystem\Filesystem;
+
+final class ClearCommandTest extends TestCase
+{
+    private Filesystem $filesystem;
+    private string $tmpDir;
+
+    protected function setUp(): void
+    {
+        $this->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');
+    }
+}

tests/Twig/Extension/ImExtensionTest.php

@@ -44,6 +44,9 @@ public static function providerConvert(): iterable
             ['hop <img src="/img.jpg" width="100" />hop <img src="/img2.jpg" width="100" /> hip', 'hop <img src="/cache/im/100x/img.jpg" width="100" />hop <img src="/cache/im/100x/img2.jpg" width="100" /> hip'],
             ['hop <img src="/img.jpg" width="100" />hop <img src="/img.jpg" width="120" /> hip', 'hop <img src="/cache/im/100x/img.jpg" width="100" />hop <img src="/cache/im/120x/img.jpg" width="120" /> hip'],
             ['hop <img src="/img.jpg" width="100" />hop <img src="/img.jpg" width="100" /> hip', 'hop <img src="/cache/im/100x/img.jpg" width="100" />hop <img src="/cache/im/100x/img.jpg" width="100" /> hip'],
+            // src containing regex-special characters must not break the internal preg_replace
+            ['hop <img src="/img(1.jpg" width="100" />hop', 'hop <img src="/cache/im/100x/img(1.jpg" width="100" />hop'],
+            ['hop <img src="/path/img[a].jpg" width="100" />hop', 'hop <img src="/cache/im/100x/path/img[a].jpg" width="100" />hop'],
         ];
     }
 

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