Commit 0040b7b3 authored by Oliver Hader's avatar Oliver Hader Committed by Oliver Hader
Browse files

[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's avatarOliver Hader <oliver.hader@typo3.org>
Reviewed-by: Oliver Hader's avatarOliver Hader <oliver.hader@typo3.org>
parent 14929b98
......@@ -1183,9 +1183,10 @@ class GeneralUtility
* 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 @@ class GeneralUtility
if ($val !== '=') {
if ($valuemode) {
if ($name) {
$attributes[$name] = htmlspecialchars_decode($val, ENT_NOQUOTES);
$attributes[$name] = $decodeEntities ? htmlspecialchars_decode($val) : $val;
$name = '';
}
} else {
......
<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>
......@@ -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 @@ class TypolinkViewHelperTest extends FunctionalTestCase
$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()));
}
}
......@@ -3984,9 +3984,11 @@ class ContentObjectRenderer implements LoggerAwareInterface
$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 @@ class ContentObjectRenderer implements LoggerAwareInterface
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 @@ class ContentObjectRenderer implements LoggerAwareInterface
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 @@ class ContentObjectRenderer implements LoggerAwareInterface
// 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 @@ class ContentObjectRenderer implements LoggerAwareInterface
// 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 @@ class ContentObjectRenderer implements LoggerAwareInterface
}
// 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 @@ class ContentObjectRenderer implements LoggerAwareInterface
'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
......
......@@ -470,7 +470,8 @@ class AbstractPlugin
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'])) . ','
......
......@@ -398,7 +398,8 @@ class Indexer
}
// @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']);
}
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment