Search code examples
phpmysqlpdosql-injection

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


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();
}

Solution

  • 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);