-1
\$\begingroup\$

I'm using these functions to filter all my input variables. I was quite confident it would protect my website from possible XSS and SQL injections, but there are still vulnerabilities.

Please suggest improvements for my filter code.

//filter input field
function cleanMe($data) {
   // normalize $data because of get_magic_quotes_gpc
   $dataNeedsStripSlashes = get_magic_quotes_gpc();
   if ($dataNeedsStripSlashes) {
       $data = stripslashes($data);
   }
   // normalize $data because of whitespace on beginning and end
   $data = trim($data);
   // strip tags
   $data = strip_tags($data);
   // replace characters with their HTML entitites
   $data = htmlentities($data);
   // mysql escape string    
   $data = mysql_real_escape_string($data);
   //call xss clean
   $data=xss_clean($data);
   return $data;
}
function xss_clean($data)
{
// Fix &entity\n;
$data = str_replace(array('&','<','>'), array('&','<','>'), $data);
$data = preg_replace('/(&#*\w+)[\x00-\x20]+;/u', '$1;', $data);
$data = preg_replace('/(&#x*[0-9A-F]+);*/iu', '$1;', $data);
$data = html_entity_decode($data, ENT_COMPAT, 'UTF-8');

// Remove any attribute starting with "on" or xmlns
$data = preg_replace('#(<[^>]+?[\x00-\x20"\'])(?:on|xmlns)[^>]*+>#iu', '$1>', $data);

// Remove javascript: and vbscript: protocols
$data = preg_replace('#([a-z]*)[\x00-\x20]*=[\x00-\x20]*([`\'"]*)[\x00-\x20]*j[\x00-\x20]*a[\x00-\x20]*v[\x00-\x20]*a[\x00-\x20]*s[\x00-\x20]*c[\x00-\x20]*r[\x00-\x20]*i[\x00-\x20]*p[\x00-\x20]*t[\x00-\x20]*:#iu', '$1=$2nojavascript...', $data);
$data = preg_replace('#([a-z]*)[\x00-\x20]*=([\'"]*)[\x00-\x20]*v[\x00-\x20]*b[\x00-\x20]*s[\x00-\x20]*c[\x00-\x20]*r[\x00-\x20]*i[\x00-\x20]*p[\x00-\x20]*t[\x00-\x20]*:#iu', '$1=$2novbscript...', $data);
$data = preg_replace('#([a-z]*)[\x00-\x20]*=([\'"]*)[\x00-\x20]*-moz-binding[\x00-\x20]*:#u', '$1=$2nomozbinding...', $data);

// Only works in IE: <span style="width: expression(alert('Ping!'));"></span>
$data = preg_replace('#(<[^>]+?)style[\x00-\x20]*=[\x00-\x20]*[`\'"]*.*?expression[\x00-\x20]*\([^>]*+>#i', '$1>', $data);
$data = preg_replace('#(<[^>]+?)style[\x00-\x20]*=[\x00-\x20]*[`\'"]*.*?behaviour[\x00-\x20]*\([^>]*+>#i', '$1>', $data);
$data = preg_replace('#(<[^>]+?)style[\x00-\x20]*=[\x00-\x20]*[`\'"]*.*?s[\x00-\x20]*c[\x00-\x20]*r[\x00-\x20]*i[\x00-\x20]*p[\x00-\x20]*t[\x00-\x20]*:*[^>]*+>#iu', '$1>', $data);

// Remove namespaced elements (we do not need them)
$data = preg_replace('#</*\w+:\w[^>]*+>#i', '', $data);

do
{
    // Remove really unwanted tags
    $old_data = $data;
    $data = preg_replace('#</*(?:applet|b(?:ase|gsound|link)|embed|frame(?:set)?|i(?:frame|layer)|l(?:ayer|ink)|meta|object|s(?:cript|tyle)|title|xml)[^>]*+>#i', '', $data);
}
while ($old_data !== $data);

// we are done...
return $data;
}
\$\endgroup\$

migrated from stackoverflow.com Sep 8 '14 at 11:15

This question came from our site for professional and enthusiast programmers.

  • 5
    \$\begingroup\$ This is a total mess. First of all, you are using 'mysql_real_escape_string' mysql_* functions are deprecated and they are removed. Can you see the big red box? php.net/manual/en/function.mysql-real-escape-string.php \$\endgroup\$ – newboyhun Aug 31 '14 at 15:51
  • 10
    \$\begingroup\$ Throw it out and start again. Don't try to write a one-function-fits-all sanitisation function. Making data safe for putting in a database is a different problem to making data safe for putting in an HTML document. (And blacklists are a terrible idea) \$\endgroup\$ – Quentin Aug 31 '14 at 15:51
  • 2
    \$\begingroup\$ Ususally htmlentities and/or strip_tags is totally sufficient... not sure what's going on in your code but it looks really messy. \$\endgroup\$ – frankster Aug 31 '14 at 15:53
  • 1
    \$\begingroup\$ mysql_real_escape is only safe when set the encoding like mysql_set_charset('UTF-8'); RegEx doesn't seem safe either. \$\endgroup\$ – Daniel W. Aug 31 '14 at 15:53
  • 2
    \$\begingroup\$ You’re calling html_entity_decode which is basically the inverse of htmlentities. \$\endgroup\$ – Gumbo Aug 31 '14 at 15:58
10
\$\begingroup\$

PHP already has a function for escaping HTML characters. htmlspecialchars().

In order to completely and absolutely prevent XSS, all you need to do is pass anything that's about to be echoed on a page through that function. So for example:

<ul>
<?php foreach ($items as $item) : ?>
    <li><?= htmlspecialchars($item); ?></li>
<?php endForeach; ?>
</ul>

This code is 100% XSS proof. And it doesn't matter what $items has in it. Note that with this method you do not escape for HTML before you insert to the database. Always escape as late as possible.

A note: This is about escaping HTML. It will not help you escape things like JavaScript or URLs. The following will not be escaped properly:

<a onclick="<?php htmlspecialchars($something); ?>">Whatever</a>
\$\endgroup\$
  • 1
    \$\begingroup\$ Certain parts of a page have different escaping rules to others. owasp.org/index.php/… \$\endgroup\$ – Laurence Aug 31 '14 at 16:02
  • \$\begingroup\$ If you're talking about HTML (including attributes), htmlspecialchars() is a good catch-all. Obviously, it doesn't apply for URLs, JavaScript or CSS, you'll have to escape those with their respective functions/libraries. \$\endgroup\$ – Madara Uchiha Aug 31 '14 at 16:03
  • \$\begingroup\$ I'm not sure it's obvious to the person asking the question. \$\endgroup\$ – Laurence Aug 31 '14 at 16:05
  • \$\begingroup\$ Good point. Editing. \$\endgroup\$ – Madara Uchiha Aug 31 '14 at 16:05
1
\$\begingroup\$

As others mentioned it is very hard to create a universal cleanup function.

The "cleanness" is a context dependant condition.

For example if you use a variable as a html tag attribute then apostrophes are probably should be escaped, otherwise not necessary.

I think at least the following variable use cases exists:

  • used as html
  • used as an attribute (for example input... value="XXX")
  • used in a textarea
  • used for assigning value to javascript variables.

Maybe not each applies to your situation, but generally these can be possibly different.

Otherwise using bb tags is usually considered safe and flexible enough if you want to enable some kind of formatting . There are existing libraries for the conversion. If you don't want enable any html, then strip_tags and/or htmlspecialchars can be your friends depending the context.

\$\endgroup\$
  • \$\begingroup\$ but we can try at least to make some function like that. \$\endgroup\$ – Faiz Rasool Aug 31 '14 at 16:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy