Good SQL Injection and XSS Prevention code

[themonkey40]

I am doing a lot of CRUD (Create, Read, Update, Delete) on a site I am building for my company. I want to be sure and prevent both sql injection and XSS. I have been reading some books on the matter and found this bit of code. What do you think, good?

<?php
$user  = mysql_entities_fix_string($_POST['user']);
$pass  = mysql_entities_fix_string($_POST['pass']);

$query = "SELECT * FROM users WHERE user='$user' AND pass='$pass'";

function mysql_entities_fix_string($string)
{
    return htmlentities(mysql_fix_string($string));
}    

function mysql_fix_string($string)
{
    if (get_magic_quotes_gpc()) $string = stripslashes($string);
    return mysql_real_escape_string($string);
}
?>

I have been looking over it for awhile and understand it for the most part, except, it refers to the variable $string. I don't see how the variable $string is at all referenced in:

$user  = mysql_entities_fix_string($_POST['user']);
$pass  = mysql_entities_fix_string($_POST['pass']); 

Any help understanding how the variable $string is referenced would be appreciated.

[NogDog]

$string is the function argument for each of the two user-defined functions in that code. In each case its scope is local only to the function where it is being used.

[themonkey40]

NogDog -- Thanks for that explanation. So $string is just the local variable to each of the functions. When I call the function:

mysql_entities_fix_string($_POST['user']); 

$string than equals $_POST['user']? I think thats right.

What do you think of the code from a security stand point?

[NogDog]

Yes, that's correct.

As far as security, the part that does the mysql_real_escape_string() takes care of any issues with SQL injection. As far as XSS and other similar concerns, simply applying a blanket solution of applying htmlentities() may not always be best. Depending on the particular data element in question, it might be better to validate it against a "white list" of allowed characters and return an error if it's invalid, rather than just blindly changing the data. Or if it's text that may need to be searchable, applying HTML character entities to it before storing it in the database could make things confusing when manipulating that data, or even make it too long to fit in a column that would otherwise hold it. (In this case it might make more sense to apply htmlentities() to the data after retrieving it from the DB and outputting it to the user.)

So read the link I pointed to above and think about what you need to do specifically with each field to ensure that it is "safe," as opposed to simply applying one blanket solution to all fields. In fact, I'd strongly suggest getting your hand's on that author's book: Essential PHP Security.

[themonkey40]

That some great advice. I was actually reading about that book last night on my iphone. It is currently an app you can by. Will look into it.