[SECURITY] Properly escape videoId for YouTube/Vimeo

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 · Dec 11, 2018 · c917493 · c917493
1 parent 4d6833a
commit c917493
Show file tree

Hide file tree

Showing 6 changed files with 123 additions and 34 deletions.
diff --git a/typo3/sysext/core/Classes/Resource/OnlineMedia/Helpers/VimeoHelper.php b/typo3/sysext/core/Classes/Resource/OnlineMedia/Helpers/VimeoHelper.php
@@ -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 @@ protected function getOEmbedUrl($mediaId, $format = 'json')
     {
         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)))
         );
     }
 }
diff --git a/typo3/sysext/core/Classes/Resource/OnlineMedia/Helpers/YouTubeHelper.php b/typo3/sysext/core/Classes/Resource/OnlineMedia/Helpers/YouTubeHelper.php
@@ -19,7 +19,7 @@
 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 @@ protected function getOEmbedUrl($mediaId, $format = 'json')
     {
         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)
         );
     }

diff --git a/typo3/sysext/core/Classes/Resource/Rendering/VimeoRenderer.php b/typo3/sysext/core/Classes/Resource/Rendering/VimeoRenderer.php
@@ -95,8 +95,8 @@ public function render(FileInterface $file, $width, $height, array $options = []
 
         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 @@ protected function createVimeoUrl(array $options, FileInterface $file)
         $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 @@ protected function getVideoIdFromFile(FileInterface $file)
      * @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);
+    }
 }
diff --git a/typo3/sysext/core/Classes/Resource/Rendering/YouTubeRenderer.php b/typo3/sysext/core/Classes/Resource/Rendering/YouTubeRenderer.php
@@ -96,8 +96,8 @@ public function render(FileInterface $file, $width, $height, array $options = []
 
         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 @@ protected function createYouTubeUrl(array $options, FileInterface $file)
             $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 @@ protected function getVideoIdFromFile(FileInterface $file)
      * @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);
+    }
 }
diff --git a/typo3/sysext/core/Tests/Unit/Resource/Rendering/VimeoRendererTest.php b/typo3/sysext/core/Tests/Unit/Resource/Rendering/VimeoRendererTest.php
@@ -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\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 @@ public function renderOutputWithPrivateVimeoCodeIsCorrect()
             $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')
+        );
+    }
 }
diff --git a/typo3/sysext/core/Tests/Unit/Resource/Rendering/YouTubeRendererTest.php b/typo3/sysext/core/Tests/Unit/Resource/Rendering/YouTubeRendererTest.php
@@ -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\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 @@ public function renderOutputWithLoopIsCorrect()
 
         $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 @@ public function renderOutputWithAutoplayIsCorrect()
 
         $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 @@ public function renderOutputWithAutoplayFromFileReferenceIsCorrect()
 
         $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 @@ public function renderOutputWithCustomAllowAndAutoplayIsCorrect()
             $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')
+        );
+    }
 }