mybb / mybb2

The repository for the MyBB 2 forum software. Not to be used on live boards.
https://www.mybb.com
BSD 3-Clause "New" or "Revised" License
109 stars 45 forks source link

#127 Username XSS #130

Closed JN-Jones closed 9 years ago

JN-Jones commented 9 years ago

127

The XSS is in the powertip plugin we're using which allows the usage of HTML but unfortunately doesn't have a setting to display it. We need to subscribe to the render event and reescape the strings there (afaik we don't have a tooltip which has HTML). As I don't have NodeJS installed on my Laptop I need to take a look at that later.

JN-Jones commented 9 years ago

Events didn't work so I needed to overwrite one of the internal functions. I've created an issue so hopefully an option will be added but till then this seems to be the best way.

Destroy666x commented 9 years ago

I don't think that escaping it globally in Powertip is a good solution, we may want to use HTML in a tooltip at a later stage. Why wouldn't escaping the username variable with Twig work instead?

JN-Jones commented 9 years ago

Twig is configured to escape everything automatically. I've also tried to escape it again but that didn't work too. As mentioned I'm hoping that a proper option is added to make it a lot easier.

Destroy666x commented 9 years ago

Twig is configured to escape everything automatically

Yes, but the "strategy" can be changed to make it work 2 times: http://twig.sensiolabs.org/doc/filters/escape.html

{% set strategy = 'html' %}

{% autoescape 'html' %}
    {{ var|escape('html') }}   {# won't be double-escaped #}
    {{ var|escape(strategy) }} {# will be double-escaped #}
{% endautoescape %}
JN-Jones commented 9 years ago

I'll try to take a look at that later but I don't think that'll work too. Also I think that the global (or option wise) escaping is better, instead of multiple escaping. Depending on how the option is implemented to the library I'd add a flag attribute to the powertip elements: <a href="(...)" title="{{ var }}">(...)</a> would be escaped (default) and <a href="(...)" title="{{ var }}" data-powertip-escape="false">(...)</a> wouldn't be escaped. But as said, I need the option first for that.

JN-Jones commented 9 years ago

@euantorano @wpillar

euantorano commented 9 years ago

Looks like this PR is now broken ;)

JN-Jones commented 9 years ago

As always xD will fix it when I'm home (haven't installed gulp on my laptop)

JN-Jones commented 9 years ago

This has been fixed @euantorano