MAGETWO-85143: Stored XSS - Downloadable products (Exfiltrating database, admin takeover)

Author: svitja

app/code/Magento/Downloadable/Controller/Download.php

@@ -6,6 +6,7 @@
 namespace Magento\Downloadable\Controller;
 
 use Magento\Downloadable\Helper\Download as DownloadHelper;
+use Magento\Framework\App\Response\Http as HttpResponse;
 
 /**
  * Download controller
@@ -14,6 +15,13 @@
  */
 abstract class Download extends \Magento\Framework\App\Action\Action
 {
+    /**
+     * @var array
+     */
+    private $disallowedContentTypes = [
+        'text/html',
+    ];
+
     /**
      * Prepare response to output resource contents
      *
@@ -28,9 +36,12 @@ protected function _processDownload($path, $resourceType)
 
         $helper->setResource($path, $resourceType);
         $fileName = $helper->getFilename();
+
         $contentType = $helper->getContentType();
 
-        $this->getResponse()->setHttpResponseCode(
+        /** @var HttpResponse $response */
+        $response = $this->getResponse();
+        $response->setHttpResponseCode(
             200
         )->setHeader(
             'Pragma',
@@ -47,15 +58,19 @@ protected function _processDownload($path, $resourceType)
         );
 
         if ($fileSize = $helper->getFileSize()) {
-            $this->getResponse()->setHeader('Content-Length', $fileSize);
+            $response->setHeader('Content-Length', $fileSize);
         }
 
-        if ($contentDisposition = $helper->getContentDisposition()) {
-            $this->getResponse()->setHeader('Content-Disposition', $contentDisposition . '; filename=' . $fileName);
+        $contentDisposition = $helper->getContentDisposition();
+        if (!$contentDisposition || in_array($contentType, $this->disallowedContentTypes)) {
+            // For security reasons we force browsers to download the file instead of opening it.
+            $contentDisposition = \Zend_Mime::DISPOSITION_ATTACHMENT;
         }
 
-        $this->getResponse()->clearBody();
-        $this->getResponse()->sendHeaders();
+        $response->setHeader('Content-Disposition', $contentDisposition  . '; filename=' . $fileName);
+        //Rendering
+        $response->clearBody();
+        $response->sendHeaders();
 
         $helper->output();
     }

app/code/Magento/Downloadable/Test/Unit/Controller/Download/LinkTest.php

@@ -273,7 +273,13 @@ public function testGetLinkForWrongCustomer()
         $this->assertEquals($this->response, $this->link->execute());
     }
 
-    public function testExceptionInUpdateLinkStatus()
+    /**
+     * @param string $mimeType
+     * @param string $disposition
+     * @dataProvider downloadTypesDataProvider
+     * @return void
+     */
+    public function testExceptionInUpdateLinkStatus($mimeType, $disposition)
     {
         $this->objectManager->expects($this->at(0))
             ->method('get')
@@ -303,7 +309,7 @@ public function testExceptionInUpdateLinkStatus()
         $this->linkPurchasedItem->expects($this->once())->method('getLinkType')->willReturn('url');
         $this->linkPurchasedItem->expects($this->once())->method('getLinkUrl')->willReturn('link_url');
 
-        $this->processDownload('link_url', 'url');
+        $this->processDownload('link_url', 'url', $mimeType, $disposition);
 
         $this->linkPurchasedItem->expects($this->any())->method('setNumberOfDownloadsUsed')->willReturnSelf();
         $this->linkPurchasedItem->expects($this->any())->method('setStatus')->with('expired')->willReturnSelf();
@@ -317,8 +323,18 @@ public function testExceptionInUpdateLinkStatus()
         $this->assertEquals($this->response, $this->link->execute());
     }
 
-    private function processDownload($resource, $resourceType)
+    /**
+     * @param string $resource
+     * @param string $resourceType
+     * @param string $mimeType
+     * @param string $disposition
+     * @return void
+     */
+    private function processDownload($resource, $resourceType, $mimeType, $disposition)
     {
+        $fileSize = 58493;
+        $fileName = 'link.jpg';
+
         $this->objectManager->expects($this->at(3))
             ->method('get')
             ->with(\Magento\Downloadable\Helper\Download::class)
@@ -327,30 +343,23 @@ private function processDownload($resource, $resourceType)
             ->method('setResource')
             ->with($resource, $resourceType)
             ->willReturnSelf();
-        $this->downloadHelper->expects($this->once())->method('getFilename')->willReturn('file_name');
-        $this->downloadHelper->expects($this->once())->method('getContentType')->willReturn('content_type');
+        $this->downloadHelper->expects($this->once())->method('getFilename')->willReturn($fileName);
+        $this->downloadHelper->expects($this->once())->method('getContentType')->willReturn($mimeType);
         $this->response->expects($this->once())->method('setHttpResponseCode')->with(200)->willReturnSelf();
-        $this->response->expects($this->at(1))->method('setHeader')->with('Pragma', 'public', true)->willReturnSelf();
-        $this->response->expects($this->at(2))
+        $this->response
+            ->expects($this->any())
             ->method('setHeader')
-            ->with('Cache-Control', 'must-revalidate, post-check=0, pre-check=0', true)
-            ->willReturnSelf();
-        $this->response->expects($this->at(3))
-            ->method('setHeader')
-            ->with('Content-type', 'content_type', true)
-            ->willReturnSelf();
-        $this->downloadHelper->expects($this->once())->method('getFileSize')->willReturn('file_size');
-        $this->response->expects($this->at(4))
-            ->method('setHeader')
-            ->with('Content-Length', 'file_size')
-            ->willReturnSelf();
-        $this->downloadHelper->expects($this->once())
-            ->method('getContentDisposition')
-            ->willReturn('content_disposition');
-        $this->response->expects($this->at(5))
-            ->method('setHeader')
-            ->with('Content-Disposition', 'content_disposition; filename=file_name')
+            ->withConsecutive(
+                ['Pragma', 'public', true],
+                ['Cache-Control', 'must-revalidate, post-check=0, pre-check=0', true],
+                ['Content-type', $mimeType, true],
+                ['Content-Length', $fileSize],
+                ['Content-Disposition', $disposition . '; filename=' . $fileName]
+            )
             ->willReturnSelf();
+
+        $this->downloadHelper->expects($this->once())->method('getContentDisposition')->willReturn($disposition);
+        $this->downloadHelper->expects($this->once())->method('getFileSize')->willReturn($fileSize);
         $this->response->expects($this->once())->method('clearBody')->willReturnSelf();
         $this->response->expects($this->once())->method('sendHeaders')->willReturnSelf();
         $this->downloadHelper->expects($this->once())->method('output');
@@ -406,4 +415,15 @@ public function linkNotAvailableDataProvider()
             ['addError', 'wrong_status', 'Something went wrong while getting the requested content.']
         ];
     }
+
+    /**
+     * @return array
+     */
+    public function downloadTypesDataProvider()
+    {
+        return [
+            ['mimeType' => 'text/html',  'disposition' => \Zend_Mime::DISPOSITION_ATTACHMENT],
+            ['mimeType' => 'image/jpeg', 'disposition' => \Zend_Mime::DISPOSITION_INLINE],
+        ];
+    }
 }