[SECURITY] Avoid XSS by correctly encoding typolink results

In order to avoid XSS through typolink, anchor text is encoded correctly to be used in a HTML context. Fallback link texts of links to pages are encoded per default in case lib.parseFunc has not been configured. Resolves: #88635 Releases: master, 9.5, 8.7 Security-Commit: 861d4b91797edeae4dd6a37ed5821d77f84f891e Security-Bulletin: TYPO3-CORE-SA-2019-022 Change-Id: If9246ed409ec581818a0f30c04316ba950dd4f86 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/62697 Tested-by: Oliver Hader <oliver.hader@typo3.org> Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
TYPO3 · Dec 17, 2019 · a35c42e · a35c42e
1 parent 966a003
commit a35c42e
Show file tree

Hide file tree

Showing 9 changed files with 382 additions and 9 deletions.
diff --git a/typo3/sysext/core/Classes/LinkHandling/UrlLinkHandler.php b/typo3/sysext/core/Classes/LinkHandling/UrlLinkHandler.php
@@ -54,6 +54,7 @@ protected function addHttpSchemeAsFallback(string $url): string
             $scheme = parse_url($url, PHP_URL_SCHEME);
             if (empty($scheme)) {
                 $url = 'http://' . $url;
+            // 'java{TAB}script:' is parsed as empty URL scheme, thus not ending up here
             } elseif (in_array(strtolower($scheme), ['javascript', 'data'], true)) {
                 // deny using insecure scheme's like `javascript:` or `data:` as URL scheme
                 $url = '';

diff --git a/typo3/sysext/frontend/Classes/ContentObject/ContentObjectRenderer.php b/typo3/sysext/frontend/Classes/ContentObject/ContentObjectRenderer.php
@@ -5623,7 +5623,7 @@ protected function resolveMixedLinkParameter($linkText, $mixedLinkParameter, &$c
                 // Resource was not found
                 return $linkText;
             }
-        } elseif (in_array(strtolower(trim($linkHandlerKeyword)), ['javascript', 'data'], true)) {
+        } elseif (in_array(strtolower(preg_replace('#\s|[[:cntrl:]]#', '', $linkHandlerKeyword)), ['javascript', 'data'], true)) {
             // Disallow insecure scheme's like javascript: or data:
             return $linkText;
         } else {

diff --git a/typo3/sysext/frontend/Classes/Typolink/AbstractTypolinkBuilder.php b/typo3/sysext/frontend/Classes/Typolink/AbstractTypolinkBuilder.php
@@ -102,6 +102,20 @@ protected function forceAbsoluteUrl(string $url, array $configuration): string
         return $url;
     }
 
+    /**
+     * Determines whether lib.parseFunc is defined.
+     *
+     * @return bool
+     */
+    protected function isLibParseFuncDefined(): bool
+    {
+        $configuration = $this->contentObjectRenderer->mergeTSRef(
+            ['parseFunc' => '< lib.parseFunc'],
+            'parseFunc'
+        );
+        return !empty($configuration['parseFunc.']) && is_array($configuration['parseFunc.']);
+    }
+
     /**
      * Helper method to a fallback method parsing HTML out of it
      *
@@ -111,10 +125,29 @@ protected function forceAbsoluteUrl(string $url, array $configuration): string
      */
     protected function parseFallbackLinkTextIfLinkTextIsEmpty(string $originalLinkText, string $fallbackLinkText): string
     {
-        if ($originalLinkText === '') {
+        if ($originalLinkText !== '') {
+            return $originalLinkText;
+        }
+        if ($this->isLibParseFuncDefined()) {
             return $this->contentObjectRenderer->parseFunc($fallbackLinkText, ['makelinks' => 0], '< lib.parseFunc');
         }
-        return $originalLinkText;
+        // encode in case `lib.parseFunc` is not configured
+        return $this->encodeFallbackLinkTextIfLinkTextIsEmpty($originalLinkText, $fallbackLinkText);
+    }
+
+    /**
+     * Helper method to a fallback method properly encoding HTML.
+     *
+     * @param string $originalLinkText the original string, if empty, the fallback link text
+     * @param string $fallbackLinkText the string to be used.
+     * @return string the final text
+     */
+    protected function encodeFallbackLinkTextIfLinkTextIsEmpty(string $originalLinkText, string $fallbackLinkText): string
+    {
+        if ($originalLinkText !== '') {
+            return $originalLinkText;
+        }
+        return htmlspecialchars($fallbackLinkText, ENT_QUOTES);
     }
 
     /**

diff --git a/typo3/sysext/frontend/Classes/Typolink/ExternalUrlLinkBuilder.php b/typo3/sysext/frontend/Classes/Typolink/ExternalUrlLinkBuilder.php
@@ -28,7 +28,7 @@ public function build(array &$linkDetails, string $linkText, string $target, arr
     {
         return [
             $this->processUrl(UrlProcessorInterface::CONTEXT_EXTERNAL, htmlspecialchars_decode($linkDetails['url']), $conf),
-            $this->parseFallbackLinkTextIfLinkTextIsEmpty($linkText, $linkDetails['url']),
+            $this->encodeFallbackLinkTextIfLinkTextIsEmpty($linkText, $linkDetails['url']),
             $target ?: $this->resolveTargetAttribute($conf, 'extTarget', true, $this->getTypoScriptFrontendController()->extTarget)
         ];
     }

diff --git a/typo3/sysext/frontend/Classes/Typolink/FileOrFolderLinkBuilder.php b/typo3/sysext/frontend/Classes/Typolink/FileOrFolderLinkBuilder.php
@@ -47,7 +47,7 @@ public function build(array &$linkDetails, string $linkText, string $target, arr
             $linkLocation = '';
         }
         // Setting title if blank value to link
-        $linkText = $this->parseFallbackLinkTextIfLinkTextIsEmpty($linkText, rawurldecode($linkLocation));
+        $linkText = $this->encodeFallbackLinkTextIfLinkTextIsEmpty($linkText, rawurldecode($linkLocation));
         if (strpos($linkLocation, '/') !== 0
             && parse_url($linkLocation, PHP_URL_SCHEME) === null
         ) {

diff --git a/typo3/sysext/frontend/Classes/Typolink/LegacyLinkBuilder.php b/typo3/sysext/frontend/Classes/Typolink/LegacyLinkBuilder.php
@@ -33,15 +33,15 @@ public function build(array &$linkDetails, string $linkText, string $target, arr
             $linkDetails['type'] = LinkService::TYPE_FILE;
             $linkLocation = $linkDetails['file'];
             // Setting title if blank value to link
-            $linkText = $this->parseFallbackLinkTextIfLinkTextIsEmpty($linkText, rawurldecode($linkLocation));
+            $linkText = $this->encodeFallbackLinkTextIfLinkTextIsEmpty($linkText, rawurldecode($linkLocation));
             $linkLocation = (strpos($linkLocation, '/') !== 0 ? $tsfe->absRefPrefix : '') . $linkLocation;
             $url = $this->processUrl(UrlProcessorInterface::CONTEXT_FILE, $linkLocation, $conf);
             $url = $this->forceAbsoluteUrl($url, $conf);
             $target = $target ?: $this->resolveTargetAttribute($conf, 'fileTarget', false, $tsfe->fileTarget);
         } elseif ($linkDetails['url']) {
             $linkDetails['type'] = LinkService::TYPE_URL;
             $target = $target ?: $this->resolveTargetAttribute($conf, 'extTarget', true, $tsfe->extTarget);
-            $linkText = $this->parseFallbackLinkTextIfLinkTextIsEmpty($linkText, $linkDetails['url']);
+            $linkText = $this->encodeFallbackLinkTextIfLinkTextIsEmpty($linkText, $linkDetails['url']);
             $url = $this->processUrl(UrlProcessorInterface::CONTEXT_EXTERNAL, $linkDetails['url'], $conf);
         } else {
             throw new UnableToLinkException('Unknown link detected, so ' . $linkText . ' was not linked.', 1490990031, null, $linkText);

diff --git a/...frontend/Tests/Functional/SiteHandling/Fixtures/TypoLinkGenerator.libParseFunc.typoscript b/...frontend/Tests/Functional/SiteHandling/Fixtures/TypoLinkGenerator.libParseFunc.typoscript
@@ -0,0 +1,23 @@
+lib.parseFunc {
+  makelinks = 1
+  makelinks {
+    http {
+      keep = path
+      extTarget = _blank
+    }
+    mailto {
+      keep = path
+    }
+  }
+  allowTags = good
+  denyTags = *
+  constants = 1
+  nonTypoTagStdWrap {
+    HTMLparser = 1
+    HTMLparser {
+      tags.good.allowedAttribs = 0
+      keepNonMatchedTags = 1
+      htmlSpecialChars = 2
+    }
+  }
+}
diff --git a/typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/TypoLinkScenario.xml b/typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/TypoLinkScenario.xml
@@ -38,6 +38,61 @@
         <shortcut_mode>0</shortcut_mode>
         <title>EN: Features</title>
     </pages>
+    <pages>
+        <uid>9911</uid>
+        <pid>1000</pid>
+        <deleted>0</deleted>
+        <hidden>0</hidden>
+        <doktype>0</doktype>
+        <is_siteroot>0</is_siteroot>
+        <shortcut>0</shortcut>
+        <shortcut_mode>0</shortcut_mode>
+        <title><![CDATA[<good>]]></title>
+    </pages>
+    <pages>
+        <uid>9912</uid>
+        <pid>1000</pid>
+        <deleted>0</deleted>
+        <hidden>0</hidden>
+        <doktype>0</doktype>
+        <is_siteroot>0</is_siteroot>
+        <shortcut>0</shortcut>
+        <shortcut_mode>0</shortcut_mode>
+        <title><![CDATA[<good a="a/" b="thing(1)">]]></title>
+    </pages>
+    <pages>
+        <uid>9913</uid>
+        <pid>1000</pid>
+        <deleted>0</deleted>
+        <hidden>0</hidden>
+        <doktype>0</doktype>
+        <is_siteroot>0</is_siteroot>
+        <shortcut>0</shortcut>
+        <shortcut_mode>0</shortcut_mode>
+        <title><![CDATA[<good%20a="a/"%20b="thing(1)">]]></title>
+    </pages>
+    <pages>
+        <uid>9914</uid>
+        <pid>1000</pid>
+        <deleted>0</deleted>
+        <hidden>0</hidden>
+        <doktype>0</doktype>
+        <is_siteroot>0</is_siteroot>
+        <shortcut>0</shortcut>
+        <shortcut_mode>0</shortcut_mode>
+        <title><![CDATA[<good/a="a/"/b="thing(1)">]]></title>
+    </pages>
+    <pages>
+        <uid>9921</uid>
+        <pid>1000</pid>
+        <deleted>0</deleted>
+        <hidden>0</hidden>
+        <doktype>0</doktype>
+        <is_siteroot>0</is_siteroot>
+        <shortcut>0</shortcut>
+        <shortcut_mode>0</shortcut_mode>
+        <title><![CDATA[<bad>]]></title>
+    </pages>
     <tt_content>
         <uid>10001</uid>
         <pid>1100</pid>