PDO/prep statement/whitelisting/set charset, is that safe enough to prevent injection?

Question

I am converting from extension mysql to PDO and after reading all I could from you gurus on Stack Overflow and elsewhere, I have some residual questions. I came up with the following to address SQL injection for a typical query. I am just wondering if that's enough or may be I am going a bit overboard with the whitelisting, before I replicate this to all my application.

It's not clear to me if I did the whitelisting properly, i.e., if I should also escape somehow.

Also, I am not sure if I should setAttribute emulate to false for every query or just once for the script.

$link = new PDO("mysql:host=$hostname;dbname=$database;charset=utf8", $username, $password);

$link->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

$arr_i = $arr_k = '';
$m_act = $v_act = 'Y';
$table = array('prices', 'versions', 'models');
$allowedTables = array('prices', 'versions', 'models');
$field = array('model_id', 'version_id', 'price', 'models.active', 'versions.active');
$allowedFields = array('model_id', 'version_id', 'price', 'models.active', 'versions.active');

if(count(array_diff($field, $allowedFields))==0 AND count(array_diff($table, $allowedTables))==0) {

    $sql = "SELECT COUNT(DISTINCT `" . $field[0] . "`) as ctmod FROM `" . $table[0] . "`
            INNER JOIN `" . $table[1] . "` USING (`" . $field[1] . "`)
            INNER JOIN `" . $table[2] . "` USING (`" . $field[0] . "`)
            WHERE `" . $field[2] . "` BETWEEN :arr_i AND :arr_k
            AND " . $field[3] . " = :m_act
            AND " . $field[4] . " = :v_act";

    $stmt = $link->prepare($sql);
    $stmt->bindParam(':arr_i', $arr_i, PDO::PARAM_INT);
    $stmt->bindParam(':arr_k', $arr_k, PDO::PARAM_INT);
    $stmt->bindParam(':m_act', $m_act, PDO::PARAM_STR);
    $stmt->bindParam(':v_act', $v_act, PDO::PARAM_STR);
    for ($i=0; $i < $ctpri; $i++) {
        $k = $i + 1;
        $arr_i = $arr_pri[$i] + 1;
        $arr_k = $arr_pri[$k];
        $stmt->execute();
        while ($r = $stmt->fetch(PDO::FETCH_ASSOC)) {
            $ctmod[] = $r['ctmod'];
        }
    }
}
else{
    die();
}

Answer 1

I suspect that you indeed are going a bit overboard with the whitelisting. Not only with whitelisting, but with prepared statements too. By protecting constant values, you overengineered your query, making it difficult to read.

You need to understand that any constant value is safe by design. Therefore there is absolutely no point in whitelisting or binding it. Just write it as is.

So, instead of

AND " . $field[3] . " = :m_act

you should write just

AND versions.active = 'Y'

without any binding or whitelisting.

While only dynamical values should be protected. Which means, in your particular situation, only $arr_i and $arr_k must be bound. All other query parts have to be written into the query directly, just like you did it before.

Answer 2

Yes, your code is thoroughly safe from SQL injection. Good job.

Though as @YourCommonSense points out, there's no reason in the example you show to make table and columns names into variables at all. It would be simpler to just write them into the query literally.

Therefore, I assume you're asking this question because you do sometimes choose table and column names through application logic or variables, even though you haven't shown it in this particular example.

---

The only tips I would offer are:

• All the string concatenation, with ending double-quotes, using . and re-starting double-quotes makes the code look untidy and it can be confusing to keep track of which double-quotes you've started and stopped. An alternative style of PHP string interpolation for variables is to enclose in curly braces, like the following:

  $sql = "SELECT COUNT(DISTINCT `{$field[0]}`) as ctmod FROM `{$table[0]}`
      INNER JOIN `{$table[1]}` USING (`{$field[1]}`)
      INNER JOIN `{$table[2]}` USING (`{$field[0]}`)
      WHERE `{$field[2]}` BETWEEN :arr_i AND :arr_k
      AND `{$field[3]}` = :m_act
      AND `{$field[4]}` = :v_act";
  

• And for yet another alternative, you can use here documents, so you don't have to worry about delimiting the string at all. Nice if you have literal double-quotes inside your string, because you don't have to escape them:

  $sql = <<<GO
      SELECT COUNT(DISTINCT `{$field[0]}`) as ctmod FROM `{$table[0]}`
      INNER JOIN `{$table[1]}` USING (`{$field[1]}`)
      INNER JOIN `{$table[2]}` USING (`{$field[0]}`)
      WHERE `{$field[2]}` BETWEEN :arr_i AND :arr_k
      AND `{$field[3]}` = :m_act
      AND `{$field[4]}` = :v_act
  GO;
  

• Finally, it has nothing to do with SQL injection, but a good practice is to check the return value from prepare() and execute(), because they return false if an error occurs in parsing or execution.

  if (($stmt = $link->prepare($sql)) === false) {
      trigger_error(PDO::errorInfo()[2], E_USER_ERROR);
  }
  

  (That example uses PHP 5.4 syntax to dereference an array returned from a function.)

  Or else you can configure PDO to throw exceptions, so you don't have to check.

  $link->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);