Jump to content

Dynamically creating ?s for a SQL prepare


Strahan

Recommended Posts

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!

Link to comment
Share on other sites


<?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 by benanamen
Link to comment
Share on other sites

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 by benanamen
Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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 by Strahan
Link to comment
Share on other sites

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.

Link to comment
Share on other sites

This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.