Using PDO but can still SQL inject

Question

Until recently all the database queries from my PHP app were going through a single function that put everything into an insecure "mysqli_query". I wanted to move away from this for obvious reasons and convert to PDO to prevent injection.

A helpful user provided this PDO class/function as an alternative:

class   DatabaseConfig
    {
        private static  $singleton;

        public  function __construct()
            {
                if(empty(self::$singleton))
                    self::$singleton    =   $this->connect();

                return self::$singleton;
            }

        public  function connect($host = "localhost", $username = "username", $password = "password", $database = "database")
            {
                // Create connection
                $opts   =   array(  PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
                                    PDO::ATTR_EMULATE_PREPARES => false,
                                    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC);
                $conn   =   new PDO('mysql:host='.$host.';dbname='.$database, $username, $password,$opts);

                return $conn;
            }
    }
// This is a query class that will run your sqls
class   QueryEngine
    {
        private $results;

        private static  $singleton;

        public  function __construct()
            {
                if(empty(self::$singleton))
                    self::$singleton    =   $this;

                return self::$singleton;
            }

        public  function query($sql = false,$bind = false)
            {
                $this->results  =   0;

                $db     =   new DatabaseConfig();
                try {
                        if(!empty($bind)) {
                                $query  =   $db ->connect()
                                                ->prepare($sql);
                                $query->execute($bind);
                            }
                        else {
                                $query  =   $db ->connect()
                                                ->query($sql);
                            }

                        $this->results  =   $query;
                    }
                catch (PDOException $e)
                    {
                        die($e->getMessage());
                    }

                return $this;
            }

        public  function fetch()
            {
                while($row = $this->results->fetch())
                    $result[]   =   $row;

                return (!empty($result))? $result : 0;
            }
    }


function dbcommand($req,$bind = false)
    {   
        // Create query instance
        $qEngine    =   new QueryEngine();
        // Run the query
        $qEngine->query($req,$bind);
        // If select, fetch array
        if(strpos(strtoupper($req), 'SELECT') === 0)
            return $qEngine->fetch();
        // The query already ran, so you can return whatever you want
        // For ease I am just returning true
        elseif(strpos($req, 'INSERT INTO') === 0)
            return  true;
    }

The above code works great but the issue is that I can still SQL inject into statements that I'm not filtering prior to using dbcommand. I was under the impression PDO automatically resolves injection issues but I'm obviously adopting it incorrectly. Can anyone englighten me?

Answer 1

This "dbcommand" function is intended to be used this way:

$row = dbcommand("SELECT * FROM users WHERE id=?", [$id]);

Here you have to substitute all the variables in the query with question marks, while sending variables as a second parameter, in the form of array.

Or you can use named placeholders and associative array:

$update = [
  'name' => $_POST['name'],
  'id'   => $_POST['id'],
]
dbcommand("UPDATE users SET name=:name WHERE id=:id", $update);

Only when variables are substituted with placeholders PDO will protect your queries.

On a side note, although this function is a good intention by itself, it severely reduces the usability of the vanilla PDO. For example, for the large datasets, it is not advised to fetch all the data at once, but this function offer no option for this. Or this function won't let you get the number of affected rows. To make a simple PDO wrapper usable, one should make it return a PDOStatement instance. It is essential, because there is way too much functionality in this class to be simply dismissed.

For a class that lets you use all the great power of PDO, refer to my Simple yet efficient PDO wrapper, especially "Examples" section. All examples can be run without any prior setup, save for setting database credentials.

Just for the clarification: a list of possible use cases from the link above which your class doesn't support:

• Prepared statement multiple execution
• Getting the insert id
• Getting rows in a loop one by one
• Getting one row
• Getting single field value
• Getting array of rows in all the dozens of predefined formats supported by PDO
• Getting the number of affected rows