From 4962a3e5ff2c3eb37831a3964c65cb99a2b8d700 Mon Sep 17 00:00:00 2001 From: jphetphoumy Date: Mon, 30 Sep 2024 11:55:55 +0000 Subject: [PATCH] CWE-434: Unrestricted Upload of File with Dangerous Type. Polyglot file could lead to remote code execution #1468 --- src/Naming/OrignameNamer.php | 8 ++++++ src/Naming/Polyfill/FileExtensionTrait.php | 5 ---- src/Naming/SlugNamer.php | 14 +++++++-- src/Naming/SmartUniqueNamer.php | 8 ++++-- tests/Naming/Base64NamerTest.php | 3 -- tests/Naming/HashNamerTest.php | 3 -- tests/Naming/PropertyNamerTest.php | 13 ++++++--- tests/Naming/SlugNamerTest.php | 15 +++++++--- tests/Naming/SmartUniqidNamerTest.php | 33 +++++++++++++--------- tests/Naming/UniqidNamerTest.php | 12 ++++---- 10 files changed, 71 insertions(+), 43 deletions(-) diff --git a/src/Naming/OrignameNamer.php b/src/Naming/OrignameNamer.php index 565ed672..ac004bec 100644 --- a/src/Naming/OrignameNamer.php +++ b/src/Naming/OrignameNamer.php @@ -14,6 +14,8 @@ */ final class OrignameNamer implements NamerInterface, ConfigurableInterface { + use Polyfill\FileExtensionTrait; + private bool $transliterate = false; public function __construct(private readonly Transliterator $transliterator) @@ -39,6 +41,12 @@ public function name(object $object, PropertyMapping $mapping): string $name = $this->transliterator->transliterate($name); } + $extension = $this->getExtension($file); + + if (\is_string($extension) && '' !== $extension) { + $name = "$name.$extension"; + } + return \uniqid().'_'.$name; } } diff --git a/src/Naming/Polyfill/FileExtensionTrait.php b/src/Naming/Polyfill/FileExtensionTrait.php index d1fdc8a3..32f1db93 100644 --- a/src/Naming/Polyfill/FileExtensionTrait.php +++ b/src/Naming/Polyfill/FileExtensionTrait.php @@ -16,11 +16,6 @@ private function getExtension(File $file): ?string if (!$file instanceof UploadedFile && !$file instanceof ReplacingFile) { throw new \InvalidArgumentException('Unexpected type for $file: '.$file::class); } - $originalName = $file->getClientOriginalName(); - - if ('' !== ($extension = \pathinfo($originalName, \PATHINFO_EXTENSION))) { - return $extension; - } if ('' !== ($extension = $file->guessExtension())) { return $extension; diff --git a/src/Naming/SlugNamer.php b/src/Naming/SlugNamer.php index 5222f8f1..12d90618 100644 --- a/src/Naming/SlugNamer.php +++ b/src/Naming/SlugNamer.php @@ -12,6 +12,8 @@ */ final class SlugNamer implements NamerInterface { + use Polyfill\FileExtensionTrait; + public function __construct(private readonly Transliterator $transliterator, private readonly object $service, private readonly string $method) { } @@ -20,10 +22,12 @@ public function name(object $object, PropertyMapping $mapping): string { $file = $mapping->getFile($object); $originalName = $file->getClientOriginalName(); - $extension = \strtolower(\pathinfo($originalName, \PATHINFO_EXTENSION)); + $extension = $this->getExtension($file); $basename = \substr(\pathinfo($originalName, \PATHINFO_FILENAME), 0, 240); $basename = \strtolower($this->transliterator->transliterate($basename)); - $slug = \sprintf('%s.%s', $basename, $extension); + $slug = is_string($extension) && '' !== $extension + ? \sprintf('%s.%s', $basename, $extension) + : $basename; // check if there another object with same slug $num = 0; @@ -32,7 +36,11 @@ public function name(object $object, PropertyMapping $mapping): string if (null === $otherObject) { return $slug; } - $slug = \sprintf('%s-%d.%s', $basename, ++$num, $extension); + + $slug = \is_string($extension) && '' !== $extension + ? \sprintf('%s-%d.%s', $basename, ++$num, $extension) + : \sprintf('%s-%d', $basename, ++$num) + ; } } } diff --git a/src/Naming/SmartUniqueNamer.php b/src/Naming/SmartUniqueNamer.php index a6213d09..2d4a1097 100644 --- a/src/Naming/SmartUniqueNamer.php +++ b/src/Naming/SmartUniqueNamer.php @@ -13,6 +13,8 @@ */ final class SmartUniqueNamer implements NamerInterface { + use Polyfill\FileExtensionTrait; + public function __construct(private readonly Transliterator $transliterator) { } @@ -22,10 +24,12 @@ public function name(object $object, PropertyMapping $mapping): string $file = $mapping->getFile($object); $originalName = $file->getClientOriginalName(); $originalName = $this->transliterator->transliterate($originalName); - $originalExtension = \strtolower(\pathinfo($originalName, \PATHINFO_EXTENSION)); + $originalExtension = $this->getExtension($file); $originalBasename = \pathinfo($originalName, \PATHINFO_FILENAME); $uniqId = \str_replace('.', '', \uniqid('-', true)); - $uniqExtension = \sprintf('%s.%s', $uniqId, $originalExtension); + $uniqExtension = \is_string($originalExtension) && '' !== $originalExtension + ? \sprintf('%s.%s', $uniqId, $originalExtension) + : $uniqId; $smartName = \sprintf('%s%s', $originalBasename, $uniqExtension); // Check if smartName is an acceptable size (some filesystems accept a max of 255) diff --git a/tests/Naming/Base64NamerTest.php b/tests/Naming/Base64NamerTest.php index 9dae8dee..f0024306 100644 --- a/tests/Naming/Base64NamerTest.php +++ b/tests/Naming/Base64NamerTest.php @@ -34,9 +34,6 @@ public static function fileDataProvider(): array public function testNameReturnsTheRightName(string $expectedFileName, string $extension, ?int $length): void { $file = $this->getUploadedFileMock(); - $file->expects(self::once()) - ->method('getClientOriginalName') - ->willReturn('foo'); $file->expects(self::once()) ->method('guessExtension') diff --git a/tests/Naming/HashNamerTest.php b/tests/Naming/HashNamerTest.php index c724f693..3779a4ef 100644 --- a/tests/Naming/HashNamerTest.php +++ b/tests/Naming/HashNamerTest.php @@ -35,9 +35,6 @@ public static function fileDataProvider(): array public function testNameReturnsTheRightName(string $expectedFileName, string $extension, string $algorithm, ?int $length): void { $file = $this->getUploadedFileMock(); - $file->expects(self::once()) - ->method('getClientOriginalName') - ->willReturn('foo'); $file->expects(self::once()) ->method('guessExtension') diff --git a/tests/Naming/PropertyNamerTest.php b/tests/Naming/PropertyNamerTest.php index 3976336e..da8cccfa 100644 --- a/tests/Naming/PropertyNamerTest.php +++ b/tests/Naming/PropertyNamerTest.php @@ -23,10 +23,10 @@ public static function fileDataProvider(): array $weirdEntity->someProperty = 'Yéô'; return [ - 'with ext' => ['some-file-name.jpeg', 'foo.jpeg', $entity, 'someProperty', false], - 'without ext' => ['some-file-name', 'foo', $entity, 'someProperty', false], - 'method call' => ['some-file-name.jpeg', 'generated-file-name.jpeg', $entity, 'generateFileName', false], - 'translit.' => ['some-file-name.jpeg', 'yeo.jpeg', $weirdEntity, 'someProperty', true], + 'with ext' => ['some-file-name.jpeg', 'foo.jpg', 'jpg', $entity, 'someProperty', false], + 'without ext' => ['some-file-name', 'foo', null, $entity, 'someProperty', false], + 'method call' => ['some-file-name.jpeg', 'generated-file-name.jpg', 'jpg', $entity, 'generateFileName', false], + 'translit.' => ['some-file-name.jpeg', 'yeo.jpg', 'jpg', $weirdEntity, 'someProperty', true], ]; } @@ -36,6 +36,7 @@ public static function fileDataProvider(): array public function testNameReturnsTheRightName( string $originalFileName, string $expectedFileName, + ?string $guessedExtension, object $entity, string $propertyName, bool $transliterate @@ -45,6 +46,10 @@ public function testNameReturnsTheRightName( ->method('getClientOriginalName') ->willReturn($originalFileName); + $file + ->method('guessExtension') + ->willReturn($guessedExtension); + $mapping = $this->getPropertyMappingMock(); $mapping->expects(self::once()) ->method('getFile') diff --git a/tests/Naming/SlugNamerTest.php b/tests/Naming/SlugNamerTest.php index 815f426b..1964c8a1 100644 --- a/tests/Naming/SlugNamerTest.php +++ b/tests/Naming/SlugNamerTest.php @@ -11,16 +11,17 @@ final class SlugNamerTest extends TestCase public static function fileDataProvider(): array { return [ - // case -> original name, result pattern - 'non existing' => ['lala.jpeg', '/lala.jpeg/'], - 'existing' => ['làlà.mp3', '/lala-1.mp3/'], + // case -> original name, guessedExtension, result pattern + 'non existing' => ['lala.jpeg', 'jpg', '/lala.jpg/'], + 'guess extension null' => ['lala.jpeg', null, '/lala$/'], + 'existing' => ['làlà.mp3', 'mp3', '/lala-1.mp3/'], ]; } /** * @dataProvider fileDataProvider */ - public function testNameReturnsAnUniqueName(string $originalName, string $pattern): void + public function testNameReturnsAnUniqueName(string $originalName, ?string $guessedExtension, string $pattern): void { $file = $this->getUploadedFileMock(); $file @@ -29,6 +30,12 @@ public function testNameReturnsAnUniqueName(string $originalName, string $patter ->willReturn($originalName) ; + $file + ->expects(self::once()) + ->method('guessExtension') + ->willReturn($guessedExtension) + ; + $entity = new \stdClass(); $mapping = $this->getPropertyMappingMock(); diff --git a/tests/Naming/SmartUniqidNamerTest.php b/tests/Naming/SmartUniqidNamerTest.php index 9b8277ef..851ffc0a 100644 --- a/tests/Naming/SmartUniqidNamerTest.php +++ b/tests/Naming/SmartUniqidNamerTest.php @@ -10,26 +10,27 @@ final class SmartUniqidNamerTest extends TestCase public static function fileDataProvider(): array { return [ - // case -> original name, result pattern - 'typical' => ['lala.jpeg', '/lala-[[:xdigit:]]{22}\.jpeg/'], - 'accented' => ['làlà.mp3', '/lala-[[:xdigit:]]{22}\.mp3/'], - 'spaced' => ['a Foo Bar.txt', '/a-foo-bar-[[:xdigit:]]{22}\.txt/'], - 'special char' => ['yezz!.png', '/yezz-[[:xdigit:]]{22}\.png/'], - 'long basename' => [\str_repeat('a', 256).'.txt', '/a{228}-[[:xdigit:]]{22}\.txt/'], - 'long extension' => ['a.'.\str_repeat('a', 256), '/a-[[:xdigit:]]{22}\.a{230}/'], + // case -> original name, guessed extension, result pattern + 'typical' => ['lala.jpeg', 'jpg', '/lala-[[:xdigit:]]{22}\.jpg/'], + 'accented' => ['làlà.mp3', 'mp3', '/lala-[[:xdigit:]]{22}\.mp3/'], + 'spaced' => ['a Foo Bar.txt', 'txt', '/a-foo-bar-[[:xdigit:]]{22}\.txt/'], + 'special char' => ['yezz!.png', 'png', '/yezz-[[:xdigit:]]{22}\.png/'], + 'long basename' => [\str_repeat('a', 256).'.txt', 'txt', '/a{228}-[[:xdigit:]]{22}\.txt/'], + 'long extension' => ['a.'.\str_repeat('a', 256), null, '/a-[[:xdigit:]]{22}$/'], 'long basename and extension' => [\str_repeat('a', 256).'.txt'.\str_repeat('a', 256), - '/a{228}-[[:xdigit:]]{22}\.txt/', ], - 'double extension' => ['lala.png.jpg', '/lala-png-[[:xdigit:]]{22}\.jpg/'], - 'uppercase extension' => ['lala.JPEG', '/lala-[[:xdigit:]]{22}\.jpeg/'], - 'double uppercase extension' => ['lala.JPEG.JPEG', '/lala-jpeg-[[:xdigit:]]{22}\.jpeg/'], - 'dot in filename' => ['filename has . spaces (2).jpg', '/filename-has-spaces-2-[[:xdigit:]]{22}\.jpg/'], + 'txt', '/a{228}-[[:xdigit:]]{22}\.txt$/', ], + 'double extension' => ['lala.png.jpg', 'jpg', '/lala-png-[[:xdigit:]]{22}\.jpg/'], + 'uppercase extension' => ['lala.JPEG', 'jpg', '/lala-[[:xdigit:]]{22}\.jpg/'], + 'double uppercase extension' => ['lala.JPEG.JPEG', 'jpg', '/lala-jpeg-[[:xdigit:]]{22}\.jpg/'], + 'dot in filename' => ['filename has . spaces (2).jpg', 'jpg', '/filename-has-spaces-2-[[:xdigit:]]{22}\.jpg/'], + 'file with no extension with null mimetype' => ['lala', null, '/lala-[[:xdigit:]]{22}$/'], ]; } /** * @dataProvider fileDataProvider */ - public function testNameReturnsAnUniqueName(string $originalName, string $pattern): void + public function testNameReturnsAnUniqueName(string $originalName, ?string $guessExtension, string $pattern): void { $file = $this->getUploadedFileMock(); $file @@ -38,6 +39,12 @@ public function testNameReturnsAnUniqueName(string $originalName, string $patter ->willReturn($originalName) ; + $file + ->expects(self::once()) + ->method('guessExtension') + ->willReturn($guessExtension) + ; + $entity = new \stdClass(); $mapping = $this->getPropertyMappingMock(); diff --git a/tests/Naming/UniqidNamerTest.php b/tests/Naming/UniqidNamerTest.php index 01d1bb17..9c85c67c 100644 --- a/tests/Naming/UniqidNamerTest.php +++ b/tests/Naming/UniqidNamerTest.php @@ -16,13 +16,13 @@ public static function fileDataProvider(): array { return [ // original_name, guessed_extension, pattern - ['lala.jpeg', null, '/[a-z0-9]{13}.jpeg/'], - ['lala.mp3', 'mpga', '/[a-z0-9]{13}.mp3/'], + ['lala.jpeg', null, '/[a-z0-9]{13}/'], + ['lala.mp3', 'mp3', '/[a-z0-9]{13}.mp3/'], ['lala', 'mpga', '/[a-z0-9]{13}.mpga/'], - ['lala', null, '/[a-z0-9]{13}/'], - ['lala.0', null, '/[a-z0-9]{13}\\.0/'], - ['lala.data.0', null, '/[a-z0-9]{13}\\.0/'], - ['lala.data.0', 'gzip', '/[a-z0-9]{13}\\.0/'], + ['lala', null, '/[a-z0-9]{13}$/'], + ['lala.0', null, '/[a-z0-9]{13}$/'], + ['lala.data.0', null, '/[a-z0-9]{13}$/'], + ['lala.data.0', 'gzip', '/[a-z0-9]{13}.gzip/'], ]; }