Am I doing everything I can to prevent SQL Exploits with this code?

Hi everyone

I know I am keeping you busy with my questions but I think I am learning so I am going to keep on with it

So, with all the threat of SQL exploits I would like to check with those better at PHP that I am correctly protecting myself in this script.

Every value that is sent to the database is run through this snippet:

function db_prepare($value)
{
	$magic 			= get_magic_quotes_gpc();

	if($magic == 1)
	{
		return $value;
	}
	else
	{
	addslashes($value);
	return $value;	
	}
	
}

$variable = "Some potentially evil code";
$safeVariable = mysql_real_escape(db_prepare($variable));

//Then $safeVariable is sent to the DB

PHP:

Am I doing it correctly? I didn't know whether real_escape allowed for Magic Quotes so I decided to be sure.

Thanks (and +REP for everyone who helps as always)
Rory

 

addslashes() isn't safe enough. And mysql_real_escape() isn't a function (unless you defined it). And if magic quotes were enabled using your code above, then you'd end up double escaping the data.

Take a look at the "best practice" example on this page.

If magic quotes are enabled, strip them off. And then use mysql_real_escape_string().

 

Sorry, I was in a rush when I wrote that code (it's not the one in my script where I do actually use mysql_real_escape_string). Looking at your "best practice" example I am a bit confused by this bit.

$query = sprintf("INSERT INTO products (`name`, `description`, `user_id`) VALUES ('%s', '%s', %d)",
                    mysql_real_escape_string($product_name, $link),
                    mysql_real_escape_string($product_description, $link),
                    $_POST['user_id']);

PHP:

I get that the bit's with % in front of them are then defined afterwards. Why are 2 %s and 1 %d though?

 

That's explained at the sprintf() manual page.

However, I wasn't referring to this part anyway. I meant this part:

if(get_magic_quotes_gpc()) {
            $product_name        = stripslashes($_POST['product_name']);
            $product_description = stripslashes($_POST['product_description']);
        } else {
            $product_name        = $_POST['product_name'];
            $product_description = $_POST['product_description'];
        }

PHP:

... don't rely on magic quotes, as they're not safe. If they've been added by PHP, strip them off and then apply mysql_real_escape_string().

 

Okay

Thanks for putting up with me, you have earned your Rep

 

You can also disable magic quotes with .htaccess.

http://us2.php.net/manual/en/security.magicquotes.php#61589

There's also functions/classes out there that you have run automatically on your GPC variables that will strip slashes for you, so you know you'll always be working with slashless data. Most frameworks that I've used do this.

There's some code in the manual to do this:

http://us2.php.net/manual/en/security.magicquotes.php#61188

 

I just noticed on the manual page that MQ is being removed in PHP 6.0. When is that scheduled anyway?

 

I don't know. The developers knew about the problem that magic quotes causes years ago. I think it was just left in PHP5 for backwards compatibility. It's great news that PHP 6 is dumping it.