search

CakePHP-Bootstrap / cakephp3-bootstrap-helpers

CakePHP 3.x Helpers for Bootstrap 3 and 4.
https://holt59.github.io/cakephp3-bootstrap-helpers/
MIT License
130 stars 79 forks source link

Bootstrap.HtmlHelper introduces HTML Injection #176

Closed asgraf closed 5 years ago

asgraf commented 5 years ago

This is a (multiple allowed):

What you did

//This code is completely safe on orginal HtmlHelper
//Loading your HtmlHelper introduces html injection
$someUserProvidedString = 'i:tag <script>alert()</script>';
echo $this->Html->link($someUserProvidedString, $someUrl);

What happened

<i class="icon-tag "></i> <script>alert()</script> HTML Injection!

What you expected to happen

<i class="icon-tag "></i> &lt;script&gt;alert()&lt;/script&gt;

Holt59 commented 5 years ago

This is "intended", you should always disable easyIcon when using user-provided code:

$someUserProvidedString = 'i:tag <script>alert()</script>';
$this->Html->easyIcon = false;
echo $this->Html->link($someUserProvidedString, $someUrl);

I will add a something at the beginning of the documentation to warn users about this feature (maybe disable it by default) and see if I can come up with something that does not require escaping directly.

asgraf commented 5 years ago

Why just not just replace i:tag with <i class="icon-tag "></i> AFTER escaping whole title string first with h()? (of course only if 'escape'=>true) I have big existing aplication (70+ tables) Checking hundreds of ctp files just to add $this->Html->easyIcon = false; would be very tedious Or at least enable such a behavior with:

echo $this->Html->link($someUserProvidedString, $someUrl,['escape'=>true, 'easyIcon'=>true]);
Holt59 commented 5 years ago

The easy-icon process is not method-specific, I could escape the string after the callback but I am not sure this would work for all methods.

You can globally disable easy-icon by setting easyIcon = false in the AppView class when you load the HtmlHelper.

Holt59 commented 5 years ago

@asgraf I don't have much time right now but I will try to look for a way to avoid escaping the whole content when using easy-icon.

Are you using the Bootstrap 3 or Bootstrap 4 versions of the helper?

asgraf commented 5 years ago

Bootstrap 3 since it is a legacy project.

asgraf commented 5 years ago

Please notice that many other plugins (like this one) may provide and use their own ctp files. In current state (without disabling easyIcon globally in AppView) your plugin inject this vunerability into those other plugins code.

I have submitted pull request with a fix for this security issue.

Holt59 commented 5 years ago

@asgraf Thanks your PR. Unfortunately this was not very satisfying for me as this added a lots of code for a single method with easy-icon, and I have multiple ones.

I have just pushed a fix for the Bootstrap 3 version (v4 coming soon) - 69653d8d1bfcdb74ad3ca8d129483b41d9338324

This basically does what your method did but with less code per-method, it injects the easy-icon process at the end of the pre-processing, so the title is escaped prior to this if needed.

I've added a quick test to see if escaping work, and it seems to: https://github.com/Holt59/cakephp3-bootstrap-helpers/blob/master/tests/TestCase/View/Helper/EasyIconTraitTest.php#L155

Sorry for the delay in fixing this, but I've had no real time for this lately.

Feel free to comment on this feed if you feel something still need fixing.

Holt59 commented 5 years ago

Closing for now, feel free to re-open if you feel the above fix is not sufficient.