Search code examples
phpmysqlsqlsql-injection

Casting variables to integers in SQL queries in PHP


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

Solution

  • 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.