From a35c42e9bcb020e16016d1c146354513a9856bc0 Mon Sep 17 00:00:00 2001 From: Oliver Hader Date: Tue, 17 Dec 2019 10:50:30 +0100 Subject: [PATCH] [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 Reviewed-by: Oliver Hader --- .../Classes/LinkHandling/UrlLinkHandler.php | 1 + .../ContentObject/ContentObjectRenderer.php | 2 +- .../Typolink/AbstractTypolinkBuilder.php | 37 ++- .../Typolink/ExternalUrlLinkBuilder.php | 2 +- .../Typolink/FileOrFolderLinkBuilder.php | 2 +- .../Classes/Typolink/LegacyLinkBuilder.php | 4 +- .../TypoLinkGenerator.libParseFunc.typoscript | 23 ++ .../Fixtures/TypoLinkScenario.xml | 55 ++++ .../SiteHandling/TypoLinkGeneratorTest.php | 265 +++++++++++++++++- 9 files changed, 382 insertions(+), 9 deletions(-) create mode 100644 typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/TypoLinkGenerator.libParseFunc.typoscript diff --git a/typo3/sysext/core/Classes/LinkHandling/UrlLinkHandler.php b/typo3/sysext/core/Classes/LinkHandling/UrlLinkHandler.php index aa9e05cbc112..f8008fcbdfcb 100644 --- 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 index 2b56c7958ce4..2eff67239dd8 100644 --- 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 index 2695f3ef90f9..0ff4237e5928 100644 --- 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 index efa447455e16..284566b30802 100644 --- 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 index 1c1c8d1f584e..b9a9da29373b 100644 --- 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 index 12c2b1a622bb..c3dcfd300a5d 100644 --- a/typo3/sysext/frontend/Classes/Typolink/LegacyLinkBuilder.php +++ b/typo3/sysext/frontend/Classes/Typolink/LegacyLinkBuilder.php @@ -33,7 +33,7 @@ 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); @@ -41,7 +41,7 @@ public function build(array &$linkDetails, string $linkText, string $target, arr } 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/typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/TypoLinkGenerator.libParseFunc.typoscript b/typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/TypoLinkGenerator.libParseFunc.typoscript new file mode 100644 index 000000000000..b20df026e4e3 --- /dev/null +++ b/typo3/sysext/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 index 8c9d63e8bf73..378d91fcf478 100644 --- a/typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/TypoLinkScenario.xml +++ b/typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/TypoLinkScenario.xml @@ -38,6 +38,61 @@ 0 EN: Features + + 9911 + 1000 + 0 + 0 + 0 + 0 + 0 + 0 + <![CDATA[<good>]]> + + + 9912 + 1000 + 0 + 0 + 0 + 0 + 0 + 0 + <![CDATA[<good a="a/" b="thing(1)">]]> + + + 9913 + 1000 + 0 + 0 + 0 + 0 + 0 + 0 + <![CDATA[<good%20a="a/"%20b="thing(1)">]]> + + + 9914 + 1000 + 0 + 0 + 0 + 0 + 0 + 0 + <![CDATA[<good/a="a/"/b="thing(1)">]]> + + + 9921 + 1000 + 0 + 0 + 0 + 0 + 0 + 0 + <![CDATA[<bad>]]> + 10001 1100 diff --git a/typo3/sysext/frontend/Tests/Functional/SiteHandling/TypoLinkGeneratorTest.php b/typo3/sysext/frontend/Tests/Functional/SiteHandling/TypoLinkGeneratorTest.php index 3a461c9a174a..329a0d2dc237 100644 --- a/typo3/sysext/frontend/Tests/Functional/SiteHandling/TypoLinkGeneratorTest.php +++ b/typo3/sysext/frontend/Tests/Functional/SiteHandling/TypoLinkGeneratorTest.php @@ -88,6 +88,10 @@ public function linkIsGeneratedDataProvider(): array 't3://email?email=user@example.org&other=other#other', 'user@example.org', ], + [ + 't3://email?email=user@example.org?subject=Hello%20World#other', + 'user@example.org?subject=Hello World', + ], [ 't3://file?uid=1&type=1&other=other#other', 'fileadmin/logo.png', @@ -113,8 +117,16 @@ public function linkIsGeneratedDataProvider(): array 'EN: Features', ], [ - 't3://url?url=https://typo3.org&other=other#other', - 'https://typo3.org', + 't3://url?url=https://typo3.org%3f%26param-a=a%26param-b=b&other=other#other', + 'https://typo3.org?&param-a=a&param-b=b', + ], + [ + '1200,1 target class title ¶m-a=a', + 'EN: Features' + ], + [ + 'user@example.org target class title &other=other', + 'user@example.org' ], ]; return $this->keysFromTemplate($instructions, '%1$s;'); @@ -134,6 +146,255 @@ public function linkIsGenerated(string $parameter, string $expectation) static::assertSame($expectation, $response->getContent()); } + /** + * @return array + */ + public function linkIsEncodedDataProvider(): array + { + $instructions = [ + [ + 't3://email?email=mailto:thing(1)', + '<bad>thing(1)</bad>', + ], + [ + 't3://email?email=mailto:', + '<good a="a/" b="thing(1)">', + ], + [ + 't3://email?email=thing(1)', + '<bad>thing(1)</bad>', + ], + [ + 't3://email?email=', + '<good a="a/" b="thing(1)">', + ], + [ + 't3://folder?identifier=', + '', + ], + [ + 't3://page?uid=', + '', + ], + [ + 't3://record?identifier=content&uid=', + '', + ], + [ + 't3://url?url=thing(1)', + 'http://<bad>thing(1)</bad>' + ], + [ + 't3://url?url=', + 'http://<good a="a/" b="thing(1)">' + ], + [ + 't3://url?url=javascript:good()', + '' + ], + [ + "t3://url?url=java\tscript:good()", + 'http://java_script:good()' + ], + [ + 't3://url?url=java script:good()', + 'http://java' + ], + [ + 't3://url?url=javascript:good()', + 'http://javascript' + ], + [ + 't3://url?url=data:text/html,', + '' + ], + [ + "t3://url?url=da\tsta:text/html,", + 'http://da_sta:text/html,<good>' + ], + [ + 't3://url?url=da ta:text/html,', + 'http://da' + ], + [ + 't3://url?url=data:text/html,', + 'http://data' + ], + [ + 't3://url?url=%26%23106%3B%26%2397%3B%26%23118%3B%26%2397%3B%26%23115%3B%26%2399%3B%26%23114%3B%26%23105%3B%26%23112%3B%26%23116%3B%26%2358%3B%26%23103%3B%26%23111%3B%26%23111%3B%26%23100%3B%26%2340%3B%26%2341%3B', + 'http://&#106;&#97;&#118;&#97;&#115;&#99;&#114;&#105;&#112;&#116;&#58;&#103;&#111;&#111;&#100;&#40;&#41;', + ], + [ + 'thing(1)', + '<bad>thing(1)</bad>' + ], + [ + '', + '<good a="a/" b="thing(1)">' + ], + [ + ' target class title &other=other', + '<good/a="a/"/b="thing(1)">' + ], + [ + 'javascript:good()', + '', + ], + [ + "java\tscript:good()", + '', + ], + [ + 'java script:good()', + '' + ], + [ + 'javascript:good()', + '' + ], + [ + 'data:text/html,', + '', + ], + [ + "da\tta:text/html,", + '', + ], + [ + 'da ta:text/html,', + '', + ], + [ + 'data:text/html,', + 'data&colon;text/html,<good>', + ], + [ + '%26%23106%3B%26%2397%3B%26%23118%3B%26%2397%3B%26%23115%3B%26%2399%3B%26%23114%3B%26%23105%3B%26%23112%3B%26%23116%3B%26%2358%3B%26%23103%3B%26%23111%3B%26%23111%3B%26%23100%3B%26%2340%3B%26%2341%3B', + '', + ], + [ + ' <"> <"> <">', + '</>', + ], + ]; + return $this->keysFromTemplate($instructions, '%1$s;'); + } + + /** + * @param string $parameter + * @param string $expectation + * + * @test + * @dataProvider linkIsEncodedDataProvider + */ + public function linkIsEncodedPerDefault(string $parameter, string $expectation) + { + $this->assignTypoScriptConstant('typolink.parameter', $parameter, 1000); + $response = $this->getFrontendResponse(1100); + static::assertSame($expectation, $response->getContent()); + } + + /** + * @param string $parameter + * @param string $expectation + * + * @test + * @dataProvider linkIsEncodedDataProvider + */ + public function linkIsEncodedHavingParseFunc(string $parameter, string $expectation) + { + $this->setUpFrontendRootPage( + 1000, + [ + 'typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/TypoLinkGenerator.typoscript', + 'typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/TypoLinkGenerator.libParseFunc.typoscript', + ] + ); + $this->assignTypoScriptConstant('typolink.parameter', $parameter, 1000); + $response = $this->getFrontendResponse(1100); + static::assertSame($expectation, $response->getContent()); + } + + /** + * @return array + */ + public function linkToPageIsProcessedDataProvider(): array + { + return [ + [ + 't3://page?uid=9911', + '<good>', + false, + ], + [ + 't3://page?uid=9911', + '', + true, + ], + [ + 't3://page?uid=9912', + '<good a="a/" b="thing(1)">', + false, + ], + [ + 't3://page?uid=9912', + '', + true, + ], + [ + 't3://page?uid=9913', + '<good%20a="a/"%20b="thing(1)">', + false, + ], + [ + 't3://page?uid=9913', + '<good%20a="a/"%20b="thing(1)">', + true, + ], + [ + 't3://page?uid=9914', + '<good/a="a/"/b="thing(1)">', + false, + ], + [ + 't3://page?uid=9914', + '<good/a="a/"/b="thing(1)">', + true, + ], + [ + 't3://page?uid=9921', + '<bad>', + false, + ], + [ + 't3://page?uid=9921', + '<bad>', + true, + ], + ]; + } + + /** + * @param string $parameter + * @param string $expectation + * @param bool $parseFuncEnabled + * + * @test + * @dataProvider linkToPageIsProcessedDataProvider + */ + public function linkToPageIsProcessed(string $parameter, string $expectation, bool $parseFuncEnabled) + { + $typoScriptFiles = ['typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/TypoLinkGenerator.typoscript']; + if ($parseFuncEnabled) { + $typoScriptFiles[] = 'typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/TypoLinkGenerator.libParseFunc.typoscript'; + } + + $this->setUpFrontendRootPage(1000, $typoScriptFiles); + $this->assignTypoScriptConstant('typolink.parameter', $parameter, 1000); + $response = $this->getFrontendResponse(1100); + static::assertSame($expectation, $response->getContent()); + } + /** * @param string $name * @param string $value