Bootstrap.HtmlHelper introduces HTML Injection

[asgraf]

This is a (multiple allowed):

• bug
• enhancement
• question

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]

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]

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]

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]

@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]

Bootstrap 3 since it is a legacy project.

[asgraf]

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]

@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) - 69653d8

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.