Doctrine queryBuilder: SQL Injection risk in addOrderBy() method?

Question

I a using Doctrine in a Symfony 2.8 project and I wonder if there is any risk of SQL Injections when using the addOrderBy() method of the queryBuilder:

// Order options. Real code does not specify this manually, but receives 
// the options via user form input
$orderBy' = array(
    'column1' => 'ASC',
    'column2' => 'DESC',
    ...
    'columnN' => 'ASC',
);

$qb = $this->em->createQueryBuilder();
...

foreach ($orderBy as $column => $orderOption) {
     $qb->addOrderBy("e.$column", $orderOption);

     // Does not work:
     // $qb->addOrderBy("e.$column", ':orderOption')
     //  ->setParameter('orderOption', $orderOption);
     // 
     // Error: Expected end of string, got ':orderOption'"
}

// Result is something like:
...ORDER BY e0_.column1 ASC, e0_.column2 DESC...

The problem is, that the order options are received via user form input that could be manipulated to something like ; DROP TABLE someTable instead of ASC or DESC.

I already tried this, but the query builder doesn't seem to accept multiple queries separated by ;, which does not mean, that there could not be any other/better injections :-)

Of course the problem could easily be solved by filtering the received results and skip all invalid search options. But I am trying to understand, if the addOrderBy() method in general. Is it save to pass any value to the method and Doctrine will handle the rest, or is there a potential risk?

I wonder why the ->setParameter() method does not work, as it would when using ->where().

Answer 1

Short answer is that column names submitted by form could in fact be used for a sql injection attack. Doctrine assumes you have properly validated column (and table) names.

The doctrine code is fairly easy to read and it's worth taking a look at for these sorts of questions:

public function addOrderBy($sort, $order = null)
{
    $orderBy = ($sort instanceof Expr\OrderBy) ? $sort : new Expr\OrderBy($sort, $order);

    return $this->add('orderBy', $orderBy, true);
}

Note that there is no value at all in using Expr in your queries. Doctrine takes care of generating them for you.

$this->add is a bit more complicated but basically the second argument ends up being passed along with no escaping or filtering etc.

I wonder why the ->setParameter() method does not work, as it would when using ->where()

The important concept is that prepared statements only protect values not column or table names.

So again, it entirely up to you to filter table/column names coming from the wild. And looking at the source can be informative.

Answer 2

You can use the Expr class: http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/query-builder.html#the-expr-class

Or a simple function/method to return a valid value:

function orderOption($option, $defaultOption = 'ASC') {
    if (in_array(strtoupper($option), ['ASC', 'DESC']) {
        return $option;
    }
    return $defaultOption;
}