CODEWITHSUNDEEP
phpmysqlsql-injection
bmaster
bmaster

Reputation: 2373

Is this a safe way to secure against sql injection?

Is this code 100% safe from sql injection:

$id = $_GET['id']
mysql_query('SELECT * FROM mytable WHERE id < ' . (int)$id);

or do I have to do this?

$id = $_GET['id']
mysql_query('SELECT * FROM mytable WHERE id < ' . mysql_real_escape_string($id));

Upvotes: 4

Views: 585

Answers (7)

Johan
Johan

Reputation: 76753

No
you have to do this:

$id = mysql_real_escape_string($_GET['id']);
//put the escaped string in a $var, so your select statement stays readable
//this will help in debugging, and make **not** forgetting those vital quotes
//easier.
$query = "SELECT * FROM mytable WHERE id < '$id'";
//                                         ^   ^always single quote your $vars
//       ^                                      ^ and double quote the query 
$result = mysql_query($query);
//and test to see if your query ran successful.
if (!$result) { //your query gave an error, handle it gracefully.

Then you're safe.

Upvotes: 0

Marc B
Marc B

Reputation: 360872

The query could still blow up if $_GET['id'] is empty, or (int)$_GET['id'] evaluates to empty. You'd end up with a syntax error in the query. It's not enough to blindly escape or type-cast a value and stuff it into a query. You have to check that the final "safe" value is actually safe and not just a wolf in grandma's clothes.

Upvotes: 2

time4tea
time4tea

Reputation: 2197

You should use parameterised queries. Then you don't need to worry about all that escaping. It also makes the SQL much easier to read. Oh and don't use select *, just select what you want.

Upvotes: 0

mvds
mvds

Reputation: 47114

This

mysql_query('SELECT * FROM mytable WHERE id < ' . mysql_real_escape_string($id));

would be bad practice. If you want it to be a string, at least quote the string:

mysql_query('SELECT * FROM `mytable` WHERE `id`<"'.mysql_real_escape_string($id)).'"';

(and while you're at it, quote all field and table names as well, for things like id might be or become reserved keywords at some point)

I would prefer the cast, if it is an integer. One argument for the string version would be that some day the id might be alphanumeric (as seen more and more often on a lot of websites).

Upvotes: 2

Jaime Garcia
Jaime Garcia

Reputation: 7116

This article seems to be a good one in explaining how mysql_real_escape_string can protect you from SQL Injection, but it also explains its "holes"

http://www.webappsec.org/projects/articles/091007.shtml

Upvotes: 2

Amir Raminfar
Amir Raminfar

Reputation: 34179

It is safe but you probably want to do 

if(is_int($id)){
mysql_query('SELECT * FROM mytable WHERE id < ' . mysql_real_escape_string($id)); // just to be safe always to escape
}

Upvotes: -1

Cesar
Cesar

Reputation: 3519

I use sprintf, mysql_query(sprintf('SELECT * FROM mytable WHERE id < %d', $id));

Upvotes: 0

Related Questions