0

I'm pretty new to using PDO so I'm not sure if I have it down correctly, however with the following test I'm able to do some injection which I would like to bypass.

In my models class I have some shortcut methods. One of them is called return_all($table,$order,$direction) which simply returns all rows from a table:

 public function return_all($table,$order = false, $direction = false) {
    try {
        if($order == false) {
            $order = "create_date";
        }
        if($direction != false && !in_array($direction,array("ASC","DESC"))) {
            $direction = "DESC";
        }
        $sql = "SELECT * FROM ".mysql_real_escape_string($table)." ORDER BY :order ".$direction;
        $query = $this->pdo->prepare($sql);
        $query->execute(array("order" => $order));
        $query->setFetchMode(PDO::FETCH_ASSOC);
        $results = $query->fetchAll();
    } catch (PDOException $e) {
        set_debug($e->getMessage(), true);
        return false;
    }
    return $results;
}

This works fine, except, if I pass the following as $table into the method:

 $table = "table_name; INSERT INTO `users` (`id`,`username`) VALUES (UUID(),'asd');";

Now it's unlikely that someone will ever be able to change the $table value as it's hard-coded into my controller functions, but, i'm a little concerned that I'm still able to do some injection even when I use PDO. What's more surprising is that the mysql_real_escape_string() did absolutely nothing, the SQL still ran and created a new user in the users array.

I also tried to make the table name a bound parameter but got a sql error I assume due to the `` PDO adds around the table name.

Is there a better way to accomplish my code below?

mauzilla
  • 3,574
  • 10
  • 50
  • 86
  • @summea, I tried making the :table a bound parameter, but sql complained about a SQL error. I assumed it was due to the `` which PDO puts around the table name – mauzilla Mar 21 '13 at 16:19
  • you don't need those `` quotes around the table name if you are not using a reserved word as a table name. Also I don't think you need a mysql_real_escape_string() as PDO does this for you automatically internally as far as I know xd – aleation Mar 21 '13 at 16:33
  • @aleation: I can definately confirm that doing something like "SELECT * FROM :table_name ORDER BY :order DESC" gives a sql error whereas the example above does not. The sql error is: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ''users' ORDER BY 'create_date'' at line 1 – mauzilla Mar 21 '13 at 17:12
  • what's the point in such return_all() function? Looks like your tables serve as some sort plain text files rather than database. Why not to have this function accept arbitrary query (and params to be bound)? – Your Common Sense Mar 21 '13 at 17:19
  • You cannot bind structural elements of the SQL query, only values that the SQL will operate on. You cannot bind column or table names. – Andy Lester Mar 21 '13 at 17:24

1 Answers1

-1

You have already solved your problem with direction.

if($direction != false && !in_array($direction,array("ASC","DESC"))) {
        $direction = "DESC";
    }

Use the same technique for table names

$allowed_tables = array('table1', 'table2');//Array of allowed tables to sanatise query
if (in_array($table, $allowed_tables)) {
    $sql = "SELECT * FROM ".$table." ORDER BY :order ".$direction;
}
david strachan
  • 7,174
  • 2
  • 23
  • 33
  • It was there,but not formtted – david strachan Mar 21 '13 at 17:33
  • @davidstrachan, hey David, thank you for your answer. This would be the safest way to do this. I wanted to write something completely dynamic so that I wouldn't need to change it with each project, but I also realise now that there is a large number of potential security issues with this kind of function so a little security is definately needed. – mauzilla Mar 21 '13 at 17:48