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

Already on GitHub? Sign in to your account

"xss_clean doesn't" the third #2066

Closed
sourcejedi opened this Issue Dec 10, 2012 · 3 comments

Comments

Projects
None yet
3 participants
Contributor

sourcejedi commented Dec 10, 2012

PR #2049 would fix #2065 (though it may open other issues at the moment).

xss_clean() should die. For educational purposes, here is the next bypass. Tested successfully both with and without the current PR #2049.

<img src="http://www.w3schools.com/tags/planets.gif" width="145" height="126" alt="Planets" usemap="#planetmap">

<map name="planetmap">
  <area shape="rect" coords="0,0,145,126" a-=">" href="j&#x61;vascript:&#x61;lert(-1)">
</map>

ariven commented Dec 12, 2012

While not an expert in xss_clean() and I haven't seen all of the discussions regarding this, so I am not sure if someone has suggested this already... what about using Html Purifier ( http://htmlpurifier.org/ ) as the core of xss_clean()?

Contributor

sourcejedi commented Dec 12, 2012

Yep, I've said I'm happy to see htmlpurifier used. That's what the two more professional reports in 2011 and 2012 recommended. I suspect they gave up, and that's why you're left with me :). It still leaves a problem with xss_clean in the form validation library, which I believe is where most people encounter it.

The docs say an xss_clean rule "remove[s] malicious data", without qualification. Which is impossible. The three devs I know believed it; they were most dismayed when I pointed out that the result is not safe to use inside an HTML attribute. A separate dev reported this as a bug. The function doesn't escape " characters by design, because it's supposed to allow non-malicious html.

I may be overreacting to the history here. I decided not to work on documentation fixes because it didn't look like CI would accept the need for them. If the docs can be fixed, and the code changed to htmlpurifier, that would be a great improvement.

I would still consider xss_clean deprecated, because the name would be wrong. Apart from the point above, htmlpurifier does more than just remove xss - e.g. it would also ensure correctly balanced tags.

Beyond that, there's an argument that a backwards compatibility wrapper does more harm than good. I would like to see a clean break myself. Forcing a simple search+replace from xss_clean to something else would give people an opportunity to stop and think about what the code is really doing. But I that's probably a more subtle issue, and long-term maintainers might know better.

ariven commented Dec 12, 2012

I think a clean break would be good too, though a search/replace could easily be circumvented by using a replacement function call that is named xss_clean() so I am not sure if it will help the people that would benefit the most from a stop and think...

For my own projects I actually am overriding the input library for get and post, and use html_purify instead of the xss_clean option.. just so I don't forget to use it instead.. or if I have legacy code that I haven't gone back to fix yet.

@narfbg narfbg closed this Dec 18, 2012

@mwhitneysdsu mwhitneysdsu referenced this issue in ci-bonfire/Bonfire Jan 22, 2014

Closed

Adding functions to BF_Model #977

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment