1

I realized that the following part of my application has potential for MySQL injection.

$id= $_GET['id'];
$query = "SELECT * FROM clients WHERE id = $id";
//run query

Is it enough security for me to check is_numeric($id) before running the MySQL query? Or is it necessary for me to re-write my code using prepared statements?

tanios
  • 53
  • 5
  • 5
    Yes, you should re-write using prepared statements. Then it's write-and-forget, v.s. write-and-ponder-if-you-thought-of-every-possible-attack-vector. – Marc B Jan 27 '15 at 20:56
  • 1
    If you are disciplined about using prepared statements correctly you won't have SQL injection bugs. If you don't you probably have dozens of SQL injection bugs you're not aware of. – tadman Jan 27 '15 at 20:57
  • tadman and marc b thanks for explaining why using prepared statements are better. – tanios Jan 27 '15 at 21:00
  • It's worth testing your application with an [injection testing tool](http://sqlmap.org/), though be advised you may not be able to sleep at night until you've patched all the holes it's found. – tadman Jan 27 '15 at 21:04
  • Don't try to take the cheap way out. Learn to use prepared statements and bound parameters. http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php – Andy Lester Jan 27 '15 at 21:46

3 Answers3

1

You SHOULD use prepared statements as mysql_* functions are deprecated. That being said, a numeric value would make that SQL statement definitively safe as SQL injections require the variable to actually have SQL like statements in them.

An example would be 1;-

thatidiotguy
  • 8,701
  • 13
  • 60
  • 105
  • This is what I was thinking, but I'm new to this so I wanted to make sure... thank you for the quick answer. – tanios Jan 27 '15 at 20:58
0

If you don't have time to rewrite your queries using prepared statements, I would say I'd have more confidence in casting the user input.

$id = (int) $_GET['id'];

Since this variable is now an int, there is no way it can contain malicious input. Of course, you should still do any necessary range validation on it (e.g. if negative numbers should be disallowed).

I've assumed this column is an integer, but (float) can be used in the same way here, for data that is numeric but not integer.

For the avoidance of doubt, parameter binding is still the best approach to injecting user input into your queries. My answer here is intended to answer the thrust of the question directly i.e. is there a way to make queries safe without binding? The answer above shows that the answer is yes.

halfer
  • 19,824
  • 17
  • 99
  • 186
  • This is like putting a bandage on someone suffering from a heart attack. Waving the `(int)` wand over your problem doesn't make it go away, it's a narrow solution for one particular type of data. Just pass this through to the wickedly ugly `mysql_real_escape_string` if it's not escaped properly or you'll be creating more problems than you'll solve. – tadman Jan 27 '15 at 20:56
  • @tadman: can you give an example of the kind of problem that might result from this? Yes, you can't do this for non-integer values - the question's premise is specifically about numbers. – halfer Jan 27 '15 at 20:58
  • 1
    If `$_GET['id'] == "abcd"` and you're type converting to INT then the SQL query will fail. There still needs to be a sanity check. Prepared statements are the only way to go at this point. – Rob W Jan 27 '15 at 21:00
  • @halfer Instead of having some arbitrary way of dealing with int, another for floats, a different one for dates, and yet another for strings, you need one simple method that's able to consistently handle them. That method is prepared statements and placeholders. If that's not available, escape it the old way. – tadman Jan 27 '15 at 21:02
  • @HalfCrazed: I'm not sure I understand. If a value is expected to be numeric, why would you compare it to a value it can (or should) never have? – halfer Jan 27 '15 at 21:03
  • @tadman: I agree that binding is the best protection method, so I've updated the answer to reflect that. I agree also that escaping or binding is more consistent across different types. Nevertheless, I interpreted the question as excluding these standard approaches, and the answer remains that if you cast an integer, it is safe. – halfer Jan 27 '15 at 21:12
  • @HalfCrazed: [in response to your now deleted comment] your assumption about casting is incorrect. If you cast 'abcd' to an int, it returns zero, so the query will remain valid. (The handy thing with primary keys is they usually start at 1, so the zero becomes a handy 'not found'). – halfer Jan 27 '15 at 22:17
  • 1
    My mistake ;) It's been a while. int(0) is correct, though that should still be checked. – Rob W Jan 27 '15 at 22:19
0

It will reduce the attack surface if you restrict the parameters to just an integer but you also want to validate the parameter on the server side (don't leave the security checks on the client-side since they client can bypass those very easily).

For application security (and especially PHP security since it is riddled with security headaches) it is always a good idea to consider that all user-supplied data is malicious. So double check that all parameters fit into the criteria expected before allowing the application to act on that data and process it.

You will do yourself a favor by using PHP prepared SQL statements as those are an added mechanism (on top of your own validation) to significantly reduce the injection attack surface.

Here is a resources to get you more familiar with PHP prepared statements:

http://php.net/manual/en/mysqli.quickstart.prepared-statements.php

The idea is that it avoids injection attacks by specifying what kind of datatype the parameter is and builds a safer query string for you. But again - always inspect incoming/user-supplied data before processing it further.

Max Worg
  • 2,932
  • 2
  • 20
  • 35
  • Please [do not link to w3schools](http://w3fools.com/). It's an out of date resource that is full of badly coded examples that encourage dangerously bad habits. That site has done enough damage already. – tadman Jan 27 '15 at 21:04
  • 1
    Ah. I was unaware. Thanks for the heads-up. I removed the link as suggested. Please consider removing the downvote since I acted on your suggestion. – Max Worg Jan 27 '15 at 21:06
  • Thanks for cleaning that up. Helps a lot. – tadman Jan 27 '15 at 21:07