Skip to content

Commit

Permalink
CWE-434: Unrestricted Upload of File with Dangerous Type. Polyglot fi…
Browse files Browse the repository at this point in the history
…le could lead to remote code execution #1468
  • Loading branch information
jphetphoumy committed Oct 12, 2024
1 parent 8056cd5 commit 4962a3e
Show file tree
Hide file tree
Showing 10 changed files with 71 additions and 43 deletions.
8 changes: 8 additions & 0 deletions src/Naming/OrignameNamer.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
*/
final class OrignameNamer implements NamerInterface, ConfigurableInterface
{
use Polyfill\FileExtensionTrait;

private bool $transliterate = false;

public function __construct(private readonly Transliterator $transliterator)
Expand All @@ -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;
}
}
5 changes: 0 additions & 5 deletions src/Naming/Polyfill/FileExtensionTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
14 changes: 11 additions & 3 deletions src/Naming/SlugNamer.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
}
Expand All @@ -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;
Expand All @@ -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)
;
}
}
}
8 changes: 6 additions & 2 deletions src/Naming/SmartUniqueNamer.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
*/
final class SmartUniqueNamer implements NamerInterface
{
use Polyfill\FileExtensionTrait;

public function __construct(private readonly Transliterator $transliterator)
{
}
Expand All @@ -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)
Expand Down
3 changes: 0 additions & 3 deletions tests/Naming/Base64NamerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
3 changes: 0 additions & 3 deletions tests/Naming/HashNamerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
13 changes: 9 additions & 4 deletions tests/Naming/PropertyNamerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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],
];
}

Expand All @@ -36,6 +36,7 @@ public static function fileDataProvider(): array
public function testNameReturnsTheRightName(
string $originalFileName,
string $expectedFileName,
?string $guessedExtension,
object $entity,
string $propertyName,
bool $transliterate
Expand All @@ -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')
Expand Down
15 changes: 11 additions & 4 deletions tests/Naming/SlugNamerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
Expand Down
33 changes: 20 additions & 13 deletions tests/Naming/SmartUniqidNamerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
Expand Down
12 changes: 6 additions & 6 deletions tests/Naming/UniqidNamerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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/'],
];
}

Expand Down

0 comments on commit 4962a3e

Please sign in to comment.