[userbinator]

Drupal uses prepared statements in all its SQL queries.

There's this common misconception "just use prepared statements and they'll completely prevent SQL injection" floating around. Good to see (yet another) counterexample of that. Prepared statements and parameters are only strategies that can help, but they don't replace an understanding of where the characters in the query are coming from and how they're being used. Escaping shouldn't be a difficult concept to understand either.

[meritt]

These aren't prepared statements. This wouldn't be an issue if they were actual RDBMS prepared statements. These are the bullshit fake prepared statements that PDO emulates by default to achieve cross-database compatibility to offer things like named-parameters (oracle, postgresql support) for databases that only offer positional parameters (mysql, mssql).

It's quite simply shoddy string substitution that's not doing proper escaping, as you pointed out.

[kijin]

PDO's prepared statement "emulation" is ridiculous, and turning it off might have blocked some forms of this vulnerability since it would prevent breaking up one query into many. But fixing PDO wouldn't have entirely prevented this fiasco, either.

No amount of prepared statement kung-fu will save you when the querystring itself contains untrustworthy data. Which is exactly what Drupal is doing here. It puts untrusted, potentially non-integer array keys directly into a querystring. Even if Drupal used a database library that supported proper prepared statements, it would have been owned just as well, only slightly less severely.

It's similar to another, much more common misuse of prepared statements: using untrusted values in the column name.

Still, it's always a good idea to set PDO::ATTR_EMULATE_PREPARES to FALSE as soon as you create any instance of that bloody class.

[taspeotis]

> databases that only offer positional parameters (mysql, mssql).

Something between PHP and MSSQL must not support named parameters, because MSSQL supports them just fine.

[eli]

So does MySQL, actually. They are just allegedly slow (though I've never benchmarked and I wouldn't be surprised if that's no longer the case).

[DCoder]

Actually, Microsoft's SQLSRV PDO driver supports named parameters.

[teyc]

If there's one thing I learnt today, I'd make sure my SQL abstraction libraries include test cases which makes sure the libraries barf when presented with bad input.