CodeIgniter - why use xss_clean

Question

if I'm sanitizing my DB inserts, and also escaping the HTML I write with htmlentities($text, ENT_COMPAT, 'UTF-8') - is there any point to also filtering the inputs with xss_clean? What other benefits does it give?

Solution 1

xss_clean() is extensive, and also silly. 90% of this function does nothing to prevent XSS. Such as looking for the word alert but not document.cookie. No hacker is going to use alert in their exploit, they are going to hijack the cookie with XSS or read a CSRF token to make an XHR.

However running htmlentities() or htmlspecialchars() with it is redundant. A case where xss_clean() fixes the issue and htmlentities($text, ENT_COMPAT, 'UTF-8') fails is the following:

<?php
print "<img src='$var'>";
?>

A simple poc is:

http://localhost/xss.php?var=http://domain/some_image.gif'%20onload=alert(/xss/)

This will add the onload= event handler to the image tag. A method of stopping this form of XSS is htmlspecialchars($var,ENT_QUOTES); or in this case xss_clean() will also prevent this.

However, quoting from the xss_clean() documentation:

Nothing is ever 100% foolproof, of course, but I haven't been able to get anything passed the filter.

That being said, XSS is an output problem not an input problem. For instance, this function cannot take into account that the variable is already within a <script> tag or event handler. It also doesn't stop DOM Based XSS. You need to take into consideration how you are using the data in order to use the best function. Filtering all data on input is a bad practice. Not only is it insecure but it also corrupts data which can make comparisons difficult.

Solution 2

In your case, "stricter methods are fine, and lighter-weight". CodeIgniter developers intend xss_clean() for a different use case, "a commenting system or forum that allows 'safe' HTML tags". This isn't clear from the documentation, where xss_clean is shown applied to a username field.

There's another reason to never use xss_clean(), that hasn't been highlighted on StackOverflow so far. xss_clean() was broken during 2011 and 2012, and it's impossible to fix completely. At least without a complete redesign, which didn't happen. At the moment, it's still vulnerable to strings like this:

<a href="j&#x26;#x41;vascript:alert%252831337%2529">Hello</a>

The current implementation of xss_clean() starts by effectively applying urldecode() and html_entity_decode() to the entire string. This is needed so it can use a naive check for things like "javascript:". In the end, it returns the decoded string.

An attacker can simply encode their exploit twice. It will be decoded once by xss_clean(), and passed as clean. You then have a singly-encoded exploit, ready for execution in the browser.

I call these checks "naive" and unfixable because they're largely reliant on regular expressions. HTML is not a regular language. You need a more powerful parser to match the one in the browser; xss_clean() doesn't have anything like that. Maybe it's possible to whitelist a subset of HTML, which lexes cleanly with regular expressions. However, the current xss_clean() is very much a blacklist.

Solution 3

I would recommend using http://htmlpurifier.org/ for doing XSS purification. I'm working on extending my CodeIgniter Input class to start leveraging it.

Solution 4

Yes you should still be using it, I generally make it a rule to use it at least on public facing input, meaning any input that anyone can access and submit to.

Generally sanitizing the input for DB queries seems like a side-effect as the true purpose of the function is to prevent Cross-site Scripting Attacks.

I'm not going to get into the nitty gritty details of every step xss_clean takes, but i will tell you it does more than the few steps you mentioned, I've pastied the source of the xss_clean function(deadlink) so you can look yourself, it is fully commented.

View more solutions

Comments

[rook]

htmlentities($text, ENT_COMPAT, 'UTF-8') is not a good method of stopping xss, no one should be using this.

[Amit Patil]

htmlentities is absolutely proof against HTML-injection, though ENT_QUOTES is needed instead of ENT_COMPAT if you ever use single quote attribute delimiters. htmlspecialchars is generally preferable to htmlentities, though, as it has less chance of messing up the charset. CodeIgniter's xss_clean is a worthless cargo-cult-programming disaster area full of wrongheaded misunderstandings of what constitutes string handling.

[Dan Searle]

Thanks. I guess the important point is really "what are you doing with the data". The job I was working on when this came up was an editable block of text, where I didn't want any active HTML tags at all, so my solution works in that case. The output is dumped into a DIV, and with all HTML tags encoded I don't see how anything malicious could be inserted there. Of course if I wanted to allow some HTML in the input that would complicate things. Still, I'm not too happy with the idea of encoding everything on input, I'd rather handle it as needed on output (while protecting the db of course).

[rook]

@Dan Searle if you want a safe sub-set of html then you should checkout htmlpurifier.org

[Amit Patil]

XSS is an output problem and not an input problem, which is why something like xss_clean() can never be a reliable approach to solving XSS problems (and xss_clean() itself is a laughably terrible implementation even by the low, low standards of anti-XSS tools). I'm very surprised you appear to be endorsing it as a preferable alternative to output-level escaping in your first sentence.

[rook]

@bobince your totally correct. In fact 90% of the checks made in xss_clean() do nothing for security. Such as looking for alert() but not document.cookie. But later in my post i did say xss is an ouput problem, and that there is only one way that i know of where xss is possible. BUt i'll update this post.

[Amit Patil]

Thanks, that's better! Actually the current version of xss_clean does appear to look for document.cookie... although obviously this is still completely useless as it wouldn't catch document . cookie, document['cookie'] or any of the other thousand ways you could refer to it.

[rook]

@bobince hah yeah that's a futile attempt at security.

[PC.]

You were able to extending CI for htmlpurifier?

[Anthony]

Sorry, I've long since moved on to Laravel. :)

[jpruiz114]

To be more precise, the Security class (codeigniter\2.1.4\core\Security.php) looks for this words: $words = array('javascript', 'expression', 'vbscript', 'script', 'base64', 'applet', 'alert', 'document', 'write', 'cookie', 'window'); It would to nothing against a real XSS attack.

[fgb]

They don't recommend global_xss_filtering now. See: codeigniter.com/user_guide/libraries/input.html "The ‘global_xss_filtering’ setting is DEPRECATED and kept solely for backwards-compatibility purposes. XSS escaping should be performed on output, not input!"

[Barbz_YHOOL]

there is a codeigniter helper for htmlpurifier on github github.com/refringe/CodeIgniter-HTMLPurifier