[SECURITY] Properly escape videoId for YouTube/Vimeo

Resolves: #83184 Releases: master, 8.7, 7.6 Security-Commit: c51313ed68970cd6d2f2172a0e3d74454cf05812 Security-Bulletin: TYPO3-CORE-SA-2018-006 Change-Id: Id982d4fc28e7817eeb88eb63f52dc3380365f3b1 Reviewed-on: https://review.typo3.org/59100 Reviewed-by: Oliver Hader <oliver.hader@typo3.org> Tested-by: Oliver Hader <oliver.hader@typo3.org>
TYPO3 · Dec 11, 2018 · a32a9a7 · a32a9a7
1 parent 6959fc7
commit a32a9a7
Show file tree

Hide file tree

Showing 6 changed files with 129 additions and 44 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
@@ -19,7 +19,6 @@
 use TYPO3\CMS\Core\Resource\FileReference;
 use TYPO3\CMS\Core\Resource\OnlineMedia\Helpers\OnlineMediaHelperInterface;
 use TYPO3\CMS\Core\Resource\OnlineMedia\Helpers\OnlineMediaHelperRegistry;
-use TYPO3\CMS\Core\Utility\GeneralUtility;
 
 /**
  * Vimeo renderer class
@@ -96,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)
         );
     }
 
@@ -146,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));
     }
 
     /**
@@ -168,34 +167,59 @@ 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 (isset($options['additionalAttributes']) && is_array($options['additionalAttributes'])) {
-            $attributes[] = GeneralUtility::implodeAttributes($options['additionalAttributes'], true, true);
+            $attributes = array_merge($attributes, $options['additionalAttributes']);
         }
         if (isset($options['data']) && is_array($options['data'])) {
-            array_walk($options['data'], function (&$value, $key) {
-                $value = 'data-' . htmlspecialchars($key) . '="' . htmlspecialchars($value) . '"';
-            });
-            $attributes[] = implode(' ', $options['data']);
+            array_walk(
+                $options['data'],
+                function (&$value, $key) use (&$attributes) {
+                    $attributes['data-' . $key] = $value;
+                }
+            );
         }
         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 (isset($GLOBALS['TSFE']) && 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,36 +185,55 @@ 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 (isset($options['additionalAttributes']) && is_array($options['additionalAttributes'])) {
-            $attributes[] = GeneralUtility::implodeAttributes($options['additionalAttributes'], true, true);
+            $attributes = array_merge($attributes, $options['additionalAttributes']);
         }
         if (isset($options['data']) && is_array($options['data'])) {
-            array_walk($options['data'], function (&$value, $key) {
-                $value = 'data-' . htmlspecialchars($key) . '="' . htmlspecialchars($value) . '"';
+            array_walk($options['data'], function (&$value, $key) use (&$attributes) {
+                $attributes['data-' . $key] = $value;
             });
-            $attributes[] = implode(' ', $options['data']);
         }
         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 (isset($GLOBALS['TSFE']) && is_object($GLOBALS['TSFE']) && $GLOBALS['TSFE']->config['config']['doctype'] !== 'html5') {
-            $attributes[] = 'frameborder="0"';
+            $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
@@ -167,8 +167,8 @@ public function renderOutputWithAdditionalAttributes()
         $fileResourceMock = $this->createMock(File::class);
 
         $this->assertSame(
-            '<iframe src="https://player.vimeo.com/video/7331?title=0&amp;byline=0&amp;portrait=0" allowfullscreen foo="bar" custom-play="preload" width="300" height="200" allow="fullscreen"></iframe>',
-            $this->subject->render($fileResourceMock, '300m', '200', ['additionalAttributes' => ['foo' => 'bar', 'custom-play' => 'preload']])
+            '<iframe src="https://player.vimeo.com/video/7331?title=0&amp;byline=0&amp;portrait=0" allowfullscreen foo="bar" custom-play="preload" sanitizetest="&lt;&gt;&quot;&apos;test" width="300" height="200" allow="fullscreen"></iframe>',
+            $this->subject->render($fileResourceMock, '300m', '200', ['additionalAttributes' => ['foo' => 'bar', 'custom-play' => 'preload', '<"\'>sanitize^&test' => '<>"\'test']])
         );
     }
 
@@ -181,8 +181,8 @@ public function renderOutputWithDataAttributesForCustomization()
         $fileResourceMock = $this->createMock(File::class);
 
         $this->assertSame(
-            '<iframe src="https://player.vimeo.com/video/7331?title=0&amp;byline=0&amp;portrait=0" allowfullscreen data-player-handler="vimeo" data-custom-playerId="player-123" width="300" height="200" allow="fullscreen"></iframe>',
-            $this->subject->render($fileResourceMock, '300m', '200', ['data' => ['player-handler' => 'vimeo', 'custom-playerId' => 'player-123']])
+            '<iframe src="https://player.vimeo.com/video/7331?title=0&amp;byline=0&amp;portrait=0" allowfullscreen data-player-handler="vimeo" data-custom-playerId="player-123" data-sanitizetest="test" width="300" height="200" allow="fullscreen"></iframe>',
+            $this->subject->render($fileResourceMock, '300m', '200', ['data' => ['player-handler' => 'vimeo', 'custom-playerId' => 'player-123', '*sanitize&test"' => 'test']])
         );
     }
 
@@ -248,4 +248,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
@@ -253,8 +253,8 @@ public function renderOutputWithAdditionalAttributes()
         $fileResourceMock = $this->createMock(File::class);
 
         $this->assertSame(
-            '<iframe src="https://www.youtube-nocookie.com/embed/7331?autohide=1&amp;controls=0&amp;enablejsapi=1&amp;origin=http%3A%2F%2Ftest.server.org&amp;showinfo=0" allowfullscreen foo="bar" custom-play="preload" width="300" height="200" allow="fullscreen"></iframe>',
-            $this->subject->render($fileResourceMock, '300m', '200', ['controls' => 0, 'additionalAttributes' => ['foo' => 'bar', 'custom-play' => 'preload']])
+            '<iframe src="https://www.youtube-nocookie.com/embed/7331?autohide=1&amp;controls=0&amp;enablejsapi=1&amp;origin=http%3A%2F%2Ftest.server.org&amp;showinfo=0" allowfullscreen foo="bar" custom-play="preload" sanitizetest="&lt;&gt;&quot;&apos;test" width="300" height="200" allow="fullscreen"></iframe>',
+            $this->subject->render($fileResourceMock, '300m', '200', ['controls' => 0, 'additionalAttributes' => ['foo' => 'bar', 'custom-play' => 'preload', '<"\'>sanitize^&test' => '<>"\'test']])
         );
     }
 
@@ -341,4 +341,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')
+        );
+    }
 }