bug #12 [Security] XSS - SVG file upload vulnerability fixed (Rafikooo)

This PR was merged into the 1.9 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.9 <!-- see the comment below -->
| Bug fix?        | yes
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no <!-- don't forget to update the UPGRADE-*.md file -->
| Related tickets | GHSA-4qrp-27r3-66fj
| License         | MIT

There is a possibility to upload an SVG file containing XSS code in the admin panel. In order to perform an XSS attack, the file itself has to be open in a new card (or loaded outside of the IMG tag). The problem applies both to the files opened on the admin panel and shop pages.

<!--
 - Bug fixes must be submitted against the 1.10 or 1.11 branch(the lowest possible)
 - Features and deprecations must be submitted against the master branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->


Commits
-------

46ed54b [Security] XSS - SVG file upload vulnerability fixed

composer.json

@@ -35,6 +35,7 @@
         "doctrine/migrations": "^3.0",
         "doctrine/orm": "^2.7",
         "egulias/email-validator": "^2.1",
+        "enshrined/svg-sanitize": "^0.15.4",
         "fakerphp/faker": "^1.9",
         "friendsofphp/proxy-manager-lts": "^1.0",
         "friendsofsymfony/oauth-server-bundle": ">2.0.0-alpha.0 ^2.0@dev",

src/Sylius/Bundle/ApiBundle/composer.json

@@ -26,6 +26,7 @@
         "php": "^7.3",
         "api-platform/core": "^2.5",
         "doctrine/dbal": "^2.7",
+        "enshrined/svg-sanitize": "^0.15.4",
         "lexik/jwt-authentication-bundle": "^2.6",
         "sylius/core-bundle": "^1.7",
         "symfony/messenger": "^4.4 || ^5.2"

src/Sylius/Component/Core/Uploader/ImageUploader.php

@@ -13,6 +13,7 @@
 
 namespace Sylius\Component\Core\Uploader;
 
+use enshrined\svgSanitize\Sanitizer;
 use Gaufrette\Filesystem;
 use Sylius\Component\Core\Generator\ImagePathGeneratorInterface;
 use Sylius\Component\Core\Generator\UploadedImagePathGenerator;
@@ -22,12 +23,18 @@
 
 class ImageUploader implements ImageUploaderInterface
 {
+    private const MIME_SVG_XML = 'image/svg+xml';
+    private const MIME_SVG = 'image/svg';
+
     /** @var Filesystem */
     protected $filesystem;
 
     /** @var ImagePathGeneratorInterface */
     protected $imagePathGenerator;
 
+    /** @var Sanitizer */
+    protected $sanitizer;
+
     public function __construct(
         Filesystem $filesystem,
         ?ImagePathGeneratorInterface $imagePathGenerator = null
@@ -41,6 +48,7 @@ public function __construct(
         }
 
         $this->imagePathGenerator = $imagePathGenerator ?? new UploadedImagePathGenerator();
+        $this->sanitizer = new Sanitizer();
     }
 
     public function upload(ImageInterface $image): void
@@ -49,11 +57,13 @@ public function upload(ImageInterface $image): void
             return;
         }
 
+        /** @var File $file */
         $file = $image->getFile();
 
-        /** @var File $file */
         Assert::isInstanceOf($file, File::class);
 
+        $fileContent = $this->sanitizeContent(file_get_contents($file->getPathname()), $file->getMimeType());
+
         if (null !== $image->getPath() && $this->has($image->getPath())) {
             $this->remove($image->getPath());
         }
@@ -64,10 +74,7 @@ public function upload(ImageInterface $image): void
 
         $image->setPath($path);
 
-        $this->filesystem->write(
-            $image->getPath(),
-            file_get_contents($image->getFile()->getPathname())
-        );
+        $this->filesystem->write($image->getPath(), $fileContent);
     }
 
     public function remove(string $path): bool
@@ -79,6 +86,15 @@ public function remove(string $path): bool
         return false;
     }
 
+    protected function sanitizeContent(string $fileContent, string $mimeType): string
+    {
+        if (self::MIME_SVG_XML === $mimeType || self::MIME_SVG === $mimeType) {
+            $fileContent = $this->sanitizer->sanitize($fileContent);
+        }
+
+        return $fileContent;
+    }
+
     private function has(string $path): bool
     {
         return $this->filesystem->has($path);

src/Sylius/Component/Core/composer.json

@@ -27,6 +27,7 @@
     ],
     "require": {
         "php": "^7.3",
+        "enshrined/svg-sanitize": "^0.15.4",
         "knplabs/gaufrette": "^0.8",
         "payum/payum": "^1.6",
         "php-http/guzzle6-adapter": "^2.0",

symfony.lock

@@ -155,6 +155,9 @@
     "egulias/email-validator": {
         "version": "2.1.19"
     },
+    "enshrined/svg-sanitize": {
+        "version": "0.15.4"
+    },
     "fakerphp/faker": {
         "version": "v1.12.0"
     },

tests/Functional/ImageUploaderTest.php

@@ -0,0 +1,59 @@
+<?php
+
+/*
+ * This file is part of the Sylius package.
+ *
+ * (c) Paweł Jędrzejewski
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Sylius\Tests\Functional;
+
+use PHPUnit\Framework\Assert;
+use Sylius\Component\Core\Model\ProductImage;
+use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
+use Symfony\Component\HttpFoundation\File\Exception\FileException;
+use Symfony\Component\HttpFoundation\File\UploadedFile;
+use Symfony\Component\BrowserKit\Client;
+
+final class ImageUploaderTest extends WebTestCase
+{
+    /** @var Client */
+    private static $client;
+
+    /** @test */
+    public function it_sanitizes_file_content_if_it_is_svg_mime_type(): void
+    {
+        self::$client = static::createClient();
+        self::$container = self::$client->getContainer();
+
+        $imageUploader = self::$container->get('sylius.image_uploader');
+        $fileSystem =  self::$container->get('gaufrette.sylius_image_filesystem');
+
+        $file = new UploadedFile(__DIR__ . '/../Resources/xss.svg', 'xss.svg');
+        Assert::assertStringContainsString('<script', $this->getContent($file));
+
+        $image = new ProductImage();
+        $image->setFile($file);
+
+        $imageUploader->upload($image);
+
+        $sanitizedFile = $fileSystem->get($image->getPath());
+        Assert::assertStringNotContainsString('<script', $sanitizedFile->getContent());
+    }
+
+    private function getContent(UploadedFile $file): string
+    {
+        $content = file_get_contents($file->getPathname());
+
+        if (false === $content) {
+            throw new FileException(sprintf('Could not get the content of the file "%s".', $file->getPathname()));
+        }
+
+        return $content;
+    }
+}