7

I am currently using PDO with prepared statements for some integer values (see PDO::PARAM_INT). That means I call PDO like this:

$stmt = $conn->prepare("SELECT `lastMove` FROM ".GAMES_TABLE.
                " WHERE `id`=:gameID AND (`whiteUserID` = ".USER_ID.
                " OR `blackUserID` = ".USER_ID.") LIMIT 1");
$stmt->bindValue(':gameID', $_POST['gameID'], PDO::PARAM_INT);
$stmt->execute();

Do I have to make any checks on $_POST['gameID']? Which vulnerabilities might exist?

PDO and integers

Krzysztof Kotowicz made a great comment. He said that PDO treats integers as Strings. So I enabled the MySQL general_log:

  1. open /etc/mysql/my.cnf
  2. set general_log to 1 and add the path to general_log_file (e.g. /var/log/mysql/mysql.log)
  3. Restart your MySQL server: sudo /etc/init.d/mysql restart
  4. Take a look at the file: tail -f /var/log/mysql/mysql.log
  5. Call the following snippet:

test.php:

$stmt = $conn->prepare("SELECT `lastMove` FROM ".GAMES_TABLE.
                " WHERE `id`=:gameID AND (`whiteUserID` = :uid".
                " OR `blackUserID` = :uid) LIMIT 1");
$stmt->bindValue(':gameID', "abc", PDO::PARAM_INT);
$stmt->bindValue(':uid', "1'", PDO::PARAM_INT);
$stmt->execute();
$result = $stmt->fetch(PDO::FETCH_ASSOC);

This is the result:

111114 17:32:54   241 Connect   chessuser@localhost on chess
          241 Query SELECT `user_id` FROM `chess_users` WHERE `user_id`='1'  LIMIT 1
          241 Query SELECT `lastMove` FROM chess_games WHERE `id`='abc' AND (`whiteUserID` = '1\'' OR `blackUserID` = '1\'') LIMIT 1
          241 Quit  

So obiously it doesn't make it to an int on the PHP-side (PHP Version 5.3.2-1ubuntu4.10)

Martin Thoma
  • 3,902
  • 6
  • 30
  • 42

1 Answers1

7

First off: You're not making full use of prepared statements. That could expose you to to SQL injection problems.

You should not be using string concatenation to build up the argument to $conn->prepare(). Instead, you should be using a string constant as the SQL query, and relying upon bindValue() to fill in dynamic values. In an ideal world you could write something like this:

$stmt = $conn->prepare("SELECT `lastMove` FROM :table".
            " WHERE `id`=:gameID AND (`whiteUserID` = :userid".
            " OR `blackUserID` = :userid) LIMIT 1");
$stmt->bindValue(':gameID', $_POST['gameID'], PDO::PARAM_INT);
$stmt->bindValue(':table', GAMES_TABLE, PDO::PARAM_STR);
$stmt->bindValue(':userid', USER_ID, PDO::PARAM_STR);
$stmt->execute();

However, PDO prepared statements don't let you specify the table name in this way, so you're forced to use string concatenation for the table name, like this:

$stmt = $conn->prepare("SELECT `lastMove` FROM ".
            mysql_real_escape_string(GAMES_TABLE).
            " WHERE `id`=:gameID AND (`whiteUserID` = :userid".
            " OR `blackUserID` = :userid) LIMIT 1");
$stmt->bindValue(':gameID', $_POST['gameID'], PDO::PARAM_INT);
$stmt->bindValue(':userid', USER_ID, PDO::PARAM_STR);
$stmt->execute();

Using string concatenation with any non-constant string to build up the first parameter to prepare() introduces a danger of SQL injection attacks, so you need to be careful here. You need to check carefully that GAMES_TABLE is a constant and can never be influenced by the attacker (you might consider inlining it). Also, you might want to sanitize/escape it just in case, as in the code I've shown above. It is an annoying limit of prepared statements that they can't handle this for you.

Rule of thumb: prepared statements are safe from SQL injection, as long as the SQL query itself is a constant string.

Unfortunately, you may discover a few cases where prepared statements are too limiting, such as dynamic ORDER BY or LIMIT parameters -- as well as dynamic table names, as illustrated above. For these cases, you will probably need to use string concatenation. In these cases, you must be extremely careful, as using string concatenation re-introduces the risk of SQL injection vulnerabilities.

In any case, you should still use prepared statements for everything you possibly can, such as for USER_ID, as it is less error-prone than doing the string concatenation yourself. Also, because prepared statements make it easier to verify you have avoided SQL injection flaws, it is to your benefit to use prepared statements instead of string concatenation everywhere you can.

Additional checks: To answer your original question, if PDO implemented its API properly, you would not need any other checks on an integer value to prevent SQL injection problems. Unfortunately, @Krzysztof Kotowicz has pointed out that PHP PDO library is buggy: it ignores the PDO::PARAM_INT argument and treats everything as a string, regardless. Therefore, you probably ought to do the extra coding that's needed to make up for this flaw in the PDO library. In particular, you should coerce (cast) the value to an integer, before passing it. Thus, I suggest something like the following:

// don't use $_POST['gameID'] elsewhere; use $gameID
$gameID = (int) $_POST['gameID'];

$stmt = $conn->prepare("SELECT `lastMove` FROM ".
            mysql_real_escape_string(GAMES_TABLE).
            " WHERE `id`=:gameID AND (`whiteUserID` = :userid".
            " OR `blackUserID` = :userid) LIMIT 1");
$stmt->bindValue(':gameID', $gameID, PDO::PARAM_INT);
$stmt->bindValue(':userid', USER_ID, PDO::PARAM_STR);
$stmt->execute();

Separately, you need to make sure that, if an attacker can guess a valid gameID, it does not help the attacker bypass any access control checks in your code. See, e.g., OWASP's insecure direct object reference vulnerability category.

For string values, it might also be prudent to sanitize the data to ensure it matches some regexp that is appropriate for that value.

D.W.
  • 98,420
  • 30
  • 267
  • 572
  • 2
    Hi D.W., as far as I know I can't use prepared statements for the table, see http://stackoverflow.com/questions/8100844/how-can-i-use-the-same-prepared-statement-for-different-tables-in-pdo – Martin Thoma Nov 14 '11 at 12:20
  • 2
    I used to check the TCP traffic for PDO mysql driver about one year ago, and I found out it actually ignores parameter types, treating all of them as strings. I don't know if they fixed that, but I'd recommend casting $_POST['gameID'] to int anyway. ( $_POST['gameID'] = (int) $_POST['gameID']). – Krzysztof Kotowicz Nov 14 '11 at 12:44
  • 1
    @moose: Iff GAMES_TABLE is a variable that is *impossible* for the user to modify, you can safely concatenate that into the prepared statement. – bstpierre Nov 14 '11 at 14:07
  • @bstpierre: I guessed that already. It is impossible for the user to change that value, as it is assigned directly in one file of the code, no matter what the user does. Nice blog, by the way. I didn't know how to use Pythons ctypes. – Martin Thoma Nov 14 '11 at 14:21
  • @Kryzysztof Kotowicz: I got this one as I took a look at the MySQL-log: (added it to my post to make it clearer) – Martin Thoma Nov 14 '11 at 16:24
  • Thank you, moose, @Krzysztof Kotowicz! Great points, very helpful feedback. I've updated my answer accordingly. – D.W. Nov 14 '11 at 17:34