Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] Improved argument validation in TagBuilder #937

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

s2b
Copy link
Contributor

@s2b s2b commented Jul 24, 2024

The current approach to sanitize HTML tag attributes by
TagBuilder can be improved to only allow certain characters.
As TagBuilder is a low-level API to create arbitrary HTML tags,
the goal is not to prevent any JavaScript from being executed
because this might be a desired behavior in some use cases.

We would like to prevent the following:

$unsafeInput = "onclick='alert(1)'";
$tagBuilder->addAttribute($unsafeInput, 'some value');

While still allowing the following:

$tagBuilder->addAttribute('onclick', 'doSomething()');

Thus, this patch applies a strict allow-list to argument names
and throws an exception if any other characters are used.
The characters are limited to ASCII characters except for
select characters that are problematic in HTML attributes,
such as single and double quotes, equal signs, less/greater than,
forward slashes, ampersants and white space.

Developers are advised to be very careful when using user input
as attribute names as this change cannot prevent all accidential
and thus potentially malicious JavaScript execution.

This will be downported to Fluid 2.15.

Thanks to @GatekeeperBuster for making us aware of this.

Resolves: #936

@s2b s2b force-pushed the bugfix/tagBuilderArguments branch 3 times, most recently from 9e34e53 to 06ba3af Compare July 26, 2024 08:40
@s2b s2b changed the title [BUGFIX] Improved argument sanitation in TagBuilder [BUGFIX] Improved argument validation in TagBuilder Jul 26, 2024
@liayn
Copy link
Contributor

liayn commented Aug 9, 2024

Thanks for the work done here. My take on this: We cannot cover all cases properly. Attributes are the responsibility of the integrator. The warning about being careful about attributes created from user-input should be places somewhere really well visible.

Code-wise I would not try to validate anything, otherwise more reports will pop-up like "nice, but you do not cover XYZ". That's not going to help us. I'd stick with the motto "Do it right or not at all." in this case.

The current approach to sanitize HTML tag attributes by
TagBuilder can be improved to only allow certain characters.
As TagBuilder is a low-level API to create arbitrary HTML tags,
the goal is not to prevent any JavaScript from being executed
because this might be a desired behavior in some use cases.

We would like to prevent the following:

```php
$unsafeInput = "onclick='alert(1)'";
$tagBuilder->addAttribute($unsafeInput, 'some value');
```

While still allowing the following:

```php
$tagBuilder->addAttribute('onclick', 'doSomething()');
```

Thus, this patch applies a strict allow-list to argument names
and throws an exception if any other characters are used.
The characters are limited to ASCII characters except for
select characters that are problematic in HTML attributes,
such as single and double quotes, equal signs, less/greater than,
forward slashes, ampersants and white space.

Developers are advised to be very careful when using user input
as attribute names as this change cannot prevent all accidential
and thus potentially malicious JavaScript execution.

This will be downported to Fluid 2.14.

Thanks to @GatekeeperBuster for making us aware of this.

Resolves: #936
@s2b s2b force-pushed the bugfix/tagBuilderArguments branch from 06ba3af to b393fe7 Compare August 14, 2024 13:08
@s2b
Copy link
Contributor Author

s2b commented Aug 14, 2024

@liayn Thank you for your feedback. I agree that it isn't really possible to cover all cases here. However, if we would follow "Do it right or not at all.", we should also remove htmlspecialchars() here because it covers even less cases.

Based on the feedback of the TYPO3 security channel, I'd argue that we should merge this imperfect improvement. The validation is better than before and a clear error is given if something is obviously wrong.

@s2b s2b merged commit 12cde31 into main Aug 14, 2024
8 checks passed
@s2b s2b deleted the bugfix/tagBuilderArguments branch August 14, 2024 14:34
s2b added a commit that referenced this pull request Aug 14, 2024
The current approach to sanitize HTML tag attributes by
TagBuilder can be improved to only allow certain characters.
As TagBuilder is a low-level API to create arbitrary HTML tags,
the goal is not to prevent any JavaScript from being executed
because this might be a desired behavior in some use cases.

We would like to prevent the following:

```php
$unsafeInput = "onclick='alert(1)'";
$tagBuilder->addAttribute($unsafeInput, 'some value');
```

While still allowing the following:

```php
$tagBuilder->addAttribute('onclick', 'doSomething()');
```

Thus, this patch applies a strict allow-list to argument names
and throws an exception if any other characters are used.
The characters are limited to ASCII characters except for
select characters that are problematic in HTML attributes,
such as single and double quotes, equal signs, less/greater than,
forward slashes, ampersants and white space.

Developers are advised to be very careful when using user input
as attribute names as this change cannot prevent all accidential
and thus potentially malicious JavaScript execution.

Thanks to @GatekeeperBuster for making us aware of this.
s2b added a commit that referenced this pull request Aug 14, 2024
The current approach to sanitize HTML tag attributes by
TagBuilder can be improved to only allow certain characters.
As TagBuilder is a low-level API to create arbitrary HTML tags,
the goal is not to prevent any JavaScript from being executed
because this might be a desired behavior in some use cases.

We would like to prevent the following:

```php
$unsafeInput = "onclick='alert(1)'";
$tagBuilder->addAttribute($unsafeInput, 'some value');
```

While still allowing the following:

```php
$tagBuilder->addAttribute('onclick', 'doSomething()');
```

Thus, this patch applies a strict allow-list to argument names
and throws an exception if any other characters are used.
The characters are limited to ASCII characters except for
select characters that are problematic in HTML attributes,
such as single and double quotes, equal signs, less/greater than,
forward slashes, ampersants and white space.

Developers are advised to be very careful when using user input
as attribute names as this change cannot prevent all accidential
and thus potentially malicious JavaScript execution.

Thanks to @GatekeeperBuster for making us aware of this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve sanitation of tag attribute names
4 participants