Casting variables to integers in SQL queries in PHP

Question

First of all, I am fully aware of SQL injection vulnerabilities and I am using PDO for newer applications that I am developing in PHP.

Long story short, the organization that I'm working for cannot afford to delegate any human resources at the moment to switch everything over to PDO for the rather large application that I'm currently working on, so I'm stuck with using mysql_* functions in the meantime.

Anyways, I am wondering if it is safe to use data validation functions to "sanitize" numeric parameters used in the interpolated queries. We do use mysql_real_escape_string() for strings (and yes I am aware of the limitations there too). Here is an example:

public function foo($id) {
    $sql = "SELECT * FROM items WHERE item_id = $id";
    $this->query($sql); // call mysql_query and does things with result
}

$id id a user-supplied value via HTTP GET so obviously this code is vulnerable. Would be OK if I did this?

public function foo($id) {

    if (!ctype_digit($id)) {
        throw new \InvalidArgumentException("ID must be numeric");
    }

    $sql = "SELECT * FROM items WHERE item_id = $id";
    $this->query($sql); // call mysql_query and does things with result
}

As I'm aware, ctype_digit is the same as checking against a regular expression of \d+.

(There's also filter_var($id, FILTER_VALIDATE_INT), but that can potentially return int(0) which evaluates to FALSE under loosely-typed comparisons, so I'd have to do === FALSE there.)

Are there any problems with this temporary solution?

Update:

• Variables do not only include primary keys, but any field with type boolean, tinyint, int, bigint, etc., which means that zero is a perfectly acceptable value to be searching for.
• We are using PHP 5.3.2

Answer 1

Yes, if you indeed religiously use the correct function to validate the data and correctly prevent the query from running if the data is not as expected, there's no vulnerability I can see. ctype_digit has a very limited and clear purpose:

Returns TRUE if every character in the string text is a decimal digit, FALSE otherwise.

There's basically nothing that can go wrong with this function, so it's safe to use. It will even return false on an empty string (since PHP 5.1). Note that is_numeric would not be so trustworthy. I would possibly still add a range check to make sure the number is within an expected range, I'm not sure what could happen with overflowing integers. If you additionally cast to (int) after this check, there's no chance of injection.

Caveat emptor: as with all non-native parameterised queries, there's still a chance of injection if you're getting into any shenanigans with connection charsets. The range of bytes that may slip through are severely limited by ctype_digit, but you never know what one could come up with.

Answer 2

Yes, it will work. Your code will raise an exception if the value isn't a numeric string, you'll just have to catch that and display an error message to the user.

Beware that ctype_digit($foo):

• Will return true if $foo is empty before PHP 5.1 (see the doc) ;
• Will return false for all int values of $foo outside of the [48, 57] interval (ASCII numbers).

So you'll also need to check that $foo is a non-empty string if you plan on using ctype_digit($foo)

Answer 3

Long story short, the organization that I'm working for cannot afford to delegate any human resources at the moment to switch everything over to PDO

I don't see where is the problem here. According to the code you posted, you are already using some sort of DB wrapper, and already planning to alter the calling code for the every numeric parameter. Why not to alter that DB wrapper to make it support prepared statements, and alter calling code to employ it?

Old mysql ext is not a problem - one can emulate prepared statements with it all right.

I am fully aware of SQL injection vulnerabilities.

Your "full awareness" is a bit exaggerated. Unfortunately, most people do not understand the real source of injection, as well as the real purpose of the prepared statement.

That thing with separating data from the query is a nice trick, but totally unnecessary one. While the real value of prepared statement is its inevitability, as opposed to essential arbitrariness of the manual formatting.

Another your fault is separated treatment for the strings - it is partly formatted in the query (adding quotes) and partly - outside (escaping special characters) which again is a call disaster.

As you decided to stick to the manual formatting, then enjoy your injections, sooner or later. Your ideas are good for the artificial, fully controlled sandbox example. However, things turn different in the real life application, with many people working on it. Instead of asking a program to format your data, you are asking people to do that. With all the obvious consequences.

It makes me wonder why PHP users unable learn from their mistakes, and still eagerly devising practices, that has been proved unreliable long time ago.

Just spotted another fallacy in your reasoning

a user-supplied value via HTTP GET so obviously this code is vulnerable.

You have to understand that any unformatted value makes this code vulnerable, no matter if its HTTP GET, FTP PUT or file read. It is not only notorious "user input" have to be properly formatted but any input. This is why it is essential to make DB driver the only place where formatting occurs. It should be not a developer who formats the data but a program. Your idea contradicts with such a cornerstone principle.