ZF2015-08 breaks binary data

[Thomas-Gelf]

ZF2015-08 prevents potential SQL injection for some PDO adapters rarely used by larger projects. Unfortunately it badly corrupts binary data for databases used for serious workloads in real world projects (SCNR).

Sarcasm apart: I didn't dig deeper, just stumbled over corrupt binary checksums and discovered that everything works fine when I revert this change. Blind guess: data is going to be quoted/escaped twice.

Therefore I'd strongly suggest to apply the fix mentioned above only to affected PDO adapter and not to the base abstract class. To me this caused "only" binary checksum collisions that have been caught and noticed. However I guess the impact for others might be far more serious, think of silent corruption of binary data (files, encrypted strings) stored to your DB.

Cheers,
Thomas

[Thomas-Gelf]

Hint: the related commit is 2ac9c30

[froschdesign]

@ezimuel
Can you look at this?

[moshevds]

We are also experiencing this issue. The native PDO::quote method for pdo_mysql handles null bytes correctly and our data is now escaped twice. First with addslashes, and again 2 lines later with $this->_connection->quote.
It seems to me this is inconsistent behavior of PDO::quote depending on the database type and should be fixed in PDO eventually (in pdo_mssql and pdo_sqllite and possibly others), and in the mean time in ZF1 for the affected adapters only.

[Thomas-Gelf]

Could someone please raise priority for this issue? The fix would be to revert the change for ZF2015-08, and to fix MsSql/SQLite issues where they belong to - in their very own adapter. People upgrading ZF1 to 1.12.16+ silently corrupt their data, and they have absolutely no chance to ever get them fixed again.

What really makes me go mad is the fact that the related test was failing (#620), but this didn't hinder 1.12.16 from being released. And instead of letting all bells ring what really happened with 1.12.17 was that the test was basically made useless with #638 to make it pass again.

Following this strategy we could also completely drop all the tests, would make life much easier.

Cheers,
Thomas

NB: When moving the fix for ZF2015-08 to the corresponding adapters please make sure to not double-escape data again. The underlying driver could eventually get it right on specific systems or with newer releases.

[weierophinney]

@Thomas-Gelf — @ezimuel will be examining the situation tomorrow, and, should he create a patch, we will get a release out next week. If you are able to create a patch in the meantime, that will greatly expedite the situation.

[ezimuel]

@Thomas-Gelf I'm working on it as first priority. Unfortunately, I changed the tests with #638 but I didn't suspect to affect binary data for other DB vendors.

[ezimuel]

@Thomas-Gelf I just sent the #670 PR to fix this issue. Can you have a look and try it? Thanks! /cc @moshevds, @weierophinney

[froschdesign]

Fixed with #670

[Thomas-Gelf]

This fix is now two months old, for an issue that silently corrupts binary data since nearly 7 months right now. I can't believe this hadn't caused any damage 'til now. The bug found it's way into many major Linux distributions. As it was flagged "security fix" Debian backported it to ZF 1.12.9. Any chance we could get 1.12.18 any time soon? Especially arguing with Debian packagers is no fun at all. But you have absolutely no chance to ask them for backporting a fix not flagged as a security issue while upstream finds it wouldn't even be worth a release tag :-(

Cheers,
Thomas

[ezimuel]

@Thomas-Gelf we are going to release ZF 1.12.18 this week!

[Thomas-Gelf]

"This week" is fantastic, that's one week earlier than the "next week" @weierophinney mentioned two months ago ;-) Just kidding, thank for your effort and all the good work! And no hurry, I just wanted to make sure that this one will not be forgotten.

Cheers,
Thomas

[Thomas-Gelf]

Once again, thanks a lot :)