5

For usual cases we have the andWhere/orWhere functions that paired with setParameters are correctly protected against injections.

I am having a bit more complex case and I want to be sure all is safe. If I read the doctrine code right seems that using literal has the same effect, but I am not sure so... Can you pls. confirm/infirm that the following 2 cases are safe (are both protected against sql injection using prepared statements, and wildcard injection)?

First case

$expr = $queryBuilder->expr()->orX();
$expr->add($queryBuilder->expr()->lt('entityName.field - ' . $queryBuilder->expr()->literal($rule->getValue()), $queryBuilder->expr()->literal(self::MAX_ERROR))); // Since the second part is a constant (and a numeric one) it shouldn't need literal but... can't hurt.

Second case

$expr = $queryBuilder->expr()->orX();
$expr->add($queryBuilder->expr()->like('entityName.field', $queryBuilder->expr()->literal(addcslashes($rule->getValue(), '%_') . '%')));
zozo
  • 8,230
  • 19
  • 79
  • 134
  • dump($queryBuilder->getQuery()->getSql()) will show you the actual generated sql. It is the only way to be sure of what the query builder is actually doing. – Cerad Dec 09 '19 at 17:03
  • Do you mean does Doctrine bind parameters from the value you supply to [`Query\Expr::literal()`](https://github.com/doctrine/orm/blob/master/lib/Doctrine/ORM/Query/Expr.php#L587)? – Will B. Dec 09 '19 at 19:01
  • @Cerad The dql looks correct (I checked before posting), but the fact that looks correct in my test cases doesn't guarantee is correct in all (that's why I ask how it works under the hood). Basically everything looks fine and test fine, but I still have a shadow of a doubt. – zozo Dec 09 '19 at 21:04
  • @fyrye Yes, you can rephrase the question like that. – zozo Dec 09 '19 at 21:05
  • Not talking about the dql. Talking about the generated sql. If it has parameters in it then all is well. If it has actual strings for data then not so much. By the way, I have been using the query builder for quite a few years. Never felt the need to use expr's. You might be over complicating things. – Cerad Dec 09 '19 at 21:22
  • @Cerad Checked the sql, it is ok (parameters not strings) so all should be fine. How do you handle a variable number of conditions of andWhere mixed with orWhere without expr? Something like WHERE condition1 and (orCondition1 OR orCondition2 or... OR orConditionN). You can of course create a loop (you need that with expr also) and generate a string that you manage manually, binding something like val1, val2 etc. for each param... is not wrong, but is messy and defeats the purpose of the builder. Expr is clean and elegant (and now that I'm at peace with security I think is the right approach). – zozo Dec 10 '19 at 00:47
  • Unlike sql, dql supports arrays as parameters. WHERE id IN (:ids) works fine when :ids is an array. – Cerad Dec 10 '19 at 02:36

1 Answers1

12

In short, no the statements you provided are not safe.

The methods within Query\Expr will not automatically convert your values to parameter placeholders.


Explanation of Query\Expr::literal

Effectively using Query\Expr::literal only converts values to a literal string value for the DQL statement, wrapping the supplied value in quotes as appropriate. While the method does escape single quotes from the supplied value, doing so does not protect against all methods of SQL injection. [sic]

In your first case

$expr = $em->getExpressionBuilder();
$orX = $expr->orX();

$orX->add(
    $expr->lt(
        'entityName.field - ' . $expr->literal($rule->getValue()), 
        $expr->literal(self::MAX_ERROR)
    )
);
//...

$qb = $em->createuQueryBuilder()
    ->where($orX);
dump($qb->getQuery()->getSQL());

If $rule->getValue() is a real number, the resulting SQL statement will become a "literal" numeric value.

WHERE (
   (alias.column - 10 < 2)
   OR
   (...)
)

If $rule->getValue() and self::MAX_VALUE are numeric strings (which may produce unexpected results), the resulting SQL output will be:

WHERE (
   (alias.column - '10' < '2')
   OR
   (...)
)

And $qb->getQuery()->getParameters() will be an empty ArrayCollection, given that no other parameters were added.


Protecting against SQL Injection

To ensure your statement is protected from SQL-injection, you must declare the parameter placeholders within your statement and use setParameter to bind the parameter values to the placeholders.

$orX->add(
    $expr->lt(
        'entityName.field - :rule_value', 
        ':max'
    )
);
$qb->setParameter('rule_value', $rule->getValue());
$qb->setParameter('max', self::MAX_VALUE);

If you have multiple placeholders, you would need to track and bind them as appropriate.

$v = 0;
for (/*...*/) {
    $param = \sprintf('rule_value%d', $v++);
    $orX->add(
        $expr->lt(
            "entityName.field - :$param", 
            ':max'
        )
    );
    $qb->setParameter($param, $rule->getValue());
}
$qb->setParameter('max', self::MAX_VALUE);

Handling nested criterion

From the question in your comment:

How do you handle a variable number of conditions of andWhere mixed with orWhere without expr? Something like WHERE condition1 and (orCondition1 OR orCondition2 or... OR orConditionN)

You can create multiple and nested criterion to supply to the WHERE clause by using the desired groupings of andX or orX.

$expr->orX(
   $expr->andX(
      'expr1',
      'expr2'
   ),
   $expr->andX(
      'expr3',
      'expr4'
   ),
);

or

$andXA = $expr>andX();
$andXB = $expr->andX();
$orX = $expr->orX();

$andXA->add('expr1');
$andXA->add('expr2');

$andXB->add('expr3');
$andXB->add('expr4');

$orX->add($andXA);
$orX->add($andXB);

Alternatively you can add expressions to the main WHERE clause part, by using andWhere or orWhere, but you would need to use the expression builder for altering the nested conditions grouping.

$qb
   ->orWhere($andXA, $andXB);

Which will produce a WHERE clause like

WHERE ((expr1 AND expr2) OR (expr3 AND expr4))

Handling Parameters

To clear up some confusion with the parameters, DQL does not support an array of values. DQL is just a standardized way of querying the object notations known to your Doctrine application.

However the Doctrine 2.1+ QueryBuilder for both the ORM and DBAL as well as the Doctrine\DBAL\Connection::executeQuery methods do support parameterizing an array of values and repeated use of the same named parameter, unlike PDO or MySQLi prepared statements. [sic]. Internally Doctrine will convert the array of parameter values and repeated parameter placeholder values, into individual parameter placeholders to send to the PDO prepared statement.

$expr = $em->getExpressionBuilder();
$qb
    ->where($expr->andX(
        $expr->in('cn.a', ':a'),
        $expr->lt('cn.b', ':b'),
        $expr->gt('cn.c', ':b')
    ))
    ->setParameter('a', ['a', 'b', 'c'])
    ->setParameter('b', 1);

The resulting SQL output.

WHERE w0_.a IN(?, ?, ?)
AND w0_.b < ?
AND w0_.c > ?

Resulting QueryBuilder parameters:

array(array("a", "b", "c"), 1, 1)

Resulting PDO parameters:

array("a", "b", "c", 1, 1)

Preventing Wildcard % Injection

To use a like statement with a parameter placeholder, you must specify the wildcard % or _ within the setParameter() value.

$qb->setParameter(0, '%' . $value . '%');

The down side is that if the variable also includes a wildcard, it can produce undesired results. To prevent wildcard injection you can specify how to escape the wildcard symbols in your query, which will work with the query builder and the parameter placeholders.

It is also considered bad-practice to use the backslash \ as an escape character for LIKE wildcards or SQL queries. Since it is not ANSI SQL standard, it is the same as the PHP escape character, and the escape character could change depending on server configuration. See this question Escaping MySQL wild cards for a more detailed answer.

$qb
    ->where($expr->like('a', ':a ESCAPE ' . $expr->literal('#')))
    ->setParameter('a', '%' . preg_replace('/([#%_])/', '#$0', $value) . '%');

The DBAL Expression Builder, as of 2.7 - 4.0.x-dev (currently), supports the escape character as the third argument. [sic]

public function like($x, $y/*, ?string $escapeChar = null */)
{
    return $this->comparison($x, 'LIKE', $y) .
        (func_num_args() >= 3 ? sprintf(' ESCAPE %s', func_get_arg(2)) : '');
}
$value = 'test%';
$d_qb = $em->getConnection()->createQueryBuilder();
$d_expr = $d_qb->expr();
$d_qb
    ->where($d_expr->like('a', ':a', $expr->literal('#')))
    ->setParameter('a', '%' . preg_replace('/([#%_])/', '#$0', $value) . '%');

Resulting SQL query:

WHERE c0_.column LIKE ? ESCAPE '#'

Resulting parameters:

array("%test#%%")
Will B.
  • 17,883
  • 4
  • 67
  • 69
  • Ty for a very complete answer (although the question in the comment was addressed to a comment above that said that expr is not useful). I know about the numeric literals so that is ok, the object returns me pure numeric formats. Regarding wildcard, why doesn't my addcslashes($rule->getValue(), '%_') . '%' suffice (from the question example)? – zozo Dec 10 '19 at 16:15
  • 2
    @zozo In general it is considered bad-practice to use the backslash as an escape character for `LIKE` wildcards or SQL queries. Since it is not ANSI SQL standard, it is the same as the PHP escape character and the escape character could change depending on [server configuration](https://dev.mysql.com/doc/refman/5.7/en/sql-mode.html#sqlmode_no_backslash_escapes). See this question [Escaping MySQL wild cards](https://stackoverflow.com/a/3683868/1144627) for a more detailed answer. – Will B. Dec 10 '19 at 17:05
  • @fyrye Where are you getting that $qb->expr()->like() accepts a 3rd argument for the character to escape? It doesn't work for me, and looking at the Doctrine ORM code (master branch), I see that the method only accepts 2 arguments. – benface Jan 18 '20 at 18:53
  • 1
    @benface I said the [DBAL expression builder](https://github.com/doctrine/dbal/blob/2.7/lib/Doctrine/DBAL/Query/Expression/ExpressionBuilder.php#L262) as of 2.7, not the ORM expression builder. The argument is commented out, but is retrieved using `func_get_arg(2)` Updated the answer to reflect the versions that support it, with a link to the source code reference. – Will B. Jan 18 '20 at 23:43