[SECURITY] Ensure validity of parameters submitted to ThumbnailController 53/57953/2
authorOliver Hader <oliver@typo3.org>
Fri, 17 Aug 2018 06:49:14 +0000 (08:49 +0200)
committerFrank Naegler <frank.naegler@typo3.org>
Fri, 17 Aug 2018 17:51:25 +0000 (19:51 +0200)
Parameters submitted to ThumbnailController via HTTP GET query parameters
can contain arbitrary information. Thus, it has to be verified that those
parameters are valid by signing them with a HMAC.

Prior to that the source code was vulnerable to information disclosure as
well as denial of service attacks due to unsanitized user input. A valid
backend user account was required in order to make use of these flaws.

Since the change which introduced this behavior was not released yet, the
security fixes are handled in public without additional announcements.

Resolves: #85875
Releases: master, 8.7
Change-Id: Ia53ba3756f140b0728b8fd1fb7e0527836639a6b
Reviewed-on: https://review.typo3.org/57953
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Frank Naegler <frank.naegler@typo3.org>
Tested-by: Frank Naegler <frank.naegler@typo3.org>
typo3/sysext/backend/Classes/Controller/File/ThumbnailController.php
typo3/sysext/backend/Classes/Utility/BackendUtility.php
typo3/sysext/backend/Tests/Unit/Controller/File/ThumbnailControllerTest.php [new file with mode: 0644]

index 734c13b..e38092c 100644 (file)
@@ -18,10 +18,10 @@ namespace TYPO3\CMS\Backend\Controller\File;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use TYPO3\CMS\Core\Http\Response;
-use TYPO3\CMS\Core\Resource\File;
 use TYPO3\CMS\Core\Resource\ProcessedFile;
 use TYPO3\CMS\Core\Resource\ResourceFactory;
 use TYPO3\CMS\Core\Utility\ArrayUtility;
+use TYPO3\CMS\Core\Utility\GeneralUtility;
 
 /**
  * Class ThumbnailController
@@ -29,35 +29,88 @@ use TYPO3\CMS\Core\Utility\ArrayUtility;
 class ThumbnailController
 {
     /**
+     * @var array
+     */
+    protected $defaultConfiguration = [
+        'width' => 64,
+        'height' => 64,
+        'crop' => null,
+    ];
+
+    /**
      * @param ServerRequestInterface $request
      * @return ResponseInterface
      */
     public function render(ServerRequestInterface $request): ResponseInterface
     {
-        $fileObject = $this->getFileObjectByCombinedIdentifier($request->getQueryParams()['fileIdentifier']);
-        if (!$fileObject->isMissing()) {
-            $processingInstructions = [
-                'width' => 64,
-                'height' => 64,
-                'crop' => null,
-            ];
-            ArrayUtility::mergeRecursiveWithOverrule($processingInstructions, $request->getQueryParams()['processingInstructions']);
-            $processedImage = $fileObject->process(ProcessedFile::CONTEXT_IMAGECROPSCALEMASK, $processingInstructions);
-            $filePath = $processedImage->getForLocalProcessing(false);
-            return new Response($filePath, 200, [
-                'Content-Type' => $processedImage->getMimeType()
-            ]);
+        try {
+            $parameters = $this->extractParameters($request->getQueryParams());
+            $response = $this->generateThumbnail(
+                $parameters['fileId'] ?? null,
+                $parameters['configuration'] ?? []
+            );
+        } catch (\TYPO3\CMS\Core\Resource\Exception $exception) {
+            // catch and handle only resource related exceptions
+            $response = $this->generateNotFoundResponse();
         }
-        return new Response('', 404);
+
+        return $response;
+    }
+
+    /**
+     * @param array $queryParameters
+     * @return array|null
+     */
+    protected function extractParameters(array $queryParameters)
+    {
+        $expectedHash = GeneralUtility::hmac(
+            $queryParameters['parameters'] ?? '',
+            ThumbnailController::class
+        );
+        if (!hash_equals($expectedHash, $queryParameters['hmac'] ?? '')) {
+            throw new \InvalidArgumentException(
+                'HMAC could not be verified',
+                1534484203
+            );
+        }
+
+        return json_decode($queryParameters['parameters'] ?? null, true);
     }
 
     /**
-     * @param string $combinedIdentifier
-     * @return File
-     * @throws \InvalidArgumentException
+     * @param mixed|int $fileId
+     * @param array $configuration
+     * @return Response
+     * @throws \TYPO3\CMS\Core\Resource\Exception\FileDoesNotExistException
      */
-    protected function getFileObjectByCombinedIdentifier(string $combinedIdentifier): File
+    protected function generateThumbnail($fileId, array $configuration): ResponseInterface
     {
-        return ResourceFactory::getInstance()->getFileObjectFromCombinedIdentifier($combinedIdentifier);
+        $file = ResourceFactory::getInstance()->getFileObject($fileId);
+        if (empty($file) || $file->isMissing()) {
+            return $this->generateNotFoundResponse();
+        }
+
+        $processingConfiguration = $this->defaultConfiguration;
+        ArrayUtility::mergeRecursiveWithOverrule(
+            $processingConfiguration,
+            $configuration
+        );
+
+        $processedImage = $file->process(
+            ProcessedFile::CONTEXT_IMAGECROPSCALEMASK,
+            $processingConfiguration
+        );
+        $filePath = $processedImage->getForLocalProcessing(false);
+        return new Response($filePath, 200, [
+            'Content-Type' => $processedImage->getMimeType()
+        ]);
+    }
+
+    /**
+     * @return ResponseInterface
+     */
+    protected function generateNotFoundResponse(): ResponseInterface
+    {
+        return new Response('', 404);
     }
 }
index 7aa8517..3f3e123 100644 (file)
@@ -1726,16 +1726,23 @@ class BackendUtility
                 ) {
                     $cropVariantCollection = CropVariantCollection::create((string)$fileReferenceObject->getProperty('crop'));
                     $cropArea = $cropVariantCollection->getCropArea();
-                    $processingInformation = [
-                        'width' => $sizeParts[0],
-                        'height' => $sizeParts[1] . 'c',
-                        'crop' => $cropArea->isEmpty() ? null : $cropArea->makeAbsoluteBasedOnFile($fileReferenceObject)
+                    $parameters = json_encode([
+                        'fileId' => $fileObject->getUid(),
+                        'configuration' => [
+                            'width' => $sizeParts[0],
+                            'height' => $sizeParts[1] . 'c',
+                            'crop' => $cropArea->isEmpty() ? null : $cropArea->makeAbsoluteBasedOnFile($fileReferenceObject),
+                        ]
+                    ]);
+                    $uriParameters = [
+                        'parameters' => $parameters,
+                        'hmac' => GeneralUtility::hmac(
+                            $parameters,
+                            \TYPO3\CMS\Backend\Controller\File\ThumbnailController::class
+                        ),
                     ];
                     $imageUrl = (string)GeneralUtility::makeInstance(UriBuilder::class)
-                        ->buildUriFromRoute('thumbnails', [
-                            'fileIdentifier' => $fileObject->getCombinedIdentifier(),
-                            'processingInstructions' => $processingInformation
-                        ]);
+                        ->buildUriFromRoute('thumbnails', $uriParameters);
                     $attributes = [
                         'src' => $imageUrl,
                         'width' => (int)$sizeParts[0],
diff --git a/typo3/sysext/backend/Tests/Unit/Controller/File/ThumbnailControllerTest.php b/typo3/sysext/backend/Tests/Unit/Controller/File/ThumbnailControllerTest.php
new file mode 100644 (file)
index 0000000..ed5a4fe
--- /dev/null
@@ -0,0 +1,125 @@
+<?php
+namespace TYPO3\CMS\Backend\Tests\Unit\Controller\File;
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+use PHPUnit\Framework\MockObject\MockObject;
+use TYPO3\CMS\Backend\Controller\File\ThumbnailController;
+use TYPO3\CMS\Core\Http\Response;
+use TYPO3\CMS\Core\Utility\GeneralUtility;
+
+/**
+ * Tests for \TYPO3\CMS\Backend\Controller\File\ThumbnailController
+ */
+class ThumbnailControllerTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
+{
+    /**
+     * @var ThumbnailController|MockObject
+     */
+    protected $subject;
+
+    /**
+     * @var array
+     */
+    protected static $parameters = [
+        'fileId' => 123,
+        'configuration' => [
+            'width' => 64,
+            'heigth' => 64,
+        ],
+    ];
+
+    protected function setUp()
+    {
+        $GLOBALS['TYPO3_CONF_VARS']['SYS']['encryptionKey']
+            = '4408d27a916d51e624b69af3554f516dbab61037a9f7b9fd6f81b4d3bedeccb6';
+        $this->subject = $this->createPartialMock(
+            ThumbnailController::class,
+            ['generateThumbnail']
+        );
+    }
+
+    /**
+     * @param string $hmac
+     *
+     * @test
+     * @dataProvider exceptionIsThrownOnInvalidHMACDataProvider
+     */
+    public function exceptionIsThrownOnInvalidHMAC(string $hmac = null)
+    {
+        $this->expectException(\InvalidArgumentException::class);
+        $this->expectExceptionCode(1534484203);
+
+        $queryParameters = [
+            'parameters' => json_encode(static::$parameters),
+            'hmac' => $hmac,
+        ];
+
+        $request = (new \TYPO3\CMS\Core\Http\ServerRequest())
+            ->withQueryParams($queryParameters);
+        $this->subject->render($request);
+    }
+
+    /**
+     * @return array
+     */
+    public function exceptionIsThrownOnInvalidHMACDataProvider(): array
+    {
+        return [
+            'null' => [null],
+            'empty' => [''],
+            'invalid' => ['invalid'],
+        ];
+    }
+
+    /**
+     * @param array|null $parameters
+     *
+     * @test
+     * @dataProvider generateThumbnailIsInvokedDataProvider
+     */
+    public function generateThumbnailIsInvoked(array $parameters = null)
+    {
+        $this->subject->expects(static::once())
+            ->method('generateThumbnail')
+            ->willReturn(new Response());
+
+        $queryParameters = [
+            'parameters' => json_encode($parameters),
+            'hmac' => GeneralUtility::hmac(
+                json_encode($parameters),
+                ThumbnailController::class
+            ),
+        ];
+
+        $request = (new \TYPO3\CMS\Core\Http\ServerRequest())
+            ->withQueryParams($queryParameters);
+        static::assertInstanceOf(
+            Response::class,
+            $this->subject->render($request)
+        );
+    }
+
+    /**
+     * @return array
+     */
+    public function generateThumbnailIsInvokedDataProvider(): array
+    {
+        return [
+            'null' => [null],
+            'empty array' => [[]],
+            'parameters' => [static::$parameters],
+        ];
+    }
+}