Need help making PDO prepared statements more secure

Question

I'm building a custom CMS in PHP, and I had the idea to generate MySQL from an array. I'm guessing that's not the safest thing to do, as I'll be putting variables directly into prepared statements. I'm hoping someone more experienced could look at what I've done so far and give me some insight into how I can make this safer.

Here's the class that I store all my data manipulation methods in

abstract class MySQLData extends MySQLDataHelper {

    // connection to database
    abstract public function getConn();
    // table name from concrete class
    abstract public function getTable();

    public function insert( $args ) {
        try {
            $stmt = $this->getConn()->prepare( $this->generateInsertSQL( $args ) );

            foreach ($args as $arg => &$value) {
                $stmt->bindParam(":{$arg}", $value);
            }

            $stmt->execute();               
        } catch (PDOException $e) {
            echo "Insert error: " . $e->getMessage();
        }
    }
//............................................
// More methods below, including delete, select and update, but lets focus on 'insert' only to keep things simple.
}

Helper method class, including insert SQL generator

abstract class MySQLDataHelper {

    public function generateInsertSQL( $args ) {
        $columns = array_keys( $args );

        $placeholders = implode(', :', $columns);
        $columns = implode(', ', $columns);

        return "INSERT INTO `{$this->getTable()}` ({$columns}) VALUES (:{$placeholders})";
    }       
}

Example use case

require_once 'core/Pages.php';
$pages = new Pages( $db->conn() );  // Pages extends MySQLData

$args = array(
    'name'      => 'New page',
    'content'   => 'Lorem ipsum dolor sit amet, consectetur adipisicing elit. Quisquam, laboriosam!',
    'enabled'   => true);

$pages->insert( $args );

Note that the output from generateInsertSQL in this case would be:

INSERT INTO `pages` (name, content, enabled) VALUES (:name, :content, :enabled)

Reminder, this code works perfectly fine, I'd just like to know if it could be made safer.

Thank you. I'm new to SO so sorry if this is a bad question or in the wrong place.

*"this code works perfectly fine, I'd just like to know if it could be made safer."* - Don't get on the web. Think about XSS injection if applicable. — Funk Forty Niner, Sep 11 '15 at 16:33
safer how? unless you do something stupid like `$args['\'; drop table students; --'] = 'haha im in ur query injecting ur tablez'`... — Marc B, Sep 11 '15 at 16:33
Thanks for you comments. @Rocket Hazmat, yes this would be user input from forms in the admin panel. Note that I would of course be sanitizing that input, so it would be more like: $args = array('name' => filterInput($_POST['name'])); — cyanographics, Sep 11 '15 at 17:58
Have you considered using [RedBean](http://redbeanphp.com/)? I highly recommend it. I used to worry about SQL injection and a host of other problems until I found that database operations are much easier through an ORM, and less prone to developer error. — Keshav Saharia, Sep 11 '15 at 22:09

score 0 · Answer 1 · edited May 23 '17 at 11:51

This is a community wiki answer. Anybody can edit it to chime in.

The problem solved by bind variables is this: SQL is a language, and when you assemble statements in the language by interpolating data as if it were text, you're providing a way for untrusted people to augment your SQL programs.

So, you have to diligently sanitize everything. For example, if you're doing this kind of thing:

 id IN (4 5,6,7)

it's not too hard to make sure all the numbers are actually numbers. But you have to do it, every time, without fail, or be pwned. On the other hand, if you're doing

name IN ('cyanographics', 'Rocket Hazmat', 'Fred -ii-1')

and somebody feeds you a malicious username -- think

    cyano'); DELETE FROM TABLE USER CASCADE CONSTRAINTS; //

You end up asking your SQL language interpreter to run this.

name IN ('cyano'); DELETE FROM TABLE users CASCADE CONSTRAINTS; //', 'Rocket Hazmat', 'Fred -ii-1')

This kind of thing can potentially make a big mess of your application.

So, you need to think carefully about how to sanitize text input. You can consider cleaning your text strings with something like PDO::quote(). It puts escape sequences around dangerous characters (like ') in the strings.

But look, you're better off using the built-in bind variable support in PDO, even if it gets harder to handle the lists you want to process.

Read this: What are the best PHP input sanitizing functions?

Also, don't forget to think about browser script injection. What if you're handed the name

 <script>alert('You are pwned, sucka!');</script>

or something worse? You need to clean that stuff out of your input too.

Need help making PDO prepared statements more secure

1 Answers1