Search code examples
phpsymfonydoctrine-ormprepared-statementsql-injection

Doctrine expr - does "literal" function use prepared statements internally?


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(), '%_') . '%')));

Solution

  • 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#%%")