Skip to content

Commit f20db4e

Browse files
NamelessCoderNamelessCoder
committed
[SECURITY] Introduce selective argument escaping
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.
1 parent f5c4593 commit f20db4e

File tree

12 files changed

+275
-68
lines changed

12 files changed

+275
-68
lines changed

‎src/Core/Parser/Configuration.php

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@
1212
*/
1313
class Configuration
1414
{
15+
/**
16+
* @var bool
17+
*/
18+
protected $viewHelperArgumentEscapingEnabled = true;
1519

1620
/**
1721
* Generic interceptors registered with the configuration.
@@ -27,6 +31,22 @@ class Configuration
2731
*/
2832
protected $escapingInterceptors = [];
2933

34+
/**
35+
* @return bool
36+
*/
37+
public function isViewHelperArgumentEscapingEnabled()
38+
{
39+
return $this->viewHelperArgumentEscapingEnabled;
40+
}
41+
42+
/**
43+
* @param bool $viewHelperArgumentEscapingEnabled
44+
*/
45+
public function setViewHelperArgumentEscapingEnabled($viewHelperArgumentEscapingEnabled): void
46+
{
47+
$this->viewHelperArgumentEscapingEnabled = (bool) $viewHelperArgumentEscapingEnabled;
48+
}
49+
3050
/**
3151
* Adds an interceptor to apply to values coming from object accessors.
3252
*

‎src/Core/Parser/TemplateParser.php

Lines changed: 41 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -489,7 +489,7 @@ protected function objectAccessorHandler(ParsingState $state, $objectAccessorStr
489489
}
490490
$viewHelper = $viewHelperResolver->createViewHelperInstance($singleMatch['NamespaceIdentifier'], $singleMatch['MethodIdentifier']);
491491
if (strlen($singleMatch['ViewHelperArguments']) > 0) {
492-
$arguments = $this->recursiveArrayHandler($singleMatch['ViewHelperArguments'], $viewHelper);
492+
$arguments = $this->recursiveArrayHandler($state, $singleMatch['ViewHelperArguments'], $viewHelper);
493493
} else {
494494
$arguments = [];
495495
}
@@ -563,28 +563,43 @@ protected function parseArguments($argumentsString, ViewHelperInterface $viewHel
563563
$undeclaredArguments = [];
564564
$matches = [];
565565
if (preg_match_all(Patterns::$SPLIT_PATTERN_TAGARGUMENTS, $argumentsString, $matches, PREG_SET_ORDER) > 0) {
566-
$escapingEnabledBackup = $this->escapingEnabled;
567-
$this->escapingEnabled = false;
568566
foreach ($matches as $singleMatch) {
569567
$argument = $singleMatch['Argument'];
570568
$value = $this->unquoteString($singleMatch['ValueQuoted']);
571-
$argumentsObjectTree[$argument] = $this->buildArgumentObjectTree($value);
569+
$escapingEnabledBackup = $this->escapingEnabled;
572570
if (isset($argumentDefinitions[$argument])) {
573571
$argumentDefinition = $argumentDefinitions[$argument];
574-
if ($argumentDefinition->getType() === 'boolean' || $argumentDefinition->getType() === 'bool') {
572+
$this->escapingEnabled = $this->escapingEnabled && $this->isArgumentEscaped($viewHelper, $argumentDefinition);
573+
$isBoolean = $argumentDefinition->getType() === 'boolean' || $argumentDefinition->getType() === 'bool';
574+
$argumentsObjectTree[$argument] = $this->buildArgumentObjectTree($value);
575+
if ($isBoolean) {
575576
$argumentsObjectTree[$argument] = new BooleanNode($argumentsObjectTree[$argument]);
576577
}
577578
} else {
578-
$undeclaredArguments[$argument] = $argumentsObjectTree[$argument];
579+
$this->escapingEnabled = false;
580+
$undeclaredArguments[$argument] = $this->buildArgumentObjectTree($value);
579581
}
582+
$this->escapingEnabled = $escapingEnabledBackup;
580583
}
581-
$this->escapingEnabled = $escapingEnabledBackup;
582584
}
583585
$this->abortIfRequiredArgumentsAreMissing($argumentDefinitions, $argumentsObjectTree);
584586
$viewHelper->validateAdditionalArguments($undeclaredArguments);
585587
return $argumentsObjectTree + $undeclaredArguments;
586588
}
587589

590+
protected function isArgumentEscaped(ViewHelperInterface $viewHelper, ArgumentDefinition $argumentDefinition = null)
591+
{
592+
$hasDefinition = $argumentDefinition instanceof ArgumentDefinition;
593+
$isBoolean = $hasDefinition && ($argumentDefinition->getType() === 'boolean' || $argumentDefinition->getType() === 'bool');
594+
$escapingEnabled = $this->configuration->isViewHelperArgumentEscapingEnabled();
595+
$isArgumentEscaped = $hasDefinition && $argumentDefinition->getEscape() === true;
596+
$isContentArgument = $hasDefinition && method_exists($viewHelper, 'resolveContentArgumentName') && $argumentDefinition->getName() === $viewHelper->resolveContentArgumentName();
597+
if ($isContentArgument) {
598+
return !$isBoolean && ($viewHelper->isChildrenEscapingEnabled() || $isArgumentEscaped);
599+
}
600+
return !$isBoolean && $escapingEnabled && $isArgumentEscaped;
601+
}
602+
588603
/**
589604
* Build up an argument object tree for the string in $argumentString.
590605
* This builds up the tree for a single argument value.
@@ -664,7 +679,7 @@ protected function textAndShorthandSyntaxHandler(ParsingState $state, $text, $co
664679
&& preg_match(Patterns::$SCAN_PATTERN_SHORTHANDSYNTAX_ARRAYS, $section, $matchedVariables) > 0
665680
) {
666681
// We only match arrays if we are INSIDE viewhelper arguments
667-
$this->arrayHandler($state, $this->recursiveArrayHandler($matchedVariables['Array']));
682+
$this->arrayHandler($state, $this->recursiveArrayHandler($state, $matchedVariables['Array']));
668683
} else {
669684
// We ask custom ExpressionNode instances from ViewHelperResolver
670685
// if any match our expression:
@@ -737,12 +752,13 @@ protected function arrayHandler(ParsingState $state, $arrayText)
737752
* - Variables
738753
* - sub-arrays
739754
*
755+
* @param ParsingState $state
740756
* @param string $arrayText Array text
741757
* @param ViewHelperInterface|null $viewHelper ViewHelper instance - passed only if the array is a collection of arguments for an inline ViewHelper
742758
* @return NodeInterface[] the array node built up
743759
* @throws Exception
744760
*/
745-
protected function recursiveArrayHandler($arrayText, ViewHelperInterface $viewHelper = null)
761+
protected function recursiveArrayHandler(ParsingState $state, $arrayText, ViewHelperInterface $viewHelper = null)
746762
{
747763
$undeclaredArguments = [];
748764
$argumentDefinitions = [];
@@ -755,14 +771,25 @@ protected function recursiveArrayHandler($arrayText, ViewHelperInterface $viewHe
755771
foreach ($matches as $singleMatch) {
756772
$arrayKey = $this->unquoteString($singleMatch['Key']);
757773
$assignInto = &$arrayToBuild;
758-
if (!isset($argumentDefinitions[$arrayKey])) {
774+
$isBoolean = false;
775+
$argumentDefinition = null;
776+
if (isset($argumentDefinitions[$arrayKey])) {
777+
$argumentDefinition = $argumentDefinitions[$arrayKey];
778+
$isBoolean = $argumentDefinitions[$arrayKey]->getType() === 'boolean' || $argumentDefinitions[$arrayKey]->getType() === 'bool';
779+
} else {
759780
$assignInto = &$undeclaredArguments;
760781
}
761782

783+
$escapingEnabledBackup = $this->escapingEnabled;
784+
$this->escapingEnabled = $this->escapingEnabled && $viewHelper instanceof ViewHelperInterface && $this->isArgumentEscaped($viewHelper, $argumentDefinition);
785+
762786
if (array_key_exists('Subarray', $singleMatch) && !empty($singleMatch['Subarray'])) {
763-
$assignInto[$arrayKey] = new ArrayNode($this->recursiveArrayHandler($singleMatch['Subarray']));
787+
$assignInto[$arrayKey] = new ArrayNode($this->recursiveArrayHandler($state, $singleMatch['Subarray']));
764788
} elseif (!empty($singleMatch['VariableIdentifier'])) {
765789
$assignInto[$arrayKey] = new ObjectAccessorNode($singleMatch['VariableIdentifier']);
790+
if ($viewHelper instanceof ViewHelperInterface && !$isBoolean) {
791+
$this->callInterceptor($assignInto[$arrayKey], InterceptorInterface::INTERCEPT_OBJECTACCESSOR, $state);
792+
}
766793
} elseif (array_key_exists('Number', $singleMatch) && (!empty($singleMatch['Number']) || $singleMatch['Number'] === '0')) {
767794
// Note: this method of casting picks "int" when value is a natural number and "float" if any decimals are found. See also NumericNode.
768795
$assignInto[$arrayKey] = $singleMatch['Number'] + 0;
@@ -771,9 +798,11 @@ protected function recursiveArrayHandler($arrayText, ViewHelperInterface $viewHe
771798
$assignInto[$arrayKey] = $this->buildArgumentObjectTree($argumentString);
772799
}
773800

774-
if (isset($argumentDefinitions[$arrayKey]) && ($argumentDefinitions[$arrayKey]->getType() === 'boolean' || $argumentDefinitions[$arrayKey]->getType() === 'bool')) {
801+
if ($isBoolean) {
775802
$assignInto[$arrayKey] = new BooleanNode($assignInto[$arrayKey]);
776803
}
804+
805+
$this->escapingEnabled = $escapingEnabledBackup;
777806
}
778807
}
779808
if ($viewHelper instanceof ViewHelperInterface) {

‎src/Core/ViewHelper/AbstractConditionViewHelper.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,8 @@ abstract class AbstractConditionViewHelper extends AbstractViewHelper
4242
*/
4343
public function initializeArguments()
4444
{
45-
$this->registerArgument('then', 'mixed', 'Value to be returned if the condition if met.', false);
46-
$this->registerArgument('else', 'mixed', 'Value to be returned if the condition if not met.', false);
45+
$this->registerArgument('then', 'mixed', 'Value to be returned if the condition if met.', false, null, true);
46+
$this->registerArgument('else', 'mixed', 'Value to be returned if the condition if not met.', false, null, true);
4747
}
4848

4949
/**

‎src/Core/ViewHelper/AbstractViewHelper.php

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -158,19 +158,20 @@ public function isOutputEscapingEnabled()
158158
* @param string $description Description of the argument
159159
* @param boolean $required If TRUE, argument is required. Defaults to FALSE.
160160
* @param mixed $defaultValue Default value of argument
161+
* @param bool|null $escape Can be toggled to TRUE to force escaping of variables and inline syntax passed as argument value.
161162
* @return \TYPO3Fluid\Fluid\Core\ViewHelper\AbstractViewHelper $this, to allow chaining.
162163
* @throws Exception
163164
* @api
164165
*/
165-
protected function registerArgument($name, $type, $description, $required = false, $defaultValue = null)
166+
protected function registerArgument($name, $type, $description, $required = false, $defaultValue = null, $escape = null)
166167
{
167168
if (array_key_exists($name, $this->argumentDefinitions)) {
168169
throw new Exception(
169170
'Argument "' . $name . '" has already been defined, thus it should not be defined again.',
170171
1253036401
171172
);
172173
}
173-
$this->argumentDefinitions[$name] = new ArgumentDefinition($name, $type, $description, $required, $defaultValue);
174+
$this->argumentDefinitions[$name] = new ArgumentDefinition($name, $type, $description, $required, $defaultValue, $escape);
174175
return $this;
175176
}
176177

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

‎src/Core/ViewHelper/ArgumentDefinition.php

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,21 @@ class ArgumentDefinition
4747
*/
4848
protected $defaultValue = null;
4949

50+
/**
51+
* Escaping instruction, in line with $this->escapeOutput / $this->escapeChildren on ViewHelpers.
52+
*
53+
* A value of NULL means "use default behavior" (which is to escape nodes contained in the value).
54+
*
55+
* A value of TRUE means "escape unless escaping is disabled" (e.g. if argument is used in a ViewHelper nested
56+
* within f:format.raw which disables escaping, the argument will not be escaped).
57+
*
58+
* A value of FALSE means "never escape argument" (as in behavior of f:format.raw, which supports both passing
59+
* argument as actual argument or as tag content, but wants neither to be escaped).
60+
*
61+
* @var bool|null
62+
*/
63+
protected $escape = null;
64+
5065
/**
5166
* Constructor for this argument definition.
5267
*
@@ -55,14 +70,16 @@ class ArgumentDefinition
5570
* @param string $description Description of argument
5671
* @param boolean $required TRUE if argument is required
5772
* @param mixed $defaultValue Default value
73+
* @param bool|null $escape Whether or not argument is escaped, or uses default escaping behavior (see class var comment)
5874
*/
59-
public function __construct($name, $type, $description, $required, $defaultValue = null)
75+
public function __construct($name, $type, $description, $required, $defaultValue = null, $escape = null)
6076
{
6177
$this->name = $name;
6278
$this->type = $type;
6379
$this->description = $description;
6480
$this->required = $required;
6581
$this->defaultValue = $defaultValue;
82+
$this->escape = $escape;
6683
}
6784

6885
/**
@@ -114,4 +131,12 @@ public function getDefaultValue()
114131
{
115132
return $this->defaultValue;
116133
}
134+
135+
/**
136+
* @return bool|null
137+
*/
138+
public function getEscape()
139+
{
140+
return $this->escape;
141+
}
117142
}

‎src/Core/ViewHelper/TagBuilder.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,9 @@ public function ignoreEmptyAttributes($ignoreEmptyAttributes)
191191
*/
192192
public function addAttribute($attributeName, $attributeValue, $escapeSpecialCharacters = true)
193193
{
194+
if ($escapeSpecialCharacters) {
195+
$attributeName = htmlspecialchars($attributeName);
196+
}
194197
if ($attributeName === 'data' && (is_array($attributeValue) || $attributeValue instanceof \Traversable)) {
195198
foreach ($attributeValue as $name => $value) {
196199
$this->addAttribute('data-' . $name, $value, $escapeSpecialCharacters);

‎src/Core/ViewHelper/Traits/CompileWithContentArgumentAndRenderStatic.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ protected function buildRenderChildrenClosure()
116116
/**
117117
* @return string
118118
*/
119-
protected function resolveContentArgumentName()
119+
public function resolveContentArgumentName()
120120
{
121121
if (empty($this->contentArgumentName)) {
122122
$registeredArguments = call_user_func_array([$this, 'prepareArguments'], []);

‎src/ViewHelpers/Format/RawViewHelper.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ class RawViewHelper extends AbstractViewHelper
6464
*/
6565
public function initializeArguments()
6666
{
67-
$this->registerArgument('value', 'mixed', 'The value to output');
67+
$this->registerArgument('value', 'mixed', 'The value to output', false, null, false);
6868
}
6969

7070
/**

0 commit comments

Comments
 (0)