Escaping DATE_FORMAT in "order" section of query REGRESSION

[druidvav]

After merging #418 we started reciving errors like that:

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'DATE_FORMAT( FROM_UNIXTIME( t.last_update ) , '%Y-%m-%d' )' in 'order clause', query was: SELECT `t`.*, `g`.`name` AS `group_name`, `g`.`located_in` AS `group_located` FROM `user_questions_tickets` AS `t`
 LEFT JOIN `user_questions_tickets_group` AS `g` ON t.ticket_group = g.id WHERE (t.status = "opened") AND (t.is_system = 'n') AND (t.oversized = 1) ORDER BY `DATE_FORMAT( FROM_UNIXTIME( t`.`last_update ) , '%Y-%m-%d' )` DESC, `t`.`last_update` DESC LIMIT 20

In previous versions DATE_FORMAT in order section worked properly, and now it is broken. I believe this is important regression and should be fixed ASAP.

[croensch]

@druidvav Is using nested SQL functions. It seems to me that it's not even possible to detect this in a regular expression (which this feature relies upon):

• http://stackoverflow.com/questions/133601/can-regular-expressions-be-used-to-match-nested-patterns
• http://stackoverflow.com/questions/14952113/how-can-i-matches-the-nested-brackets-by-regex,
• http://stackoverflow.com/questions/17118032/regular-expression-to-extract-php-code-partially-array-definition/17134110).

Therefore nested SQL functions should have never been supported in the first place if the feature where implemented properly.

[druidvav]

Anyway, this is a major framework and you cannot just fix something and break backward compatibility with lots of projects in minor update and without proper announcement.

[croensch]

The security advisory first mentioned in 1.12.7 that certain code should be fixed on the consumer side. Granted 1.2.7 would have run your code. In the announcment of 1.12.8 however, the full changelog also leads to #378 "ZF-1.12.7 breaks code when using multi column ordering".

That's alarm bells for me... I've not made the upgrade yet. It's only out since yesterday anyway - the security threat is present but not too risky for a day (in our environment).

In the long term, consumer side code must be fixed - because it's not a feature it's a security hole!

I've made a regex to find all places in the code that need that fixing http://regex101.com/r/nK7yK3/2
As i said, not tested on large codebase yet, but should run fine in any good IDE.

_edit: updated regex_
pessimistic, optimistic (not for from), optimistic (fixed for from and joins)

[druidvav]

This change is not only a security hole. It will lead us (and not only us, i suppose) to freeze our version to 1.12.7. I am not sure that we'll get the budget to fix this in really old and big project.

[jameshalsall]

@druidvav we also have fairly large projects running 1.12.7 and after updating to 1.12.8 we had to rollback

Is there another way around this issue?

[froschdesign]

@druidvav

This change is not only a security hole.

Sorry, but I think you misunderstand the comment from @croensch. The patch for 1.27.7 fixes a potential security hole!

[croensch]

I've upgraded our projects today, here is another regex i used to find any suspicious SQL function usage:
(?<!new Zend_Db_Expr\()\s*['"]\s*[\w_]+\(
It yields some false-positives when there is a space between the constructor paranthesis _Expr( and the actuall query part "SQL(. Altough i used alot of \s* .. Well it's pretty usable tough.

[ezimuel]

@druidvav I introduced a more restrictive regular expression in from(), group() and order() methods to prevent potential SQL injections, see the security advisory ZF2014-04. This restriction can be a potential BC break for complex SQL statement, like in you case where you have sub-functions. To avoid this problem you can use the new Zend_Db_Expr() as suggested also by @croensch. I'm sorry if this change have broken some codes.
I also send a message to the fw-general and fw-accounce mailint lists to report this problem.