CWE-434: Unrestricted Upload of File with Dangerous Type. Polyglot file could lead to remote code execution

[jphetphoumy]

Q	A
Bundle version	2.3.1
Symfony version	7.0.5
PHP version	8.3.12

Support Question

I was recently testing our application for CWE 434 - Unrestricted Upload of File with Dangerous Type and found out that a user could upload a php file by appending PHP code to the end of a PNG file when using VichUploader

A user could easily bypass the MIME_TYPE check and upload a polyglot file ( a mixed of php/png file ), this could lead to remote code execution on the server and allow a malicious user to run code on the server.

While browsing the issues of VichUploaderBundler to check if similar issue was reported I found this ticket : #1129

The issue talk about updating nginx configuration to deny the execution of a PHP file. While I agree that this could fix the problem, I think using guessExtension instead of getExtension inside of the namer would help to prevent a malicious user bypassing the form upload.

Namer:

https://github.com/dustin10/VichUploaderBundle/blob/master/src/Naming/Base64Namer.php#L44 https://github.com/dustin10/VichUploaderBundle/blob/master/src/Naming/HashNamer.php#L44 https://github.com/dustin10/VichUploaderBundle/blob/master/src/Naming/SlugNamer.php#L23 https://github.com/dustin10/VichUploaderBundle/blob/master/src/Naming/UniqidNamer.php#L20

The Symfony documentation on file upload says the following:

A well-known security best practice is to never trust the input provided by users. This also applies to the files uploaded by your visitors. The UploadedFile class provides methods to get the original file extension (getClientOriginalExtension()), the original file size (getSize()), the original file name (getClientOriginalName()) and the original file path (getClientOriginalPath()). However, they are considered not safe because a malicious user could tamper that information. That's why it's always better to generate a unique name and use the guessExtension() method to let Symfony guess the right extension according to the file MIME type;

I think using guessExtension instead of getExtension would help prevent security issue

Reference:

CWE 434: https://cwe.mitre.org/data/definitions/434.html Polyglot file: https://book.hacktricks.xyz/pentesting-web/file-upload#polyglot-files Symfony File Upload: https://symfony.com/doc/current/controller/upload_file.html

[garak]

It seems we can easily apply the required change, the following should be enough:

index d1fdc8a..64a292a 100644
--- a/src/Naming/Polyfill/FileExtensionTrait.php
+++ b/src/Naming/Polyfill/FileExtensionTrait.php
@@ -18,10 +18,6 @@ trait FileExtensionTrait
         }
         $originalName = $file->getClientOriginalName();

-        if ('' !== ($extension = \pathinfo($originalName, \PATHINFO_EXTENSION))) {
-            return $extension;
-        }
-
         if ('' !== ($extension = $file->guessExtension())) {
             return $extension;
         }

[jphetphoumy]

@garak Removing the part you mention fix the problem for most of the namer, Some namer like SlugNamer would need to use the Polyfill to avoid uploading PNG file as PHP file :

https://github.com/dustin10/VichUploaderBundle/blob/8056cd5561507ccf599500449cb95970ffb41ca7/src/Naming/SlugNamer.php#L20-L26

diff --git a/src/Naming/SlugNamer.php b/src/Naming/SlugNamer.php
index 5222f8f..0f006ee 100644
--- a/src/Naming/SlugNamer.php
+++ b/src/Naming/SlugNamer.php
@@ -12,6 +12,8 @@ use Vich\UploaderBundle\Util\Transliterator;
  */
 final class SlugNamer implements NamerInterface
 {
+    use Polyfill\FileExtensionTrait;
+
     public function __construct(private readonly Transliterator $transliterator, private readonly object $service, private readonly string $method)
     {
     }
@@ -20,7 +22,7 @@ final class SlugNamer implements NamerInterface
     {
         $file = $mapping->getFile($object);
         $originalName = $file->getClientOriginalName();
-        $extension = \strtolower(\pathinfo($originalName, \PATHINFO_EXTENSION));
+        $extension = $this->getExtension($file);

If this looks good to you I'll make a Pull Request

[garak]

Please go on