[SECURITY] Properly escape videoId for YouTube/Vimeo 92/59092/2
authorSusanne Moog <s.moog@neusta.de>
Tue, 11 Dec 2018 09:56:04 +0000 (10:56 +0100)
committerOliver Hader <oliver.hader@typo3.org>
Tue, 11 Dec 2018 09:56:06 +0000 (10:56 +0100)
Resolves: #83184
Releases: master, 8.7, 7.6
Security-Commit: 20b6cff301205505b620bffb5be4807636b014e7
Security-Bulletin: TYPO3-CORE-SA-2018-006
Change-Id: Ifb6b7588c7a06ca29a7c2a6382f95bbfb52f392e
Reviewed-on: https://review.typo3.org/59092
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
typo3/sysext/core/Classes/Resource/OnlineMedia/Helpers/VimeoHelper.php
typo3/sysext/core/Classes/Resource/OnlineMedia/Helpers/YouTubeHelper.php
typo3/sysext/core/Classes/Resource/Rendering/VimeoRenderer.php
typo3/sysext/core/Classes/Resource/Rendering/YouTubeRenderer.php
typo3/sysext/core/Tests/Unit/Resource/Rendering/VimeoRendererTest.php
typo3/sysext/core/Tests/Unit/Resource/Rendering/YouTubeRendererTest.php

index 968df03..3f017be 100644 (file)
@@ -34,7 +34,7 @@ class VimeoHelper extends AbstractOEmbedHelper
     public function getPublicUrl(File $file, $relativeToCurrentScript = false)
     {
         $videoId = $this->getOnlineMediaId($file);
-        return sprintf('https://vimeo.com/%s', $videoId);
+        return sprintf('https://vimeo.com/%s', rawurlencode($videoId));
     }
 
     /**
@@ -92,8 +92,8 @@ class VimeoHelper extends AbstractOEmbedHelper
     {
         return sprintf(
             'https://vimeo.com/api/oembed.%s?width=2048&url=%s',
-            urlencode($format),
-            urlencode(sprintf('https://vimeo.com/%s', $mediaId))
+            rawurlencode($format),
+            rawurlencode(sprintf('https://vimeo.com/%s', rawurlencode($mediaId)))
         );
     }
 }
index 8eb5b33..90cae5c 100644 (file)
@@ -19,7 +19,7 @@ use TYPO3\CMS\Core\Resource\Folder;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
 
 /**
- * SlideShare helper class
+ * Youtube helper class
  */
 class YouTubeHelper extends AbstractOEmbedHelper
 {
@@ -33,7 +33,7 @@ class YouTubeHelper extends AbstractOEmbedHelper
     public function getPublicUrl(File $file, $relativeToCurrentScript = false)
     {
         $videoId = $this->getOnlineMediaId($file);
-        return sprintf('https://www.youtube.com/watch?v=%s', $videoId);
+        return sprintf('https://www.youtube.com/watch?v=%s', rawurlencode($videoId));
     }
 
     /**
@@ -101,7 +101,7 @@ class YouTubeHelper extends AbstractOEmbedHelper
     {
         return sprintf(
             'https://www.youtube.com/oembed?url=%s&format=%s',
-            urlencode(sprintf('https://www.youtube.com/watch?v=%s', $mediaId)),
+            rawurlencode(sprintf('https://www.youtube.com/watch?v=%s', rawurlencode($mediaId))),
             rawurlencode($format)
         );
     }
index e180ae7..662c7d4 100644 (file)
@@ -95,8 +95,8 @@ class VimeoRenderer implements FileRendererInterface
 
         return sprintf(
             '<iframe src="%s"%s></iframe>',
-            $src,
-            empty($attributes) ? '' : ' ' . implode(' ', $attributes)
+            htmlspecialchars($src, ENT_QUOTES | ENT_HTML5),
+            empty($attributes) ? '' : ' ' . $this->implodeAttributes($attributes)
         );
     }
 
@@ -145,7 +145,7 @@ class VimeoRenderer implements FileRendererInterface
         $urlParams[] = 'byline=' . (int)!empty($options['showinfo']);
         $urlParams[] = 'portrait=0';
 
-        return sprintf('https://player.vimeo.com/video/%s?%s', $videoId, implode('&amp;', $urlParams));
+        return sprintf('https://player.vimeo.com/video/%s?%s', $videoId, implode('&', $urlParams));
     }
 
     /**
@@ -167,25 +167,48 @@ class VimeoRenderer implements FileRendererInterface
      * @param int|string $width
      * @param int|string $height
      * @param array $options
-     * @return array
+     * @return array pairs of key/value; not yet html-escaped
      */
     protected function collectIframeAttributes($width, $height, array $options)
     {
-        $attributes = ['allowfullscreen'];
+        $attributes = [];
+        $attributes['allowfullscreen'] = true;
+
         if ((int)$width > 0) {
-            $attributes[] = 'width="' . (int)$width . '"';
+            $attributes['width'] = (int)$width;
         }
         if ((int)$height > 0) {
-            $attributes[] = 'height="' . (int)$height . '"';
+            $attributes['height'] = (int)$height;
         }
-        if (is_object($GLOBALS['TSFE']) && $GLOBALS['TSFE']->config['config']['doctype'] !== 'html5') {
-            $attributes[] = 'frameborder="0"';
+        if (isset($GLOBALS['TSFE']) &&
+            is_object($GLOBALS['TSFE']) &&
+            $GLOBALS['TSFE']->config['config']['doctype'] !== 'html5') {
+            $attributes['frameborder'] = 0;
         }
         foreach (['class', 'dir', 'id', 'lang', 'style', 'title', 'accesskey', 'tabindex', 'onclick', 'allow'] as $key) {
             if (!empty($options[$key])) {
-                $attributes[] = $key . '="' . htmlspecialchars($options[$key]) . '"';
+                $attributes[$key] = $options[$key];
             }
         }
         return $attributes;
     }
+
+    /**
+     * @internal
+     * @param array $attributes
+     * @return string
+     */
+    protected function implodeAttributes(array $attributes): string
+    {
+        $attributeList = [];
+        foreach ($attributes as $name => $value) {
+            $name = preg_replace('/[^\p{L}0-9_.-]/u', '', $name);
+            if ($value === true) {
+                $attributeList[] = $name;
+            } else {
+                $attributeList[] = $name . '="' . htmlspecialchars($value, ENT_QUOTES | ENT_HTML5) . '"';
+            }
+        }
+        return implode(' ', $attributeList);
+    }
 }
index 28c82cb..9699336 100644 (file)
@@ -96,8 +96,8 @@ class YouTubeRenderer implements FileRendererInterface
 
         return sprintf(
             '<iframe src="%s"%s></iframe>',
-            $src,
-            empty($attributes) ? '' : ' ' . implode(' ', $attributes)
+            htmlspecialchars($src, ENT_QUOTES | ENT_HTML5),
+            empty($attributes) ? '' : ' ' . $this->implodeAttributes($attributes)
         );
     }
 
@@ -146,21 +146,21 @@ class YouTubeRenderer implements FileRendererInterface
             $urlParams[] = 'modestbranding=1';
         }
         if (!empty($options['loop'])) {
-            $urlParams[] = 'loop=1&amp;playlist=' . $videoId;
+            $urlParams[] = 'loop=1&playlist=' . rawurlencode($videoId);
         }
         if (isset($options['relatedVideos'])) {
             $urlParams[] = 'rel=' . (int)(bool)$options['relatedVideos'];
         }
         if (!isset($options['enablejsapi']) || !empty($options['enablejsapi'])) {
-            $urlParams[] = 'enablejsapi=1&amp;origin=' . rawurlencode(GeneralUtility::getIndpEnv('TYPO3_REQUEST_HOST'));
+            $urlParams[] = 'enablejsapi=1&origin=' . rawurlencode(GeneralUtility::getIndpEnv('TYPO3_REQUEST_HOST'));
         }
         $urlParams[] = 'showinfo=' . (int)!empty($options['showinfo']);
 
         $youTubeUrl = sprintf(
             'https://www.youtube%s.com/embed/%s?%s',
             !isset($options['no-cookie']) || !empty($options['no-cookie']) ? '-nocookie' : '',
-            $videoId,
-            implode('&amp;', $urlParams)
+            rawurlencode($videoId),
+            implode('&', $urlParams)
         );
 
         return $youTubeUrl;
@@ -185,27 +185,47 @@ class YouTubeRenderer implements FileRendererInterface
      * @param int|string $width
      * @param int|string $height
      * @param array $options
-     * @return array
+     * @return array pairs of key/value; not yet html-escaped
      */
     protected function collectIframeAttributes($width, $height, array $options)
     {
-        $attributes = ['allowfullscreen'];
+        $attributes = [];
+        $attributes['allowfullscreen'] = true;
 
         if ((int)$width > 0) {
-            $attributes[] = 'width="' . (int)$width . '"';
+            $attributes['width'] = (int)$width;
         }
         if ((int)$height > 0) {
-            $attributes[] = 'height="' . (int)$height . '"';
+            $attributes['height'] = (int)$height;
         }
-        if (is_object($GLOBALS['TSFE']) && $GLOBALS['TSFE']->config['config']['doctype'] !== 'html5') {
-            $attributes[] = 'frameborder="0"';
+        if (isset($GLOBALS['TSFE']) && is_object($GLOBALS['TSFE']) && $GLOBALS['TSFE']->config['config']['doctype'] !== 'html5') {
+            $attributes['frameborder'] = 0;
         }
         foreach (['class', 'dir', 'id', 'lang', 'style', 'title', 'accesskey', 'tabindex', 'onclick', 'poster', 'preload', 'allow'] as $key) {
             if (!empty($options[$key])) {
-                $attributes[] = $key . '="' . htmlspecialchars($options[$key]) . '"';
+                $attributes[$key] = $options[$key];
             }
         }
 
         return $attributes;
     }
+
+    /**
+     * @internal
+     * @param array $attributes
+     * @return string
+     */
+    protected function implodeAttributes(array $attributes): string
+    {
+        $attributeList = [];
+        foreach ($attributes as $name => $value) {
+            $name = preg_replace('/[^\p{L}0-9_.-]/u', '', $name);
+            if ($value === true) {
+                $attributeList[] = $name;
+            } else {
+                $attributeList[] = $name . '="' . htmlspecialchars($value, ENT_QUOTES | ENT_HTML5) . '"';
+            }
+        }
+        return implode(' ', $attributeList);
+    }
 }
index 230346f..fb6d7e0 100644 (file)
@@ -1,4 +1,5 @@
 <?php
+declare(strict_types = 1);
 namespace TYPO3\CMS\Core\Tests\Unit\Resource\Rendering;
 
 /*
@@ -18,11 +19,12 @@ use TYPO3\CMS\Core\Resource\File;
 use TYPO3\CMS\Core\Resource\FileReference;
 use TYPO3\CMS\Core\Resource\OnlineMedia\Helpers\VimeoHelper;
 use TYPO3\CMS\Core\Resource\Rendering\VimeoRenderer;
+use TYPO3\TestingFramework\Core\Unit\UnitTestCase;
 
 /**
  * Class VimeoRendererTest
  */
-class VimeoRendererTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
+class VimeoRendererTest extends UnitTestCase
 {
     /**
      * @var VimeoRenderer|\PHPUnit_Framework_MockObject_MockObject
@@ -204,4 +206,25 @@ class VimeoRendererTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
             $subject->render($fileResourceMock, '300m', '200')
         );
     }
+
+    /**
+     * @test
+     */
+    public function renderOutputIsEscaped()
+    {
+        /** @var VimeoHelper|\PHPUnit_Framework_MockObject_MockObject $vimeoHelper */
+        $vimeoHelper = $this->getAccessibleMock(VimeoHelper::class, ['getOnlineMediaId'], ['vimeo']);
+        $vimeoHelper->expects($this->any())->method('getOnlineMediaId')->will($this->returnValue('7331<script>danger</script>\'"random"quotes;'));
+
+        $subject = $this->getAccessibleMock(VimeoRenderer::class, ['getOnlineMediaHelper'], []);
+        $subject->expects($this->any())->method('getOnlineMediaHelper')->will($this->returnValue($vimeoHelper));
+
+        /** @var File|\PHPUnit_Framework_MockObject_MockObject $fileResourceMock */
+        $fileResourceMock = $this->createMock(File::class);
+
+        $this->assertSame(
+            '<iframe src="https://player.vimeo.com/video/7331&lt;script&gt;danger&lt;/script&gt;&apos;&quot;random&quot;quotes;?title=0&amp;byline=0&amp;portrait=0" allowfullscreen width="300" height="200" allow="fullscreen"></iframe>',
+            $subject->render($fileResourceMock, '300m', '200')
+        );
+    }
 }
index 811b93c..429e823 100644 (file)
@@ -1,4 +1,5 @@
 <?php
+declare(strict_types = 1);
 namespace TYPO3\CMS\Core\Tests\Unit\Resource\Rendering;
 
 /*
@@ -18,11 +19,12 @@ use TYPO3\CMS\Core\Resource\File;
 use TYPO3\CMS\Core\Resource\FileReference;
 use TYPO3\CMS\Core\Resource\OnlineMedia\Helpers\YouTubeHelper;
 use TYPO3\CMS\Core\Resource\Rendering\YouTubeRenderer;
+use TYPO3\TestingFramework\Core\Unit\UnitTestCase;
 
 /**
  * Class YouTubeRendererTest
  */
-class YouTubeRendererTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
+class YouTubeRendererTest extends UnitTestCase
 {
     /**
      * @var YouTubeRenderer|\PHPUnit_Framework_MockObject_MockObject
@@ -92,7 +94,7 @@ class YouTubeRendererTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
 
         $this->assertSame(
             '<iframe src="https://www.youtube-nocookie.com/embed/7331?autohide=1&amp;controls=2&amp;loop=1&amp;playlist=7331&amp;enablejsapi=1&amp;origin=http%3A%2F%2Ftest.server.org&amp;showinfo=0" allowfullscreen width="300" height="200" allow="fullscreen"></iframe>',
-            $this->subject->render($fileResourceMock, '300m', '200', ['loop' => 1])
+            $this->subject->render($fileResourceMock, '300m', '200', ['controls' => 2, 'loop' => 1])
         );
     }
 
@@ -106,7 +108,7 @@ class YouTubeRendererTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
 
         $this->assertSame(
             '<iframe src="https://www.youtube-nocookie.com/embed/7331?autohide=1&amp;controls=2&amp;autoplay=1&amp;enablejsapi=1&amp;origin=http%3A%2F%2Ftest.server.org&amp;showinfo=0" allowfullscreen width="300" height="200" allow="autoplay; fullscreen"></iframe>',
-            $this->subject->render($fileResourceMock, '300m', '200', ['autoplay' => 1])
+            $this->subject->render($fileResourceMock, '300m', '200', ['controls' => 2, 'autoplay' => 1])
         );
     }
 
@@ -125,7 +127,7 @@ class YouTubeRendererTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
 
         $this->assertSame(
             '<iframe src="https://www.youtube-nocookie.com/embed/7331?autohide=1&amp;controls=2&amp;autoplay=1&amp;enablejsapi=1&amp;origin=http%3A%2F%2Ftest.server.org&amp;showinfo=0" allowfullscreen width="300" height="200" allow="autoplay; fullscreen"></iframe>',
-            $this->subject->render($fileReferenceMock, '300m', '200')
+            $this->subject->render($fileReferenceMock, '300m', '200', ['controls' => 2])
         );
     }
 
@@ -297,4 +299,25 @@ class YouTubeRendererTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
             $this->subject->render($fileResourceMock, '300m', '200', ['controls' => 0, 'autoplay' => 1, 'allow' => 'foo; bar'])
         );
     }
+
+    /**
+     * @test
+     */
+    public function renderOutputIsEscaped()
+    {
+        /** @var YouTubeHelper|\PHPUnit_Framework_MockObject_MockObject $youtubeHelper */
+        $youtubeHelper = $this->getAccessibleMock(YouTubeHelper::class, ['getOnlineMediaId'], ['youtube']);
+        $youtubeHelper->expects($this->any())->method('getOnlineMediaId')->will($this->returnValue('7331<script>danger</script>\'"random"quotes;'));
+
+        $subject = $this->getAccessibleMock(YouTubeRenderer::class, ['getOnlineMediaHelper'], []);
+        $subject->expects($this->any())->method('getOnlineMediaHelper')->will($this->returnValue($youtubeHelper));
+
+        /** @var File|\PHPUnit_Framework_MockObject_MockObject $fileResourceMock */
+        $fileResourceMock = $this->createMock(File::class);
+
+        $this->assertSame(
+            '<iframe src="https://www.youtube-nocookie.com/embed/7331%3Cscript%3Edanger%3C%2Fscript%3E%27%22random%22quotes%3B?autohide=1&amp;controls=2&amp;enablejsapi=1&amp;origin=http%3A%2F%2Ftest.server.org&amp;showinfo=0" allowfullscreen width="300" height="200" allow="fullscreen"></iframe>',
+            $subject->render($fileResourceMock, '300m', '200')
+        );
+    }
 }