[SECURITY] Ensure decoded entities are encoded for HTML again

HTML entities being used in link tags created with `typolink` have to be encoded correctly again after entities have been decoded for internal processing. Resolves: #91161 Releases: master, 9.5 Change-Id: Ifc4d2da669aab01f2b3041bb32c0a24a727634b4 Security-Bulletin: TYPO3-CORE-SA-2020-003 Security-References: CVE-2020-11065 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/64467 Tested-by: Oliver Hader <oliver.hader@typo3.org> Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
TYPO3 · May 12, 2020 · 0040b7b · 0040b7b
1 parent 14929b9
commit 0040b7b
Show file tree

Hide file tree

Showing 6 changed files with 65 additions and 13 deletions.
diff --git a/typo3/sysext/core/Classes/Utility/GeneralUtility.php b/typo3/sysext/core/Classes/Utility/GeneralUtility.php
@@ -1183,9 +1183,10 @@ public static function removeDotsFromTS(array $ts)
      * If an attribute is empty, then the value for the key is empty. You can check if it existed with isset()
      *
      * @param string $tag HTML-tag string (or attributes only)
+     * @param bool $decodeEntities Whether to decode HTML entities
      * @return string[] Array with the attribute values.
      */
-    public static function get_tag_attributes($tag)
+    public static function get_tag_attributes($tag, bool $decodeEntities = false)
     {
         $components = self::split_tag_attributes($tag);
         // Attribute name is stored here
@@ -1197,7 +1198,7 @@ public static function get_tag_attributes($tag)
             if ($val !== '=') {
                 if ($valuemode) {
                     if ($name) {
-                        $attributes[$name] = htmlspecialchars_decode($val, ENT_NOQUOTES);
+                        $attributes[$name] = $decodeEntities ? htmlspecialchars_decode($val) : $val;
                         $name = '';
                     }
                 } else {

diff --git a/...ysext/fluid/Tests/Functional/ViewHelpers/Fixtures/link_typolink_additionalAttributes.html b/...ysext/fluid/Tests/Functional/ViewHelpers/Fixtures/link_typolink_additionalAttributes.html
@@ -0,0 +1,6 @@
+<html xmlns:f="http://typo3.org/ns/TYPO3/CMS/Fluid/ViewHelpers" data-namespace-typo3-fluid="true">
+
+<f:link.typolink parameter="{parameter}"
+                 additionalAttributes="{additionalAttributes}">Link Text</f:link.typolink>
+
+</html>
diff --git a/typo3/sysext/fluid/Tests/Functional/ViewHelpers/TypolinkViewHelperTest.php b/typo3/sysext/fluid/Tests/Functional/ViewHelpers/TypolinkViewHelperTest.php
@@ -26,6 +26,8 @@ class TypolinkViewHelperTest extends FunctionalTestCase
 {
     use SiteBasedTestTrait;
 
+    private const TEMPLATE_BASE_PATH = 'typo3/sysext/fluid/Tests/Functional/ViewHelpers/Fixtures/';
+
     /**
      * @var array
      */
@@ -374,4 +376,41 @@ public function typoLinkPartsAreRendered(string $parameter, string $expectation)
         $view->assignMultiple(['parameter' => $parameter]);
         self::assertSame($expectation, trim($view->render()));
     }
+
+    public function typoLinkAdditionalAttributesAreRenderedDataProvider(): array
+    {
+        return [
+            [
+                [
+                    'parameter' => 'http://typo3.org/ "_self" "<CSS>" "<Title>"',
+                    'additionalAttributes' => [
+                        'data-html' => '<div data-template="template">'
+                            . '<img src="logo.png" alt="&quot;&lt;ALT&gt;&quot;"></div>',
+                        'data-other' => '\'\'',
+                    ],
+                ],
+                '<a href="http://typo3.org/" title="&lt;Title&gt;" target="_self"'
+                    . ' class="&lt;CSS&gt;" data-html="&lt;div data-template=&quot;template&quot;&gt;'
+                    . '&lt;img src=&quot;logo.png&quot; alt=&quot;&amp;quot;&amp;lt;ALT&amp;gt;&amp;quot;&quot;&gt;&lt;/div&gt;"'
+                    . ' data-other="\'\'">Link Text</a>'
+            ]
+        ];
+    }
+
+    /**
+     * @param array $configuration
+     * @param string $expectation
+     *
+     * @test
+     * @dataProvider typoLinkAdditionalAttributesAreRenderedDataProvider
+     */
+    public function typoLinkAdditionalAttributesAreRendered(array $configuration, string $expectation): void
+    {
+        $view = new StandaloneView();
+        $view->setTemplatePathAndFilename(
+            self::TEMPLATE_BASE_PATH . 'link_typolink_additionalAttributes.html'
+        );
+        $view->assignMultiple($configuration);
+        self::assertSame($expectation, trim($view->render()));
+    }
 }
diff --git a/typo3/sysext/frontend/Classes/ContentObject/ContentObjectRenderer.php b/typo3/sysext/frontend/Classes/ContentObject/ContentObjectRenderer.php
@@ -3984,9 +3984,11 @@ public function _parseFunc($theValue, $conf)
                         $breakOut = (bool)($theConf['breakoutTypoTagContent'] ?? false);
                         $this->parameters = [];
                         if (isset($currentTag[1])) {
-                            $params = GeneralUtility::get_tag_attributes($currentTag[1]);
+                            // decode HTML entities in attributes, since they're processed
+                            $params = GeneralUtility::get_tag_attributes($currentTag[1], true);
                             if (is_array($params)) {
                                 foreach ($params as $option => $val) {
+                                    // contains non-encoded values
                                     $this->parameters[strtolower($option)] = $val;
                                 }
                             }
@@ -4102,11 +4104,13 @@ public function encaps_lineSplit($theValue, $conf)
                     if (substr($fwParts[0], -1) === '/') {
                         $sameBeginEnd = 1;
                         $emptyTag = true;
-                        $attrib = GeneralUtility::get_tag_attributes('<' . substr($fwParts[0], 0, -1) . '>');
+                        // decode HTML entities, they're encoded later again
+                        $attrib = GeneralUtility::get_tag_attributes('<' . substr($fwParts[0], 0, -1) . '>', true);
                     }
                 } else {
                     $backParts = GeneralUtility::revExplode('<', substr($fwParts[1], 0, -1), 2);
-                    $attrib = GeneralUtility::get_tag_attributes('<' . $fwParts[0] . '>');
+                    // decode HTML entities, they're encoded later again
+                    $attrib = GeneralUtility::get_tag_attributes('<' . $fwParts[0] . '>', true);
                     $str_content = $backParts[0];
                     $sameBeginEnd = substr(strtolower($backParts[1]), 1, strlen($tagName)) === strtolower($tagName);
                 }
@@ -4153,6 +4157,7 @@ public function encaps_lineSplit($theValue, $conf)
                 if ((!isset($attrib['align']) || !$attrib['align']) && $defaultAlign) {
                     $attrib['align'] = $defaultAlign;
                 }
+                // implode (insecure) attributes, that's why `htmlspecialchars` is used here
                 $params = GeneralUtility::implodeAttributes($attrib, true);
                 if (!isset($conf['removeWrapping']) || !$conf['removeWrapping'] || ($emptyTag && $conf['removeWrapping.']['keepSingleTag'])) {
                     $selfClosingTagList = ['area', 'base', 'br', 'col', 'embed', 'hr', 'img', 'input', 'keygen', 'link', 'meta', 'param', 'source', 'track', 'wbr'];
@@ -5082,10 +5087,10 @@ public function typoLink($linkText, $conf)
 
         // Ensure "href" is not in the list of aTagParams to avoid double tags, usually happens within buggy parseFunc settings
         if (!empty($finalTagParts['aTagParams'])) {
-            $aTagParams = GeneralUtility::get_tag_attributes($finalTagParts['aTagParams']);
+            $aTagParams = GeneralUtility::get_tag_attributes($finalTagParts['aTagParams'], true);
             if (isset($aTagParams['href'])) {
                 unset($aTagParams['href']);
-                $finalTagParts['aTagParams'] = GeneralUtility::implodeAttributes($aTagParams);
+                $finalTagParts['aTagParams'] = GeneralUtility::implodeAttributes($aTagParams, true);
             }
         }
 
@@ -5123,6 +5128,7 @@ public function typoLink($linkText, $conf)
             // Imploding into string:
             $JSwindowParams = implode(',', $JSwindow_paramsArr);
         }
+
         if (!$JSwindowParams && $linkDetails['type'] === LinkService::TYPE_EMAIL && $tsfe->spamProtectEmailAddresses === 'ascii') {
             $tagAttributes['href'] = $finalTagParts['url'];
         } else {
@@ -5154,13 +5160,11 @@ public function typoLink($linkText, $conf)
         }
 
         // Prevent trouble with double and missing spaces between attributes and merge params before implode
+        // (skip decoding HTML entities, since `$tagAttributes` are expected to be encoded already)
         $finalTagAttributes = array_merge($tagAttributes, GeneralUtility::get_tag_attributes($finalTagParts['aTagParams']));
         $finalTagAttributes = $this->addSecurityRelValues($finalTagAttributes, $target, $tagAttributes['href']);
         $finalAnchorTag = '<a ' . GeneralUtility::implodeAttributes($finalTagAttributes) . '>';
 
-        if (!empty($finalTagParts['aTagParams'])) {
-            $tagAttributes = array_merge($tagAttributes, GeneralUtility::get_tag_attributes($finalTagParts['aTagParams']));
-        }
         // kept for backwards-compatibility in hooks
         $finalTagParts['targetParams'] = !empty($tagAttributes['target']) ? ' target="' . $tagAttributes['target'] . '"' : '';
         $this->lastTypoLinkTarget = $target;
@@ -5178,7 +5182,7 @@ public function typoLink($linkText, $conf)
             'finalTag' => &$finalAnchorTag,
             'finalTagParts' => &$finalTagParts,
             'linkDetails' => &$linkDetails,
-            'tagAttributes' => &$tagAttributes
+            'tagAttributes' => &$finalTagAttributes
         ];
         foreach ($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['tslib/class.tslib_content.php']['typoLink_PostProc'] ?? [] as $_funcRef) {
             $ref = $this; // introduced for phpstan to not lose type information when passing $this into callUserFunction

diff --git a/typo3/sysext/frontend/Classes/Plugin/AbstractPlugin.php b/typo3/sysext/frontend/Classes/Plugin/AbstractPlugin.php
@@ -470,7 +470,8 @@ public function pi_list_linkSingle($str, $uid, $cache = false, $mergeArr = [], $
     public function pi_openAtagHrefInJSwindow($str, $winName = '', $winParams = 'width=670,height=500,status=0,menubar=0,scrollbars=1,resizable=1')
     {
         if (preg_match('/(.*)(<a[^>]*>)(.*)/i', $str, $match)) {
-            $aTagContent = GeneralUtility::get_tag_attributes($match[2]);
+            // decode HTML entities, `href` is used in escaped JavaScript context
+            $aTagContent = GeneralUtility::get_tag_attributes($match[2], true);
             $onClick = 'vHWin=window.open('
                 . GeneralUtility::quoteJSvalue($this->frontendController->baseUrlWrap($aTagContent['href'])) . ','
                 . GeneralUtility::quoteJSvalue($winName ?: md5($aTagContent['href'])) . ','

diff --git a/typo3/sysext/indexed_search/Classes/Indexer.php b/typo3/sysext/indexed_search/Classes/Indexer.php
@@ -398,7 +398,8 @@ public function splitHTMLContent($content)
             }
             // @todo The code below stops at first unset tag. Is that correct?
             for ($i = 0; isset($meta[$i]); $i++) {
-                $meta[$i] = GeneralUtility::get_tag_attributes($meta[$i]);
+                // decode HTML entities, meta tag content needs to be encoded later
+                $meta[$i] = GeneralUtility::get_tag_attributes($meta[$i], true);
                 if (stripos($meta[$i]['name'], 'keywords') !== false) {
                     $contentArr['keywords'] .= ',' . $this->addSpacesToKeywordList($meta[$i]['content']);
                 }