Add new PDO mysql connection attr to control multi statements option #896

Closed
wants to merge 7 commits into
from

Projects

None yet

5 participants

@pwolanin
Contributor

mysqli does not set the CLIENT_MULTI_STATEMENTS flag on connect, so a query with multiple statements fails.

For PDO mysql, it hard-codes the CLIENT_MULTI_STATEMENTS flag in the connection, so there is no way to disable it. However, if using native prepares, sending multiple statements fails on the server. This is inconsistent.

This PR adds a new MySQL-specific attribute that can be only set at connection time to explicitly enable or disable multi statements. As written, it leaves the default the same as current master (enabled).

While this PR is against master, I think this change falls somewhere between a feature and a bugfix and should be back-ported to all supported versions.

The motivation for this is the severity of the recent SQL injection vulnerability in Drupal. If we had any way to disable multi statement in PDO (which is used in Drupal 7.x but not 6.x), we would have, and the vulnerability would have been significantly mitigated. see: https://www.drupal.org/SA-CORE-2014-005

@pwolanin
Contributor

cross-reference to feature request: https://bugs.php.net/bug.php?id=68424

@pwolanin
Contributor

I just rebased on master, so let's see if Travis is happier

@LawnGnome LawnGnome commented on the diff Nov 18, 2014
ext/pdo_mysql/pdo_mysql.c
@@ -126,7 +126,7 @@ static PHP_MINIT_FUNCTION(pdo_mysql)
#if MYSQL_VERSION_ID > 50605 || defined(PDO_USE_MYSQLND)
REGISTER_PDO_CLASS_CONST_LONG("MYSQL_ATTR_SERVER_PUBLIC_KEY", (zend_long)PDO_MYSQL_ATTR_SERVER_PUBLIC_KEY);
#endif
-
+ REGISTER_PDO_CLASS_CONST_LONG("MYSQL_ATTR_MULTI_STATEMENTS", (zend_long)PDO_MYSQL_ATTR_MULTI_STATEMENTS);
@LawnGnome
LawnGnome Nov 18, 2014 Contributor

This needs to be guarded like the previous constant — we still support MySQL 4.1 (not entirely clear on why, admittedly), and it doesn't support CLIENT_MULTI_STATEMENTS.

@pwolanin
pwolanin Nov 18, 2014 Contributor

So, the connection code is guarded with:

#ifdef CLIENT_MULTI_STATEMENTS

not by MySQL version. In fact, it looks like the 4.1 API supports it:
http://dev.mysql.com/doc/refman/4.1/en/c-api-multiple-queries.html

So, I think guarding the constant with the same check would make sense, though I'm not sure at this point when it would be undefined.

@LawnGnome
LawnGnome Nov 18, 2014 Contributor

I think I got tricked by a sentence in the MySQL 5.0 manual implying it was new in that version. Sorry about that! Disregard what I said.

@pwolanin
pwolanin Nov 18, 2014 Contributor

Ok, so does it make sense to guard the class constant using

#ifdef CLIENT_MULTI_STATEMENTS

or since it would be ignored if somehow that wan't defined, it's better to always define it to avoid PHP code having to check if it exists?

@LawnGnome
LawnGnome Nov 18, 2014 Contributor

If you're sure it's always defined in MySQL >= 4.1, then let's not bother with the #ifdef. No point cluttering the code unnecessarily. :)

@pwolanin
pwolanin Nov 18, 2014 Contributor

Well, it's actually defined by default by mysqlnd:

mysqlnd/mysqlnd_enum_n_def.h
96:#define CLIENT_MULTI_STATEMENTS      (1UL << 16) /* Enable/disable multi-stmt support */

It's also available in the C API since January 2003
http://bazaar.launchpad.net/~mysql/libmysql/1.0/view/1412/include/mysql_com.h#L109

http://bazaar.launchpad.net/~mysql/libmysql/1.0/revision/1403.62.1

@pwolanin
pwolanin Nov 20, 2014 Contributor

So, my conclusion is we really don't need the #ifdef anywhere any more, but I guess leaving it in where it is might be fine for now.

@LawnGnome LawnGnome commented on the diff Nov 18, 2014
ext/pdo_mysql/php_pdo_mysql_int.h
#endif
+ PDO_MYSQL_ATTR_MULTI_STATEMENTS,
@LawnGnome
LawnGnome Nov 18, 2014 Contributor

As above, this needs to be behind a MYSQL_VERSION_ID check.

@pwolanin
pwolanin Nov 28, 2014 Contributor

As above - this should not be needed - supported by Mysql 4.1

@LawnGnome LawnGnome and 1 other commented on an outdated diff Nov 18, 2014
ext/pdo_mysql/tests/pdo_mysql_attr_multi_statements.phpt
+ $info = $db->errorInfo();
+ var_dump($info[0]);
+
+ $db = new PDO($dsn, $user, $pass, array(PDO::MYSQL_ATTR_MULTI_STATEMENTS => false));
+ $stmt = $db->exec(sprintf('SELECT * FROM %s; INSERT INTO %s(id) VALUES (3)', $table, $table));
+ $info = $db->errorInfo();
+ var_dump($info[0]);
+
+ $stmt = $db->query(sprintf('SELECT id FROM %s', $table));
+ var_dump($stmt->fetchAll(PDO::FETCH_ASSOC));
+
+ $db->exec(sprintf('DROP TABLE IF EXISTS %s', $table));
+ print "done!";
+?>
+--EXPECTF--
+%unicode|string%(5) "00000"
@LawnGnome
LawnGnome Nov 18, 2014 Contributor

The PHP 6 era %unicode|string% and %u|b% aren't needed in new tests.

@pwolanin
pwolanin Nov 20, 2014 Contributor

Ok, I'll fix this and hopefully that will be good for commit?

@imnotjames

I'd like to really emphasize the importance of this flag for systems that use MySQL with PDO and how lacking we will be without it in older versions of PHP.

Myself and many others cannot disable emulated prepared statements for various reasons. MYSQL_ATTR_MULTI_STATEMENTS would promote good security while still allowing users to use emulated prepared statements. Getting this security enhancing feature into older supported versions of PHP would really benefit the community, and without it mistakes in usage PDO queries can become much worse - without many sane options to mitigate.

Would this be accepted in a PHP 5.6.x release? Or older releases? For 5.4 being security only I could see it being less likely as this is a feature - but it is a feature to fix a hole in security. However, for a 5.6.x and 5.5.x release it would make sense to include this as it is a self contained, small feature which doesn't affect backwards compatibility.

I would be more than willing to help however I can to get this feature into older versions, although I am a bit unclear on the processes involved in doing so.

@php-pulls

Comment on behalf of jpauli at php.net:

Squashed and merged against 5.5 & up

@php-pulls php-pulls closed this Dec 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment