XSS Filter - Please Critique

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

XSS Filter - Please Critique

Postby php3ch0 » Fri Jan 27, 2012 12:35 pm

I have a function that I am using to hopefully make sure that data is safe.

Syntax: [ Download ] [ Hide ]
function xssfilter($data) {
       
        if(!get_magic_quotes_gpc()) {
                $data = addslashes($data);
                    }
                        $data = strip_tags(htmlspecialchars($data));
                       
        $data = mysql_real_escape_string($data);
       
        return $data;
       
}
 

I use it as such

$var = xssfilter($_POST['var']);

Does this look secure enough?
User avatar
php3ch0
Forum Contributor
 
Posts: 212
Joined: Sun Nov 13, 2005 8:35 am
Location: Folkestone, Kent, UK

Re: XSS Filter - Please Critique

Postby tr0gd0rr » Fri Jan 27, 2012 2:04 pm

You are confusing XSS with SQL injection.

The simplest way to avoid XSS is to run htmlspecialchars() before echoing anything. I don't recommend putting anything in the database with htmlspecialchars already run. Wait until output. Some day you may want to send your database data via a web service or print a check. It looks pretty stupid if you print a check made out to "Mr. O'Reilly".

The exception would be rich text. If you are accepting rich text like a blog post, you should run it through a full HTML parser and cleaner such as HTMLPurifier before putting it in the database.

As far as SQL injection, before putting anything into the database always use PDO binding or mysql_real_escape_string(). And addslashes() is unnecessary. In fact, addslashes() is rarely useful.
User avatar
tr0gd0rr
Forum Contributor
 
Posts: 305
Joined: Thu May 11, 2006 8:58 pm
Location: Utah, USA

Re: XSS Filter - Please Critique

Postby social_experiment » Fri Jan 27, 2012 5:43 pm

If you are going to use the code, get rid of the magic quotes checks and apply filtering regardless of it; magic_quotes_gpc has been deprecated and shouldn't be relied upon at all.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
social_experiment
DevNet Master
 
Posts: 2775
Joined: Sun Feb 15, 2009 12:08 pm
Location: .za

Re: XSS Filter - Please Critique

Postby Celauran » Fri Jan 27, 2012 5:45 pm

social_experiment wrote:If you are going to use the code, get rid of the magic quotes checks and apply filtering regardless of it; magic_quotes_gpc has been deprecated and shouldn't be relied upon at all.

Though at that point all you're left with is a mysql_real_escape_string wrapper function.
PHP 5.6 released! August 28, 2014
PSA: PHP 5.3 end of life August 14, 2014.
User avatar
Celauran
Moderator
 
Posts: 3493
Joined: Tue Nov 09, 2010 3:39 pm
Location: Montreal, Canada

Re: XSS Filter - Please Critique

Postby social_experiment » Sat Jan 28, 2012 3:22 am

I should have probably worded that better :) I mean he can leave the checks for magic_quotes_gpc() out; since it's no longer useful but still keep the other filters in place. This strip_tags(htmlspecialchars($data)); is redundant as htmlspecialchars() will make sure strip_tags() find nothing to strip, i would change it like this:
Syntax: [ Download ] [ Hide ]
<?php
function xssfilter($data) {        
        $data = addslashes($data);
        #
       $data = strip_tags($data);
         #              
       $data = mysql_real_escape_string($data);
        #
       return $data;        
}
?>
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
social_experiment
DevNet Master
 
Posts: 2775
Joined: Sun Feb 15, 2009 12:08 pm
Location: .za

Re: XSS Filter - Please Critique

Postby php3ch0 » Sun Jan 29, 2012 6:30 am

Thanks Will do.
User avatar
php3ch0
Forum Contributor
 
Posts: 212
Joined: Sun Nov 13, 2005 8:35 am
Location: Folkestone, Kent, UK

Re: XSS Filter - Please Critique

Postby tr0gd0rr » Mon Jan 30, 2012 10:59 am

@social_experiment, your function probably behaves different than you are thinking. If you pass the string "Jane's <3 is Joe", you will get out of the database "Jane\'s ". Note how strip_tags() cuts off strings with less than signs in the middle. But if you run mysql_real_escape_string() alone on the way in, and htmlspecialchars() before displaying to the browser, the string will look identical.
User avatar
tr0gd0rr
Forum Contributor
 
Posts: 305
Joined: Thu May 11, 2006 8:58 pm
Location: Utah, USA

Re: XSS Filter - Please Critique

Postby social_experiment » Mon Jan 30, 2012 4:27 pm

Thanks for pointing that out tr0gd0rr :) I personally don't use strip_tags() [and it shows doesn't it]; if you catch " or ' (which mysql_real_escape_string() does) and use htmlspecialchars() when displaying data then strip_tags() becomes redundant
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
social_experiment
DevNet Master
 
Posts: 2775
Joined: Sun Feb 15, 2009 12:08 pm
Location: .za

Re: XSS Filter - Please Critique

Postby Mordred » Wed Feb 01, 2012 7:50 am

Syntax: [ Download ] [ Hide ]
$data = htmlspecialchars($data, ENT_QUOTES, 'utf8');


None of the other listed options so far will work correctly:
strip_tags() is of dubious value and will damage the original data,
mysql_real_escape_string() is about MySQL and SQL injection, not XSS
addslashes() is just silly
htmlspecialchars($data) is insufficient protection when used in attribute context.
Things need not have happened to be true. Tales and dreams are the shadow-truths that will endure when mere facts are dust and ashes, and forgot.
Image
My security blog. (not updated lately)
The Unexpected SQL Injection (article) (.txt, cause the .html version is broken)
Password hashing howto (and how not to) (article)
Salt strengths (article)
User avatar
Mordred
DevNet Resident
 
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: XSS Filter - Please Critique

Postby tr0gd0rr » Wed Feb 01, 2012 10:26 pm

Mordred wrote:htmlspecialchars($data) is insufficient protection when used in attribute context.

What do you mean exactly? Because an attribute might contain some JavaScript? Or how is it insufficient?
User avatar
tr0gd0rr
Forum Contributor
 
Posts: 305
Joined: Thu May 11, 2006 8:58 pm
Location: Utah, USA

Re: XSS Filter - Please Critique

Postby Mordred » Thu Feb 02, 2012 5:41 am

If it's an attribute and you use the 'wrong' type of quotes, plain htmlspecialchars() will fail to protect against XSS, read the docs. It's amazing how stupid some of the PHP functions are - it's a security measure which is insecure by default!
Things need not have happened to be true. Tales and dreams are the shadow-truths that will endure when mere facts are dust and ashes, and forgot.
Image
My security blog. (not updated lately)
The Unexpected SQL Injection (article) (.txt, cause the .html version is broken)
Password hashing howto (and how not to) (article)
Salt strengths (article)
User avatar
Mordred
DevNet Resident
 
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: XSS Filter - Please Critique

Postby social_experiment » Thu Feb 02, 2012 5:57 am

So a function such as htmlentities() with ENT_QUOTES and charset should be the default if you wish to properly safeguard against XSS?
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
social_experiment
DevNet Master
 
Posts: 2775
Joined: Sun Feb 15, 2009 12:08 pm
Location: .za

Re: XSS Filter - Please Critique

Postby Mordred » Thu Feb 02, 2012 8:14 am

Mordred wrote:
Syntax: [ Download ] [ Hide ]
$data = htmlspecialchars($data, ENT_QUOTES, 'utf8');


Things need not have happened to be true. Tales and dreams are the shadow-truths that will endure when mere facts are dust and ashes, and forgot.
Image
My security blog. (not updated lately)
The Unexpected SQL Injection (article) (.txt, cause the .html version is broken)
Password hashing howto (and how not to) (article)
Salt strengths (article)
User avatar
Mordred
DevNet Resident
 
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria


Return to Coding Critique

Who is online

Users browsing this forum: No registered users and 2 guests