Should I sanitize inputs to a parametrized query?

Question

I have a couple of basic questions on parametrized queries

Consider this code:

$id = (int)$_GET['id'];
mysql_query("UPDATE table SET field=1 WHERE id=".$id);

Now the same thing using a parametrized query

$sql = "UPDATE table SET field=1 WHERE id=?";
$q = $db->prepare($sql); 
$q->execute(array($_GET['id']));

My questions are:

1. is there any situation where the first code (i.e. with the (int) cast) is unsafe?
2. is the second piece of code OK or should I also cast $_GET['id'] to int?
3. is there any known vulnerability of the second piece of code? That is, is there any way an SQL attack can be made if I am using the second query?

Answer 1

1. is there any situation where the first code (i.e. with the (int) cast) is unsafe?

   I'm not a PHP expert, but I think there shouldn't be. That's not to say that PHP doesn't have bugs (either known or yet to be discovered) that could be exploited here.

2. is the second piece of code OK or should I also cast $_GET['id'] to int?

   Likewise, the second piece of code should be absolutely fine - even if the data type was a string, MySQL would know not to evaluate it for SQL as it's a parameter and therefore only to be treated as a literal value. However, there's certainly no harm in also performing the cast (which would avoid any flaws in MySQL's handling of parameters) - I'd recommend doing both.

   EDIT - @Tomalak makes a very good point about cast potentially resulting in incorrect data and suggests first verifying your inputs with sanity checks such as is_numeric(); I agree wholeheartedly.

3. is there any known vulnerability of the second piece of code? That is, is there any way an SQL attack can be made if I am using the second query?

   Not to my knowledge.

Answer 2

1. (int) will yield 0 when the conversion fails. This could lead to updating the wrong record. Besides, it's sloppy and an open invitation to "forget" proper type casting when the query gets more complex later-on.
   It's safe in its current form (against SQL injection, not against updating the wrong record) but I'd still not recommend it. Once the query gets more complex you're bound to use prepared statements anyway, so just do it right from the start - also for the sake of consistency.

2. That's sloppy, too. The parameter will be transferred to the DB as a string and the DB will try to cast it. It's safe (against SQL injection), but unless you know exactly how the DB server reacts when you pass invalid data, you should sanitize the value up-front (is_numeric() and casting).

3. No. (Unless there is a bug in PDO, that is.)

As a rule of thumb:

• Don't pass unchecked data to the database and expect the right thing to happen.
• Don't knowingly pass invalid data and trust that the other system reacts in a certain way. Do sanity checks and error handling yourself.
• Don't make "Oh, that converts to 0 and I don't have a record with ID 0 anyway so that's okay." part of your thought process.