SQL injection: Is this line safe?

Question

I have this line:

$sql = "UPDATE votes SET up=up+1 WHERE id='{$p}'";

Now from what I've read one way of sql injection is caused by not "closing" sql queries properly which allows hackers to add additional info.

So my question is, is that line safe as to me the up=up+1 has not been "closed" but if I set it like this up='up+1' which to me makes it "closed" it does not work.

row up type is int(11) if that makes any difference.

Update:

$p is sanitized with a function

function sanitize($foo) { if(get_magic_quotes_gpc() == true) { $foo = stripslashes($foo); } return mysql_real_escape_string(htmlspecialchars($foo)); }

That statement looks pretty safe to me except you just need to make sure $p is always number before this statement. — Jonas T
– Jonas T, Commented May 11, 2012 at 1:49
Never sanitize. Escape. sanitizing depends on you knowing every possible route of attack and filtering specifically for it. Anytime a new attack route is discovered, your function is useless. Whereas escaping handles ALL attacks. — Marc B
– Marc B, Commented May 11, 2012 at 1:49
If you are concerned about SQL injection attacks, then you should limit database access through stored procedures and views. That said, the particular query looks pretty safe, especially since $p is checked by a function. — Gordon Linoff
– Gordon Linoff, Commented May 11, 2012 at 1:50
@GordonLinoff parameterized queries can offer the same "security" that stored procedures can. — jordanm
– jordanm, Commented May 11, 2012 at 1:51
@Marc B: Sorry I think I may have used the wrong word "sanitized" thats just what I call my function (see op edit) — Anna Riekic
– Anna Riekic, Commented May 11, 2012 at 1:57

Michael Berkowski · Accepted Answer · 2012-05-11 01:47:49Z

The up=up+1 is not vulnerable because it does not accept user input via a variable. It is merely incrementing an INT value already in your database.

The WHERE id='{$p}' may be vulnerable, however, if the variable $p is not properly filtered and escaped, but as you added above, it has been sanitized by a function. Hopefully the sanitize function goes as far as checking the appropriate type of the variable (be it an int or string or whatever) and checks the appropriate bounds for it.

As always, the safest method is to make use of prepared statements and parameterized queries rather than pass any variables directly into a SQL string. Most of the database APIs available in PHP for the various RDBMS' support prepared statements. (Notably not the mysql_*() functions, however).

Thanks, like I said in op $p has been sanitized using a function function sanitize($foo) { if(get_magic_quotes_gpc() == true) { $foo = stripslashes($foo); } return mysql_real_escape_string(htmlspecialchars($foo)); }

Jonathan Hall · Accepted Answer · 2012-05-11 01:51:39Z

It is not safe, unless $p is properly escaped. Otherwise imagine...

$p = "foo'; DROP TABLE votes; SELECT '1"

Then you'd end up executing something like:

UPDATE votes SET up=up+1 WHERE id='foo'; DROP TABLE votes; SELECT '1';

That wouldn't be very pretty...

Collectives™ on Stack Overflow

SQL injection: Is this line safe?

2 Answers 2

1 Comment

Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

Comments

Related