Dynamically creating ?s for a SQL prepare

[Strahan]

Hi.  I have an array of fields I'm using to create a SQL query.  Imploding it works fine for building the field list, then I figured I'd str_repeat to build the ?s.  I did:

 

$sql = "INSERT INTO test (" . implode(",", $fields) . ") VALUES (" . str_repeat("?,", count($fields)) . ")";

 

...but that gave me:

 

INSERT INTO test (blah, blah, blah) VALUES (?,?,?,)

 

So to fix it I did:

 

$sql = "INSERT INTO test (" . implode(",", $fields) . ") VALUES (" . substr(str_repeat("?,", count($fields)), 0, strlen(str_repeat("?,", count($fields)))-1) . ")";

 

It works, but that's ugly as sin.  I thought about just doing VALUES(" . str_repeat("?,", count($fields)-1) . "?) but I prefer to make things totally automatic with no hard coding.  I'm sure there is a more appropriate way.  Could someone enlighten me?  Thanks!

[benanamen]

<?php

function insert($table, array $data)

  {


      /*

       * Check for input errors.

       */

      if(empty($data)) {

          throw new InvalidArgumentException('Cannot insert an empty array.');

     }

     if(!is_string($table)) {

         throw new InvalidArgumentException('Table name must be a string.');

     }


     $fields = '`' . implode('`, `', array_keys($data)) . '`';

     $placeholders = ':' . implode(', :', array_keys($data));


     $sql = "INSERT INTO {$table} ($fields) VALUES ({$placeholders})";


     // Prepare new statement

     $stmt = $dbconn->prepare($sql);


     /*

      * Bind parameters into the query.

      *

      * We need to pass the value by reference as the PDO::bindParam method uses

      * that same reference.

      */

     foreach($data as $placeholder => &$value) {


         // Prefix the placeholder with the identifier

         $placeholder = ':' . $placeholder;


         // Bind the parameter.

         $stmt->bindParam($placeholder, $value);


     }


     /*

      * Check if the query was executed. This does not check if any data was actually

      * inserted as MySQL can be set to discard errors silently.

      */

     if(!$stmt->execute()) {

         throw new ErrorException('Could not execute query');

     }


     /*

      * Check if any rows was actually inserted.

      */

     if($stmt->rowCount() == 0) {


         var_dump($dbconn->errorCode());


         throw new ErrorException('Could not insert data into users table.');

     }


     return true;


 }





 $table  = 'your_table';

  $fields = [

      'col_1' => 'value_1',

      'col_2' => 'value_2'

  ];


  //Check if insert returns success

  if(insert($table, $fields)){

 	//The insert was successful

 	echo "success";

 }else {

 	//The insert was not successful


 }


?>

Edited February 17, 2016 by benanamen

[Strahan]

Thanks.  Interesting.  I usually put ? for each piece of data then do $sql->execute(array("blah")).  Is using the : format safer or is there some other reason to do that?  Thanks again.

[benanamen]

I usually use positional placeholders as well (?,?). The only technical difference I am aware of is that named parameters can be in any order. On a large query, named parameters could be easier to use. Some will say it is easier to read. Whatever method you choose, just be consistent. Both are just as safe.

Edited February 17, 2016 by benanamen

[Jacques1]

The above function makes the application vulnerable to SQL injection through the array keys:

$table = 'test';
$values = [
    'x`) SELECT password FROM users -- ' => 123,
    'x' => 123,
];

insert($table, $values);

This will result in the following query:

INSERT INTO test (`x`) SELECT password FROM users -- `, `x`) VALUES (:x`) SELECT password FROM users -- , :x)

Dynamically generated queries are notoriously insecure and should be avoided at all costs. When they cannot be avoided, you have to religiously validate all dynamic input:

• Check the the field names against a whitelist of permitted column names. Do not just insert them into the query, because they may very well come from an untrusted source (e. g. the keys of $_POST).
• Validate the whitelisted column names. By default, only standard MySQL identifiers should be allowed. If the user needs nonstandard identifiers (e. g. column names with spaces), they should have to explicitly set a flag.
• Escape all backticks within the identifiers (through doubling) and then wrap the result in backticks.
• Put a big warning on top of the code (or into DocBlock in case it's a function). Explain the risks and recommend the use of hard-coded keys.

This may sound paranoid, but a very similar problem has lead to the catastrophic Drupageddon vulnerability despite the use of prepared statements. So dynamic queries really are a huge risk.

[benanamen]

Lol, that's what I get for pulling something off stack that I have never used or tested.

[Strahan]

Hmm.  But doesn't that SQL injection vulnerability depend on bad input being put into the array being used to create the query?  The array I use to build the query is created from a SELECT against "DB.INFORMATION_SCHEMA.COLUMNS".  Unless someone renames my table columns I'd think I'm reasonably safe, yes?

Edited February 19, 2016 by Strahan

[Jacques1]

The fact that table columns can be renamed and may contain virtually any character is exactly the problem. Sure, this attack is more difficult, because it requires a second injection vulnerability. But that doesn't mean you're safe.

 

I wouldn't take any risks, especially when the countermeasures are very simple like in this case.

 

One addition to #5: When you escape the backticks, make sure to use the current character encoding of the database system.

[Strahan]

True.  Thanks!