[CVE-2022-38724] Restrict embed shortcode attributes

silverstripe · Nov 20, 2022 · c5578db · c5578db
1 parent 55b23d0
commit c5578db
Show file tree

Hide file tree

Showing 2 changed files with 99 additions and 1 deletion.
diff --git a/src/View/Shortcodes/EmbedShortcodeProvider.php b/src/View/Shortcodes/EmbedShortcodeProvider.php
@@ -19,6 +19,9 @@
 use Embed\Exceptions\InvalidUrlException;
 use SilverStripe\View\Parsers\ShortcodeParser;
 use SilverStripe\Control\Director;
+use SilverStripe\Core\Config\Configurable;
+use SilverStripe\Dev\Deprecation;
+use SilverStripe\View\Embed\EmbedContainer;
 
 /**
  * Provider for the [embed] shortcode tag used by the embedding service
@@ -27,6 +30,23 @@
  */
 class EmbedShortcodeProvider implements ShortcodeHandler
 {
+    use Configurable;
+
+    /**
+     * A whitelist of shortcode attributes which are allowed in the resultant markup.
+     * Note that the tinymce plugin restricts attributes on the client-side separately.
+     *
+     * @config
+     * @deprecated 4.12.0 Removed without equivalent functionality to replace it
+     */
+    private static array $attribute_whitelist = [
+        'url',
+        'thumbnail',
+        'class',
+        'width',
+        'height',
+        'caption',
+    ];
 
     /**
      * Gets the list of shortcodes provided by this handler
@@ -193,9 +213,17 @@ protected static function videoEmbed($arguments, $content)
             }
         }
 
+        $attributes = static::buildAttributeListFromArguments($arguments, ['width', 'height', 'url', 'caption']);
+        if (array_key_exists('style', $arguments)) {
+            $attributes->push(ArrayData::create([
+                'Name' => 'style',
+                'Value' => Convert::raw2att($arguments['style']),
+            ]));
+        }
+
         $data = [
             'Arguments' => $arguments,
-            'Attributes' => static::buildAttributeListFromArguments($arguments, ['width', 'height', 'url', 'caption']),
+            'Attributes' => $attributes,
             'Content' => DBField::create_field('HTMLFragment', $content)
         ];
 
@@ -250,6 +278,12 @@ protected static function photoEmbed($arguments, $src)
      */
     private static function buildAttributeListFromArguments(array $arguments, array $exclude = []): ArrayList
     {
+        // Clean out any empty arguments and anything not whitelisted
+        $whitelist = static::config()->get('attribute_whitelist');
+        $arguments = array_filter($arguments, function ($value, $key) use ($whitelist) {
+            return in_array($key, $whitelist) && strlen(trim($value ?? ''));
+        }, ARRAY_FILTER_USE_BOTH);
+
         $attributes = ArrayList::create();
         foreach ($arguments as $key => $value) {
             if (in_array($key, $exclude)) {

diff --git a/tests/php/View/Shortcodes/EmbedShortcodeProviderTest.php b/tests/php/View/Shortcodes/EmbedShortcodeProviderTest.php
@@ -3,6 +3,7 @@
 namespace SilverStripe\View\Tests\Shortcodes;
 
 use Psr\SimpleCache\CacheInterface;
+use SilverStripe\Core\Config\Config;
 use SilverStripe\View\Parsers\ShortcodeParser;
 use SilverStripe\View\Shortcodes\EmbedShortcodeProvider;
 use SilverStripe\Dev\SapphireTest;
@@ -151,4 +152,67 @@ public function testFlushCachedShortcodes()
         EmbedShortcodeProvider::flushCachedShortcodes($parser, $content);
         $this->assertFalse($cache->has($key));
     }
+
+    public function testOnlyWhitelistedAttributesAllowed()
+    {
+        $url = 'https://www.youtube.com/watch?v=dM15HfUYwF0';
+        $html = $this->getShortcodeHtml(
+            $url,
+            $url,
+            <<<EOT
+            <link rel="alternate" type="application/json+oembed" href="https://www.youtube.com/oembed?format=json&amp;url=https%3A%2F%2Fwww.youtube.com%2Fwatch%3Fv%3Da2tDOYkFCYo" title="The flying car completes first ever inter-city flight (Official Video)">
+            EOT,
+            <<<EOT
+            {"title":"The flying car completes first ever inter-city flight (Official Video)","author_name":"KleinVision","author_url":"https://www.youtube.com/channel/UCCHAHvcO7KSNmgXVRIJLNkw","type":"video","height":113,"width":200,"version":"1.0","provider_name":"YouTube","provider_url":"https://www.youtube.com/","thumbnail_height":360,"thumbnail_width":480,"thumbnail_url":"https://i.ytimg.com/vi/a2tDOYkFCYo/hqdefault.jpg","html":"\u003ciframe width=\u0022200\u0022 height=\u0022113\u0022 src=\u0022https://www.youtube.com/embed/a2tDOYkFCYo?feature=oembed\u0022 frameborder=\u00220\u0022 allow=\u0022accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture\u0022 allowfullscreen\u003e\u003c/iframe\u003e"}
+            EOT,
+            [
+                'url' => $url,
+                'caption' => 'A nice video',
+                'width' => 778,
+                'height' => 437,
+                'data-some-value' => 'my-data',
+                'onmouseover' => 'alert(2)',
+                'style' => 'background-color:red;',
+            ],
+        );
+        $this->assertEqualIgnoringWhitespace(
+            <<<EOT
+            <div style="width:778px;"><iframe width="778" height="437" src="https://www.youtube.com/embed/a2tDOYkFCYo?feature=oembed" frameborder="0" allow="accelerometer;autoplay;clipboard-write;encrypted-media;gyroscope;picture-in-picture" allowfullscreen></iframe><p class="caption">A nice video</p></div>
+            EOT,
+            $html
+        );
+    }
+
+    public function testWhitelistIsConfigurable()
+    {
+        // Allow new whitelisted attribute
+        Config::modify()->merge(EmbedShortcodeProvider::class, 'attribute_whitelist', ['data-some-value']);
+
+        $url = 'https://www.youtube.com/watch?v=dM15HfUYwF0';
+        $html = $this->getShortcodeHtml(
+            $url,
+            $url,
+            <<<EOT
+            <link rel="alternate" type="application/json+oembed" href="https://www.youtube.com/oembed?format=json&amp;url=https%3A%2F%2Fwww.youtube.com%2Fwatch%3Fv%3Da2tDOYkFCYo" title="The flying car completes first ever inter-city flight (Official Video)">
+            EOT,
+            <<<EOT
+            {"title":"The flying car completes first ever inter-city flight (Official Video)","author_name":"KleinVision","author_url":"https://www.youtube.com/channel/UCCHAHvcO7KSNmgXVRIJLNkw","type":"video","height":113,"width":200,"version":"1.0","provider_name":"YouTube","provider_url":"https://www.youtube.com/","thumbnail_height":360,"thumbnail_width":480,"thumbnail_url":"https://i.ytimg.com/vi/a2tDOYkFCYo/hqdefault.jpg","html":"\u003ciframe width=\u0022200\u0022 height=\u0022113\u0022 src=\u0022https://www.youtube.com/embed/a2tDOYkFCYo?feature=oembed\u0022 frameborder=\u00220\u0022 allow=\u0022accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture\u0022 allowfullscreen\u003e\u003c/iframe\u003e"}
+            EOT,
+            [
+                'url' => $url,
+                'caption' => 'A nice video',
+                'width' => 779,
+                'height' => 437,
+                'data-some-value' => 'my-data',
+                'onmouseover' => 'alert(2)',
+                'style' => 'background-color:red;',
+            ],
+        );
+        $this->assertEqualIgnoringWhitespace(
+            <<<EOT
+            <div data-some-value="my-data" style="width:779px;"><iframe width="779" height="437" src="https://www.youtube.com/embed/a2tDOYkFCYo?feature=oembed" frameborder="0" allow="accelerometer;autoplay;clipboard-write;encrypted-media;gyroscope;picture-in-picture" allowfullscreen></iframe><p class="caption">A nice video</p></div>
+            EOT,
+            $html
+        );
+    }
 }