Is this PDO function safe from sql injection

Question

I have the following insert function. Is it safe from a sql injection. If it isn't then how do I make it safe.

public function insert($postValues, $table){

    $dbh = $this->connect();

    try {
        $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

        $fields = implode(array_keys($postValues), ',');
        $values = "'".implode(array_values($postValues), "','")."'"; 
        $insertQuery = 'INSERT INTO '.$table.' ('.$fields.') VALUES (:'.$fields.')';

        $stmt = $dbh->prepare($insertQuery);

        foreach($postValues as $vals) {
            $stmt->execute($vals);
        }

        $message = $sucessMessage;
    }
    catch(PDOException $e){
        $message = $e->getMessage();
    }

    $dbh = null;

    return $message;
}

Thanks in Advance

Answer 1

No, because you're just executing a raw SQL query with the PDO extension. I do something similar to the following:

$fields = array();
$values = array();

foreach ($_POST as $field => $value) {
    $fields[] = $field;
    $values[] = $this->pdo->quote($value);
}

$fields = implode(',', $fields);
$values = implode(',', $values);

$sql = "INSERT INTO $table ($fields) VALUES ($values)";
$res = $this->pdo->query($sql);

I'm sure you can modify the above to fit your set-up.

Answer 2

By the way: when asking if PDO is safer from sql injection than some other PHP MySQL connection library, the answer is NO when we talk about PDO_MYSQL (don't know if the following is true for some other databases).

One could even argue the other way round, PDO is less secure and more dangerous than any other PHP MySQL connection library (ext/mysql and ext/mysqli) because PDO_MYSQL allows for multiple queries in one SQL statement while ext/mysql stops multi-queries completely and ext/mysqli has a sparate function mysqli_multi_query().

I just tried to find any sources to support this statement, but the only things I found are:

• Protection against SQL Injection using PDO and Zend Framework

  PDO_MySQL is a more dangerous application than any other traditional MySQL applications. Traditional MySQL allows only a single SQL query. In PDO_MySQL there is no such limitation, but you risk to be injected with multiple queries.

• Zend Webcast "Secure Application Development with the Zend Framework" by Stefan Esser (slide 18)

Answer 3

The only sane way is to use PDO::prepare with parameters (see example in manual). Moreover, field names should be taken from trusted source, i.e. not user. This way, you build your query string from trusted components:

function insert ($table, $fields, $data)
{
    $field_names = implode (", ", $fields);                      # "a, b"
    $values = ":" . implode (", :", $fields);                    # ":a, :b"
    $query = "INSERT INTO $table($field_names) VALUES($values)";
    $sth = $pdo->prepare ($query);

    foreach ($data as $row) {
        # Here you can even remove "bad" keys from $row
        $sth->execute ($row);
    }
}

$fields = array ('a', 'b'); # those are hard-coded in application
$data = array (             # those come from user
    array ('a'=>'Apple', 'b'=>'Bean'),
    array ('a'=>'Avocado', 'b'=>'Blueberry', '); DELETE FROM fruits; -- '=>'evil'),
);
insert ('fruits', $fields, $data);

Answer 4

If each column type is a PDO::PARAM_STR, then it is fairly simple to bind your parameters to unamed paramter markers using PDOStatement::execute. However, if the column types vary, then you need to specify the column type for each column when you bind to it with PDOStatement::bindParam.

Accepting table and column names from what appears to be user input, is not a good idea. The query will fail if the table or column names are incorrect, but you need to be very careful to ensure that the table and column names are safe to use. The following example checks the table and column names against a whitelist, prior to executing any SQL:

function insert($postValues, $table) {
    $dbh = $this->connect();

    // Create a simple whitelist of table and column names.
    $whitelist = array('my_table' => array('col1', 'col2', 'col3'));

    // Check if the table name exists in the whitelist.
    if(!array_key_exists($table, $whitelist)) {
        exit("$table is not a valid table name.\n");
    }

    // Count the number of columns that are found in the whitelist.
    $cols = count(
        array_intersect(
            $whitelist[$table],
            array_keys($postValues)));

    if($cols !== count($postValues)) {
        exit("One or more invalid column names have been supplied.\n");
    }

    // Create a comma separated list of column names.
    $columns = implode(', ', array_keys($postValues));
    // Create a comma separated list of unnamed placeholders.
    $params = implode(', ', array_fill(0, count($postValues), '?'));
    // Create a SQL statement.
    $sql = "INSERT INTO $table ($columns) VALUES ($params)";

    // Prepare the SQL statement.
    $stmt = $dbh->prepare($sql);
    // Bind the values to the statement, and execute it.
    return $stmt->execute(array_values($postValues));
}

echo insert(
    array(
        'col1' => 'value1',
        'col2' => 'value2',
        'col3' => 'value3'),
    'my_table');

// 1

echo insert(
    array(
        'col1' => 'value1',
        'col2' => 'value2',
        'col3' => 'value3'),
    'unsafe_table');

// unsafe_table is not a valid table name.

echo insert(
    array(
        'col1' => 'value1',
        'col2' => 'value2',
        'unsafe_col' => 'value3'),
    'my_table');

// One or more invalid column names have been supplied.