Skip to content

Commit

Permalink
[SECURITY] Introduce selective argument escaping
Browse files Browse the repository at this point in the history 
Addresses three XSS vulnerabilities:

* The "then" and "else" arguments of condition ViewHelpers
  were not escaped. They are now escaped based on the
  escapeChildren toggle of the ViewHelper, which is ON by
  default in subclasses of AbstractConditionViewHelper.
* Content arguments in ViewHelpers which disable
  escapeOutput were not escaped, but values passed as
  child node were escaped. Both cases are now treated
  the same and escaping is based on escapeChildren state.
* TagBased ViewHelpers allowed attribute names containing
  HTML if passed in "additionalAttributes" which made XSS
  possible by crafting array keys with HTML. Attribute names
  are now subjected to the same escaping as attribute values.

Also fixes a couple of undesirable behaviors as well, e.g. avoids
double escaping of output in some combinations of escapeOutput=true
and quoted arguments.
  • Loading branch information
@NamelessCoder
NamelessCoder committed Nov 16, 2020
1 parent f5c4593 commit f20db4e
Show file tree
Hide file tree
Showing 12 changed files with 275 additions and 68 deletions.
20 changes: 20 additions & 0 deletions src/Core/Parser/Configuration.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
*/
class Configuration
{
/**
* @var bool
*/
protected $viewHelperArgumentEscapingEnabled = true;

/**
* Generic interceptors registered with the configuration.
Expand All @@ -27,6 +31,22 @@ class Configuration
*/
protected $escapingInterceptors = [];

/**
* @return bool
*/
public function isViewHelperArgumentEscapingEnabled()
{
return $this->viewHelperArgumentEscapingEnabled;
}

/**
* @param bool $viewHelperArgumentEscapingEnabled
*/
public function setViewHelperArgumentEscapingEnabled($viewHelperArgumentEscapingEnabled): void
{
$this->viewHelperArgumentEscapingEnabled = (bool) $viewHelperArgumentEscapingEnabled;
}

/**
* Adds an interceptor to apply to values coming from object accessors.
*
Expand Down
53 changes: 41 additions & 12 deletions src/Core/Parser/TemplateParser.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ protected function objectAccessorHandler(ParsingState $state, $objectAccessorStr
}
$viewHelper = $viewHelperResolver->createViewHelperInstance($singleMatch['NamespaceIdentifier'], $singleMatch['MethodIdentifier']);
if (strlen($singleMatch['ViewHelperArguments']) > 0) {
$arguments = $this->recursiveArrayHandler($singleMatch['ViewHelperArguments'], $viewHelper);
$arguments = $this->recursiveArrayHandler($state, $singleMatch['ViewHelperArguments'], $viewHelper);
} else {
$arguments = [];
}
Expand Down Expand Up @@ -563,28 +563,43 @@ protected function parseArguments($argumentsString, ViewHelperInterface $viewHel
$undeclaredArguments = [];
$matches = [];
if (preg_match_all(Patterns::$SPLIT_PATTERN_TAGARGUMENTS, $argumentsString, $matches, PREG_SET_ORDER) > 0) {
$escapingEnabledBackup = $this->escapingEnabled;
$this->escapingEnabled = false;
foreach ($matches as $singleMatch) {
$argument = $singleMatch['Argument'];
$value = $this->unquoteString($singleMatch['ValueQuoted']);
$argumentsObjectTree[$argument] = $this->buildArgumentObjectTree($value);
$escapingEnabledBackup = $this->escapingEnabled;
if (isset($argumentDefinitions[$argument])) {
$argumentDefinition = $argumentDefinitions[$argument];
if ($argumentDefinition->getType() === 'boolean' || $argumentDefinition->getType() === 'bool') {
$this->escapingEnabled = $this->escapingEnabled && $this->isArgumentEscaped($viewHelper, $argumentDefinition);
$isBoolean = $argumentDefinition->getType() === 'boolean' || $argumentDefinition->getType() === 'bool';
$argumentsObjectTree[$argument] = $this->buildArgumentObjectTree($value);
if ($isBoolean) {
$argumentsObjectTree[$argument] = new BooleanNode($argumentsObjectTree[$argument]);
}
} else {
$undeclaredArguments[$argument] = $argumentsObjectTree[$argument];
$this->escapingEnabled = false;
$undeclaredArguments[$argument] = $this->buildArgumentObjectTree($value);
}
$this->escapingEnabled = $escapingEnabledBackup;
}
$this->escapingEnabled = $escapingEnabledBackup;
}
$this->abortIfRequiredArgumentsAreMissing($argumentDefinitions, $argumentsObjectTree);
$viewHelper->validateAdditionalArguments($undeclaredArguments);
return $argumentsObjectTree + $undeclaredArguments;
}

protected function isArgumentEscaped(ViewHelperInterface $viewHelper, ArgumentDefinition $argumentDefinition = null)
{
$hasDefinition = $argumentDefinition instanceof ArgumentDefinition;
$isBoolean = $hasDefinition && ($argumentDefinition->getType() === 'boolean' || $argumentDefinition->getType() === 'bool');
$escapingEnabled = $this->configuration->isViewHelperArgumentEscapingEnabled();
$isArgumentEscaped = $hasDefinition && $argumentDefinition->getEscape() === true;
$isContentArgument = $hasDefinition && method_exists($viewHelper, 'resolveContentArgumentName') && $argumentDefinition->getName() === $viewHelper->resolveContentArgumentName();
if ($isContentArgument) {
return !$isBoolean && ($viewHelper->isChildrenEscapingEnabled() || $isArgumentEscaped);
}
return !$isBoolean && $escapingEnabled && $isArgumentEscaped;
}

/**
* Build up an argument object tree for the string in $argumentString.
* This builds up the tree for a single argument value.
Expand Down Expand Up @@ -664,7 +679,7 @@ protected function textAndShorthandSyntaxHandler(ParsingState $state, $text, $co
&& preg_match(Patterns::$SCAN_PATTERN_SHORTHANDSYNTAX_ARRAYS, $section, $matchedVariables) > 0
) {
// We only match arrays if we are INSIDE viewhelper arguments
$this->arrayHandler($state, $this->recursiveArrayHandler($matchedVariables['Array']));
$this->arrayHandler($state, $this->recursiveArrayHandler($state, $matchedVariables['Array']));
} else {
// We ask custom ExpressionNode instances from ViewHelperResolver
// if any match our expression:
Expand Down Expand Up @@ -737,12 +752,13 @@ protected function arrayHandler(ParsingState $state, $arrayText)
* - Variables
* - sub-arrays
*
* @param ParsingState $state
* @param string $arrayText Array text
* @param ViewHelperInterface|null $viewHelper ViewHelper instance - passed only if the array is a collection of arguments for an inline ViewHelper
* @return NodeInterface[] the array node built up
* @throws Exception
*/
protected function recursiveArrayHandler($arrayText, ViewHelperInterface $viewHelper = null)
protected function recursiveArrayHandler(ParsingState $state, $arrayText, ViewHelperInterface $viewHelper = null)
{
$undeclaredArguments = [];
$argumentDefinitions = [];
Expand All @@ -755,14 +771,25 @@ protected function recursiveArrayHandler($arrayText, ViewHelperInterface $viewHe
foreach ($matches as $singleMatch) {
$arrayKey = $this->unquoteString($singleMatch['Key']);
$assignInto = &$arrayToBuild;
if (!isset($argumentDefinitions[$arrayKey])) {
$isBoolean = false;
$argumentDefinition = null;
if (isset($argumentDefinitions[$arrayKey])) {
$argumentDefinition = $argumentDefinitions[$arrayKey];
$isBoolean = $argumentDefinitions[$arrayKey]->getType() === 'boolean' || $argumentDefinitions[$arrayKey]->getType() === 'bool';
} else {
$assignInto = &$undeclaredArguments;
}

$escapingEnabledBackup = $this->escapingEnabled;
$this->escapingEnabled = $this->escapingEnabled && $viewHelper instanceof ViewHelperInterface && $this->isArgumentEscaped($viewHelper, $argumentDefinition);

if (array_key_exists('Subarray', $singleMatch) && !empty($singleMatch['Subarray'])) {
$assignInto[$arrayKey] = new ArrayNode($this->recursiveArrayHandler($singleMatch['Subarray']));
$assignInto[$arrayKey] = new ArrayNode($this->recursiveArrayHandler($state, $singleMatch['Subarray']));
} elseif (!empty($singleMatch['VariableIdentifier'])) {
$assignInto[$arrayKey] = new ObjectAccessorNode($singleMatch['VariableIdentifier']);
if ($viewHelper instanceof ViewHelperInterface && !$isBoolean) {
$this->callInterceptor($assignInto[$arrayKey], InterceptorInterface::INTERCEPT_OBJECTACCESSOR, $state);
}
} elseif (array_key_exists('Number', $singleMatch) && (!empty($singleMatch['Number']) || $singleMatch['Number'] === '0')) {
// Note: this method of casting picks "int" when value is a natural number and "float" if any decimals are found. See also NumericNode.
$assignInto[$arrayKey] = $singleMatch['Number'] + 0;
Expand All @@ -771,9 +798,11 @@ protected function recursiveArrayHandler($arrayText, ViewHelperInterface $viewHe
$assignInto[$arrayKey] = $this->buildArgumentObjectTree($argumentString);
}

if (isset($argumentDefinitions[$arrayKey]) && ($argumentDefinitions[$arrayKey]->getType() === 'boolean' || $argumentDefinitions[$arrayKey]->getType() === 'bool')) {
if ($isBoolean) {
$assignInto[$arrayKey] = new BooleanNode($assignInto[$arrayKey]);
}

$this->escapingEnabled = $escapingEnabledBackup;
}
}
if ($viewHelper instanceof ViewHelperInterface) {
Expand Down
4 changes: 2 additions & 2 deletions src/Core/ViewHelper/AbstractConditionViewHelper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ abstract class AbstractConditionViewHelper extends AbstractViewHelper
*/
public function initializeArguments()
{
$this->registerArgument('then', 'mixed', 'Value to be returned if the condition if met.', false);
$this->registerArgument('else', 'mixed', 'Value to be returned if the condition if not met.', false);
$this->registerArgument('then', 'mixed', 'Value to be returned if the condition if met.', false, null, true);
$this->registerArgument('else', 'mixed', 'Value to be returned if the condition if not met.', false, null, true);
}

/**
Expand Down
10 changes: 6 additions & 4 deletions src/Core/ViewHelper/AbstractViewHelper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,19 +158,20 @@ public function isOutputEscapingEnabled()
* @param string $description Description of the argument
* @param boolean $required If TRUE, argument is required. Defaults to FALSE.
* @param mixed $defaultValue Default value of argument
* @param bool|null $escape Can be toggled to TRUE to force escaping of variables and inline syntax passed as argument value.
* @return \TYPO3Fluid\Fluid\Core\ViewHelper\AbstractViewHelper $this, to allow chaining.
* @throws Exception
* @api
*/
protected function registerArgument($name, $type, $description, $required = false, $defaultValue = null)
protected function registerArgument($name, $type, $description, $required = false, $defaultValue = null, $escape = null)

This comment has been minimized.

Sign in to view

Copy link
@albe

albe Nov 16, 2020

Contributor

Note: Those signature changes are actually breaking (see https://travis-ci.com/github/neos/flow-development-collection/jobs/441172242#L1188), because downstream extensions of this class with those methods need to be changed in a way that can not be made b/c. So they need to target 2.6.10 minimum
{
if (array_key_exists($name, $this->argumentDefinitions)) {
throw new Exception(
'Argument "' . $name . '" has already been defined, thus it should not be defined again.',
1253036401
);
}
$this->argumentDefinitions[$name] = new ArgumentDefinition($name, $type, $description, $required, $defaultValue);
$this->argumentDefinitions[$name] = new ArgumentDefinition($name, $type, $description, $required, $defaultValue, $escape);
return $this;
}

Expand All @@ -184,19 +185,20 @@ protected function registerArgument($name, $type, $description, $required = fals
* @param string $description Description of the argument
* @param boolean $required If TRUE, argument is required. Defaults to FALSE.
* @param mixed $defaultValue Default value of argument
* @param bool|null $escape Can be toggled to TRUE to force escaping of variables and inline syntax passed as argument value.
* @return \TYPO3Fluid\Fluid\Core\ViewHelper\AbstractViewHelper $this, to allow chaining.
* @throws Exception
* @api
*/
protected function overrideArgument($name, $type, $description, $required = false, $defaultValue = null)
protected function overrideArgument($name, $type, $description, $required = false, $defaultValue = null, $escape = null)
{
if (!array_key_exists($name, $this->argumentDefinitions)) {
throw new Exception(
'Argument "' . $name . '" has not been defined, thus it can\'t be overridden.',
1279212461
);
}
$this->argumentDefinitions[$name] = new ArgumentDefinition($name, $type, $description, $required, $defaultValue);
$this->argumentDefinitions[$name] = new ArgumentDefinition($name, $type, $description, $required, $defaultValue, $escape);
return $this;
}

Expand Down
27 changes: 26 additions & 1 deletion src/Core/ViewHelper/ArgumentDefinition.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,21 @@ class ArgumentDefinition
*/
protected $defaultValue = null;

/**
* Escaping instruction, in line with $this->escapeOutput / $this->escapeChildren on ViewHelpers.
*
* A value of NULL means "use default behavior" (which is to escape nodes contained in the value).
*
* A value of TRUE means "escape unless escaping is disabled" (e.g. if argument is used in a ViewHelper nested
* within f:format.raw which disables escaping, the argument will not be escaped).
*
* A value of FALSE means "never escape argument" (as in behavior of f:format.raw, which supports both passing
* argument as actual argument or as tag content, but wants neither to be escaped).
*
* @var bool|null
*/
protected $escape = null;

/**
* Constructor for this argument definition.
*
Expand All @@ -55,14 +70,16 @@ class ArgumentDefinition
* @param string $description Description of argument
* @param boolean $required TRUE if argument is required
* @param mixed $defaultValue Default value
* @param bool|null $escape Whether or not argument is escaped, or uses default escaping behavior (see class var comment)
*/
public function __construct($name, $type, $description, $required, $defaultValue = null)
public function __construct($name, $type, $description, $required, $defaultValue = null, $escape = null)
{
$this->name = $name;
$this->type = $type;
$this->description = $description;
$this->required = $required;
$this->defaultValue = $defaultValue;
$this->escape = $escape;
}

/**
Expand Down Expand Up @@ -114,4 +131,12 @@ public function getDefaultValue()
{
return $this->defaultValue;
}

/**
* @return bool|null
*/
public function getEscape()
{
return $this->escape;
}
}
3 changes: 3 additions & 0 deletions src/Core/ViewHelper/TagBuilder.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ public function ignoreEmptyAttributes($ignoreEmptyAttributes)
*/
public function addAttribute($attributeName, $attributeValue, $escapeSpecialCharacters = true)
{
if ($escapeSpecialCharacters) {
$attributeName = htmlspecialchars($attributeName);
}
if ($attributeName === 'data' && (is_array($attributeValue) || $attributeValue instanceof \Traversable)) {
foreach ($attributeValue as $name => $value) {
$this->addAttribute('data-' . $name, $value, $escapeSpecialCharacters);
Expand Down
2 changes: 1 addition & 1 deletion src/Core/ViewHelper/Traits/CompileWithContentArgumentAndRenderStatic.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ protected function buildRenderChildrenClosure()
/**
* @return string
*/
protected function resolveContentArgumentName()
public function resolveContentArgumentName()
{
if (empty($this->contentArgumentName)) {
$registeredArguments = call_user_func_array([$this, 'prepareArguments'], []);
Expand Down
2 changes: 1 addition & 1 deletion src/ViewHelpers/Format/RawViewHelper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class RawViewHelper extends AbstractViewHelper
*/
public function initializeArguments()
{
$this->registerArgument('value', 'mixed', 'The value to output');
$this->registerArgument('value', 'mixed', 'The value to output', false, null, false);
}

/**
Expand Down
Loading

0 comments on commit f20db4e

Please sign in to comment.