BUGFIX: Sanitize uploaded svg files for suspicious contents

Adding an internal methods `isSanitizingRequired` and `sanitizeImportedFileContent` to the resourceManager. The import is adjusted to first determine the mediaType of an imported resource to decide wether sanitizing is needed which for now happens only for svg files. If no sanitizing is needed the code will perform as before by passing streams or filenames around. If suspicious content was removed from a warning is logged that mentions the remove data and line. The sanitizing is done using "enshrined/svg-sanitize" that is used by other cms aswell. The initial implementation will only sanitize SVG files as those can contain malicious scripts. In future this should be expanded to a feature that allows registering of custom sanitizing functions.
neos · Sep 25, 2023 · 8af17d8 · 8af17d8
1 parent 848edb4
commit 8af17d8
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 6 deletions.
diff --git a/Neos.Flow/Classes/ResourceManagement/ResourceManager.php b/Neos.Flow/Classes/ResourceManagement/ResourceManager.php
@@ -11,11 +11,14 @@
  * source code.
  */
 
+use enshrined\svgSanitize\Sanitizer;
 use Neos\Flow\Annotations as Flow;
 use Neos\Flow\Log\Utility\LogEnvironment;
 use Neos\Flow\ObjectManagement\ObjectManagerInterface;
 use Neos\Flow\Persistence\PersistenceManagerInterface;
+use Neos\Utility\MediaTypes;
 use Neos\Utility\ObjectAccess;
+use Neos\Flow\ResourceManagement\Storage\Exception as StorageException;
 use Neos\Flow\ResourceManagement\Storage\StorageInterface;
 use Neos\Flow\ResourceManagement\Storage\WritableStorageInterface;
 use Neos\Flow\ResourceManagement\Target\TargetInterface;
@@ -160,18 +163,36 @@ public function importResource($source, $collectionName = ResourceManager::DEFAU
         $collection = $this->collections[$collectionName];
 
         try {
-            $resource = $collection->importResource($source);
-            if ($forcedPersistenceObjectIdentifier !== null) {
-                ObjectAccess::setProperty($resource, 'Persistence_Object_Identifier', $forcedPersistenceObjectIdentifier, true);
-            }
-            if (!is_resource($source)) {
+            if (is_resource($source)) {
+                $mediaType = MediaTypes::getMediaTypeFromResource($source);
+                if ($this->isSanitizingRequired($mediaType)) {
+                    $content = stream_get_contents($source);
+                    $resource = $this->importResourceFromContent($content, '', $collectionName, $forcedPersistenceObjectIdentifier);
+                } else {
+                    $resource = $collection->importResource($source);
+                }
+            } else {
+                $resource = fopen($source, 'rb');
+                $mediaType = MediaTypes::getMediaTypeFromResource($resource);
+                fclose($resource);
+                if ($this->isSanitizingRequired($mediaType)) {
+                    $content = file_get_contents($source);
+                    /** @var string $filename */
+                    $resource = $this->importResourceFromContent($content, '', $collectionName, $forcedPersistenceObjectIdentifier);
+                } else {
+                    $resource = $collection->importResource($source);
+                }
                 $pathInfo = UnicodeFunctions::pathinfo($source);
                 $resource->setFilename($pathInfo['basename']);
             }
         } catch (Exception $exception) {
             throw new Exception(sprintf('Importing a file into the resource collection "%s" failed: %s', $collectionName, $exception->getMessage()), 1375197120, $exception);
         }
 
+        if ($forcedPersistenceObjectIdentifier !== null) {
+            ObjectAccess::setProperty($resource, 'Persistence_Object_Identifier', $forcedPersistenceObjectIdentifier, true);
+        }
+
         $this->resourceRepository->add($resource);
         $this->logger->debug(sprintf('Successfully imported file "%s" into the resource collection "%s" (storage: %s, a %s. SHA1: %s)', $source, $collectionName, $collection->getStorage()->getName(), get_class($collection), $resource->getSha1()));
         return $resource;
@@ -206,6 +227,11 @@ public function importResourceFromContent($content, $filename, $collectionName =
             throw new Exception(sprintf('Tried to import a file into the resource collection "%s" but no such collection exists. Please check your settings and the code which triggered the import.', $collectionName), 1380878131);
         }
 
+        $mediaType = MediaTypes::getMediaTypeFromFileContent($content);
+        if ($this->isSanitizingRequired($mediaType)) {
+            $content = $this->sanitizeImportedFileContent($mediaType, $content, $filename);
+        }
+
         /* @var CollectionInterface $collection */
         $collection = $this->collections[$collectionName];
 
@@ -608,6 +634,44 @@ protected function initializeCollections()
         }
     }
 
+    /**
+     * Decide weather the given media-type has to be sanitized
+     * for now this only checks svg file to solve the issue here https://nvd.nist.gov/vuln/detail/CVE-2023-37611
+     *
+     * @todo create a feature from this and allow to register code for sanitizing file content before importing
+     */
+    protected function isSanitizingRequired(string $mediaType): bool
+    {
+        return $mediaType === 'image/svg+xml';
+    }
+
+    /**
+     * Sanitize file content and remove content that is suspicious
+     * for now this only checks svg file to solve the issue here https://nvd.nist.gov/vuln/detail/CVE-2023-37611
+     *
+     * @todo create a feature from this and allow to register code for sanitizing file content before importing
+     */
+    protected function sanitizeImportedFileContent(string $mediaType, string $content, $filename = ''): string
+    {
+        if ($mediaType === 'image/svg+xml') {
+            // @todo: Simplify again when https://github.com/darylldoyle/svg-sanitizer/pull/90 is merged and released.
+            $previousXmlErrorHandling = libxml_use_internal_errors(true);
+            $sanitizer = new Sanitizer();
+            $sanitizedContent = $sanitizer->sanitize($content);
+            libxml_clear_errors();
+            libxml_use_internal_errors($previousXmlErrorHandling);
+            $issues = $sanitizer->getXmlIssues();
+            if ($issues && count($issues) > 0) {
+                if ($sanitizedContent === false) {
+                    throw new Exception('Sanitizing of suspicious file "' . $filename . '" failed during import.', 1695395560);
+                }
+                $content = $sanitizedContent;
+                $this->logger->warning(sprintf('Imported file "%s" contained suspicious content and was sanitized.', $filename), $issues);
+            }
+        }
+        return $content;
+    }
+
     /**
      * Prepare an uploaded file to be imported as resource object. Will check the validity of the file,
      * move it outside of upload folder if open_basedir is enabled and check the filename.

diff --git a/Neos.Flow/composer.json b/Neos.Flow/composer.json
@@ -51,7 +51,8 @@
 
         "composer/composer": "^2.2.8",
 
-        "egulias/email-validator": "^2.1.17 || ^3.0"
+        "egulias/email-validator": "^2.1.17 || ^3.0",
+        "enshrined/svg-sanitize": "^0.16.0"
     },
     "require-dev": {
         "vimeo/psalm": "~4.30.0",

diff --git a/Neos.Utility.MediaTypes/Classes/MediaTypes.php b/Neos.Utility.MediaTypes/Classes/MediaTypes.php
@@ -11,6 +11,7 @@
  * source code.
  */
 
+
 /**
  * Utility class for converting Internet Media Types to file extensions and vice
  * versa.
@@ -1830,6 +1831,21 @@ public static function getMediaTypeFromFileContent(string $fileContent): string
         return isset(self::$mediaTypeToFileExtension[$mediaType]) ? $mediaType : 'application/octet-stream';
     }
 
+    /**
+     * Returns a Media Type based on the given resource
+     *
+     * @param resource $resource The resource to determine the media type from
+     * @return string The IANA Internet Media Type
+     */
+    public static function getMediaTypeFromResource($resource): string
+    {
+        if (!is_resource($resource)) {
+            throw new \TypeError('Argument "resource" has to be a resource');
+        }
+        $mediaType = self::trimMediaType(mime_content_type($resource));
+        return isset(self::$mediaTypeToFileExtension[$mediaType]) ? $mediaType : 'application/octet-stream';
+    }
+
     /**
      * Returns the primary filename extension based on the given Media Type.
      *

diff --git a/Neos.Utility.MediaTypes/Tests/Unit/MediaTypesTest.php b/Neos.Utility.MediaTypes/Tests/Unit/MediaTypesTest.php
@@ -68,6 +68,22 @@ public function getMediaTypeFromFileContent(string $filename, string $expectedMe
         self::assertSame($expectedMediaType, MediaTypes::getMediaTypeFromFileContent($fileContent));
     }
 
+    /**
+     * @test
+     * @dataProvider filesAndMediaTypes
+     */
+    public function getMediaTypeFromResource(string $filename, string $expectedMediaType)
+    {
+        $filePath = __DIR__ . '/Fixtures/' . $filename;
+        $resource = is_file($filePath) ? fopen($filePath, 'rb') : fopen('data://text/plain,', 'rb');
+        if ($resource !== false) {
+            self::assertSame($expectedMediaType, MediaTypes::getMediaTypeFromResource($resource));
+            fclose($resource);
+        } else {
+            $this->fail('fixture ' . $filePath . ' could not be read');
+        }
+    }
+
     /**
      * Data Provider
      */

diff --git a/composer.json b/composer.json
@@ -32,6 +32,7 @@
         "neos/composer-plugin": "^2.0",
         "composer/composer": "^2.2.8",
         "egulias/email-validator": "^2.1.17 || ^3.0",
+        "enshrined/svg-sanitize": "^0.16.0",
         "typo3fluid/fluid": "~2.7.0",
         "guzzlehttp/psr7": "^1.8.4",
         "ext-mbstring": "*"