(followup) Limit PDO MySQL to executing single statements if PHP supports it

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -83,6 +83,11 @@ public static function open(array &$connection_options = array()) {
+    if (defined('\PDO::MYSQL_ATTR_MULTI_STATEMENTS')) {
+      // An added connection option in PHP 5.5.21 to optionally limit SQL to a
+      // single statement like mysqli.
+      $connection_options['pdo'] += array(\PDO::MYSQL_ATTR_MULTI_STATEMENTS => FALSE);
+    }
 

Are you sure this constant is valid? I can't find any documentation for this and I also tried running this on my local and it came back as:

PHP Fatal error: Undefined class constant 'MYSQL_ATTR_MULTI_STATEMENTS'

I am using PHP 5.6.0. Also, other constants documented here work fine.

Okay, I read the issue summary again and see it is only available on 5.6.5. I checked the downloads page and the current release is 5.6.3, which means the next should be 5.6.4. Any idea why this feature would skip a release?

Also, this means that the effect of this cannot yet be tested (unless we compile php from dev).

On doing some research, I found this answer which says that multiple queries are executed only if emulated prepares are enabled. The answer is old and things might have changed since then. However, I am just putting it in here as a note. I have not tried this yet.

However, if it is still valid, it looks like #2348931: Use native MySQL statement preparation via PDO might solve the problem here.

I am +1 here, pending a test server to verify that it works as advertised.

@pwolanin I think the new test infrastructure has working bots on multiple PHP versions - however those obviously aren't picking up patches from d.o yet. Doubt that existing bot will change since that'd also affect Drupal 6/7.

@pwolanin
Did you found the time to build such a testbot, at least temporarily?

No question, this improves the security quite a bit, but still, I'm not entirely sure whether this is release blocking, and not "just" a major
which should really get in, because its so damn useful. (especially given that many hosts with run 5.4 or older versions of 5.5, and so never profit from it).

I also think this isn't release blocking - it's something we could add in a patch release with no issues from what I can see.

Leaving critical for now, but adding the needs triage tag.

It would be great to document in install.txt / install-mysql.txt as well as drupal.org/requirements that php 5.5.21 would be recommended as it improves security.

I added some text in INSTALL.txt to explain this. I couldn't find any relevant section to add something similar in INSTALL.mysql.txt.

By the way, PHP 5.5.21 is released but I still don't see PHP 5.6.5. I have still mentioned that version in the INSTALL.txt.

Just an update: PHP 5.6.5 was released as well. More details here: http://php.net/archive/2015.php#id2015-01-22-3

Tested applying patch 2388255-15.patch and it is was successful:
patching file core/INSTALL.txt
Hunk #1 succeeded at 62 (offset -1 lines).
patching file core/lib/Drupal/Core/Database/Driver/mysql/Connection.php

I think this is ready.

Hold on we have failing tests - see #11. Also what about #2348931: Use native MySQL statement preparation via PDO? Wouldn't that make us secure regardless?

@alexpott the fails in #11 are false negatives due to autoincrement != 1 on pwolanin's system, not due to this patch it looked like.

I think it's worth doing this even if we eventually do #2348931: Use native MySQL statement preparation via PDO, that issue doesn't look like it'll be ready any time soon.

Several people have already commented that this does not seem critical, and I agree. Still an important improvement though.

This is only as important as SA-CORE-2014-005, because this weakness is basically responsible for how quickly and dramatically it was possible to exploit that vulnerability.

I can't tell if #23 is arguing that this is less important now that 005 is patched, or should be at OMG! priority level because 005 was such a huge deal. :-) As this is a security question I defer to the security team on the priority, but either way we should get it in as soon as we can verify it.

*more* critical. It's possible that 005 was the last sql injection in core, but definitely not contrib and maybe not even in core. The question from my perspective is: how easy do we want to make it for attackers when the next issue drops.

I think lets get this tested on a PHP 5.5 with proper auto increment setting and we can get this in ...

Per #25.

There we go.

~/w/d8 (2388255) $ php --version
PHP 5.5.21 (cli) (built: Feb  8 2015 13:59:47)
Copyright (c) 1997-2014 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2014 Zend Technologies
    with Zend OPcache v7.0.4-dev, Copyright (c) 1999-2014, by Zend Technologies
    with Xdebug v2.2.6, Copyright (c) 2002-2014, by Derick Rethans

~/w/d8 (2388255) $ d8st --class "\Drupal\system\Tests\Database\ConnectionTest"
Drupal test run
---------------

Tests to be run:
  - \Drupal\system\Tests\Database\ConnectionTest

Test run started:
  Tuesday, March 10, 2015 - 20:55

Test summary
------------

\Drupal\system\Tests\Database\ConnectionTest                  24 passes   1 fails

Test run duration: 12 sec

~/w/d8 (2388255) $ gpatch https://www.drupal.org/files/issues/2388255-15.patch
--2015-03-10 20:55:53--  https://www.drupal.org/files/issues/2388255-15.patch
Resolving www.drupal.org... 93.184.220.99
Connecting to www.drupal.org|93.184.220.99|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 1963 (1.9K) [text/plain]
Saving to: 'STDOUT'

-                                         100%[====================================================================================>]   1.92K  --.-KB/s   in 0s

Test run duration: 4 sec

~/w/d8 (2388255) $ d8st --class "\Drupal\system\Tests\Database\ConnectionTest"

Drupal test run
---------------

Tests to be run:
  - \Drupal\system\Tests\Database\ConnectionTest

Test run started:
  Tuesday, March 10, 2015 - 20:56

Test summary
------------

\Drupal\system\Tests\Database\ConnectionTest                  25 passes

Test run duration: 4 sec

It's up to @dawehner if he wants to reroll the patch for these minor comment changes, otherwise the patch looks good to me.

1. +++ b/core/modules/system/src/Tests/Database/ConnectionTest.php
   @@ -118,4 +118,22 @@ function testConnectionOptions() {
   +   * Ensure that you cannot execute multiple queries on phpversion() > 5.5.21.
   

   I think it's enough to say "PHP > 5.5.21", and we should also mention 5.6.5.

2. +++ b/core/modules/system/src/Tests/Database/ConnectionTest.php
   @@ -118,4 +118,22 @@ function testConnectionOptions() {
   +      $this->fail('NO PDO exception thrown for multiple queries.');
   ...
   +      $this->pass('PDO exception thrown for multiple queries.');
   

   multiple statements? :)

Should we add a requirements warning for this to prompt users to upgrade if possible?

I think it makes sense to suggest users good practise, both in hook_requirements but also in INSTALL.txt

+++ b/core/modules/system/system.install
@@ -80,6 +80,17 @@ function system_requirements($phase) {
+  if ((version_compare($phpversion, '5.6.0') < 0 && version_compare($phpversion, '5.5.21') < 0) || (version_compare($phpversion, '5.6.0') > 0 && version_compare($phpversion, '5.6.5') < 0)) {
+    $requirements['php'] = array(

I didn't check if this is the reason for the failures but something seems wrong here. I think you meant version_compare($phpversion, '5.5.0') > 0 in the first condition. If I read this correctly, you are only warning if the PHP version is 5.5.* or 5.6.*. Why not warn for 5.4.* too?

Also, I think it might be better to write this as:

if (version_compare($phpversion, '5.5.0', '<')
  || (version_compare($phpversion, '5.5.0', '>=') && version_compare($phpversion, '5.5.21', '<'))
  || (version_compare($phpversion, '5.6.0', '>=') && version_compare($phpversion, '5.6.5', '<'))) {
...

EDIT: Fixing number of brackets (might still be wrong).

@hussainweb
Feel free to patch it.

This is why I love test first ... you can write this much easier actually

As it turns out, REQUIREMENTS_WARNINGS are treat as errors ...

... in more than one place

   // If there are errors, always display them. If there are only warnings, skip
   // them if the user has provided a URL parameter acknowledging the warnings
   // and indicating a desire to continue anyway. See drupal_requirements_url().
-  if ($severity == REQUIREMENT_ERROR || ($severity == REQUIREMENT_WARNING && empty($install_state['parameters']['continue']))) {
+  if ($severity == REQUIREMENT_ERROR) {

Why are you removing this?

If it's not working as documented, it should be fixed... but given that there are tests for the intended REQUIREMENT_WARNING behavior I'm surprised that it could be broken.

I'm not sure if this requirement warning is important enough to show during install anyway though (maybe only after install). It would be an annoying interruption for anyone installing with PHP 5.4 since it's not too likely they'll drop what they're doing and switch to a different PHP version in the middle of the install.

+    $requirements['php'] = array(
+      'title' => t('PHP (multiple statement disabling)'),
+      'value' => $phpversion,
+      'description' => t('PHP versions higher than 5.6.5 or 5.5.21 provides strengthening against SQL injections. Its recommended to update.'),

There are some typos here ("injections" => "injection" and "Its" => "It's" or "It is") but overall this just isn't clear. Maybe a title more like "PHP built-in SQL injection protection" with enabled/disabled for the status and a description that makes clear that this is only backup protection (your site won't automatically be vulnerable to SQL injection just because you don't have it).

Also this appears to be displayed to everyone, even though it's only relevant for sites using MySQL?

Personally I'd be happy to just defer the requirements warning part of this patch to a followup issue.

It's an important security improvement to get this in (regardless of whether we warn/inform people about it or not).

Changing sentence as per #41 and also (hopefully) improving readabality in SystemRequirements::phpVersionWithPdoDisallowMultipleStatements. Great job on simplifying the logic here!

@David_Rothstein
When we (alexpott and I) talked about it, we seemed to get confused by the way how things work. In fact, a warning works like a warning, you just need an additional
step by explicit clicking on continue, which let the test fail ...

Reverted all those changes, and replaced REQUIREMENT_WARNING with REQUIREMENT_INFO for now, and instead opened up a followup: #2452587: Make PHP version INFO a WARNING

I'm not sure if this requirement warning is important enough to show during install anyway though (maybe only after install). It would be an annoying interruption for anyone installing with PHP 5.4 since it's not too likely they'll drop what they're doing and switch to a different PHP version in the middle of the install.

Good point, let's discuss in the other issue. One question we should think about: how do we get the users to see the message, the installer is certainly the most "in your face" position.

Ups.

RTBC, lets indeed make more prominent warnings in a follow-up.

I'm right in thinking we actually have no test coverage of this because the bots are on the minimum PHP version - right?

I'm right in thinking we actually have no test coverage of this because the bots are on the minimum PHP version - right?

Well, yeah, until Drupal CI is there, we run always on the minimal version which leads to a non-running test. Once we have the newer version,
we would have automatically test coverage.

using the more common tag.

.

So has anyone actually done a full test run with this enable on a php version that supports it?

I haven't.

@pwolanin
Have you executed a full test suite?

It is hard to commit this is we know we are not actually testing it. I think this is a fantastic security improvement but I'd hate to up the bots PHP version and be surprised.

I could test this on 5.6.6 if that would help?

@mikey_p: Yes, that will be great. You have to run the full test suite though.

Hrm, I'll give it a shot, but I don't think my local setup likes simpletest much since I tend to have random failures and differences between the command line and web runner all the time anyway.

Sounds like this still needs review, per #52.

... Alright, lets use the testbot project and run the tests ... note: this takes a hell long time, currently waiting for the ConfigImportAllTest
this is the command I used:

vagrant@debian-7:~/drupalci_testbot$ sudo DCI_TESTGROUPS="--all" DCI_CONCURRENCY="2" DCI_DRUPALBRANCH="8.0.x" DCI_PATCH="https://www.drupal.org/files/issues/2388255-46.patch" ./run.sh

Applying Patch: https://www.drupal.org/files/issues/2388255-46.patch
patch: unified diff output, ASCII text
Checking patch core/INSTALL.txt...
Checking patch core/includes/install.inc...
Hunk #1 succeeded at 925 (offset 1 lines).
Checking patch core/lib/Drupal/Core/Database/Driver/mysql/Connection.php...
Checking patch core/modules/system/src/SystemRequirements.php...
Checking patch core/modules/system/src/Tests/Database/ConnectionTest.php...
Checking patch core/modules/system/system.install...
Checking patch core/modules/system/tests/src/Unit/SystemRequirementsTest.php...
Applied patch core/INSTALL.txt cleanly.
Applied patch core/includes/install.inc cleanly.
Applied patch core/lib/Drupal/Core/Database/Driver/mysql/Connection.php cleanly.
Applied patch core/modules/system/src/SystemRequirements.php cleanly.
Applied patch core/modules/system/src/Tests/Database/ConnectionTest.php cleanly.
Applied patch core/modules/system/system.install cleanly.
Applied patch core/modules/system/tests/src/Unit/SystemRequirementsTest.php cleanly.
Done!

Mh, I had a test failure last time, let's run it again.

Note: The following tests fail for me:

• Drupal\system\Tests\Session\SessionHttpsTest 29 passes 5 fails 4 exceptions
• Drupal\rest\Tests\Views\StyleSerializerTest 119 passes 14 fails 1 exceptions
• Drupal\system\Tests\Module\InstallUninstallTest 1443 passes 19 fails

Will do some research why they fail.

Do those tests work on vanilla HEAD with PHP 5.5?

@dawehner: SessionHttpsTest and StyleSerializerTest are bot failures. This is an already known issue within the current docker web containers. Seem to be caused by broken json mime support in apache. So please ignore them :-)

ah, that is issue: #2385697: Broken Tests because of missing mime type support in web container

Finally completed ...

Test run duration: 1 day 17 hours

real 148918.04

... I ran the InstallUninstallTests a couple of times more and that failure seemed to be just caused by some timeout due to my slow machine.
When you don't do anything else, it can work without any failure.

@dawehner thank you!

This is an obvious security hardening of Drupal. Nice work everyone. Committed a68c046 and pushed to 8.0.x. Thanks!

Great, thank you alex!

@xjm pointed out that this probably should have a change record

Sure https://www.drupal.org/node/2463973

I don't think we should claim that this solves SQL injection (it doesn't - just reduces the typical severity, as far as I understand). So I edited the change notice for that and fixed a few other small things too:

https://www.drupal.org/node/2463973/revisions/view/8313659/8313927

I also published the change record (it was still in draft state).

Filed #2464605: Regression: no link to admin/reports/status/php

+++ b/includes/database/mysql/database.inc
@@ -47,10 +47,15 @@ class DatabaseConnection_mysql extends DatabaseConnection {
-      PDO::MYSQL_ATTR_USE_BUFFERED_QUERY => TRUE,
+      \PDO::MYSQL_ATTR_USE_BUFFERED_QUERY => TRUE,
       // Because MySQL's prepared statements skip the query cache, because it's dumb.
-      PDO::ATTR_EMULATE_PREPARES => TRUE,
+      \PDO::ATTR_EMULATE_PREPARES => TRUE,

Drupal 7 should be able to run on PHP 5.2 which does not know anything about "\" namespaces, right?

Definitely an improvement worth backporting to D7.

I just tested this in an environment using php 5.5.26 (where the mysql option is supported):
1. I ran the automated tests for the site, just to get some more diverse testing - they all passed.
2. I manually created a sql injection and tested using multiple statements to delete the contents of watchdog - without the patch I'm able to do that, with the patch the statement is blocked and there's an error message

In case anyone wants to test:

<?
function user_init() {
$foo = range(1, 10);
foreach ($foo as $bar) {
watchdog('dummy', 'messages');
}
$result = db_query("select uid, name from users where uid = 0 " . $_GET['sql'])->fetchAll();
dsm($result);
}
?>

Be sure to enable devel and clear caches so that Drupal can find the new hook_init.

Then
1. On http://www.example.dev/ - you should see a single record returned in the dsm.
2. On http://www.example.dev/?sql=or%20uid%20in%20%281%29 - you should see 2 records - anonymous user and uid 1.
3. Then confirm you have lots of messages in watchdog
4. Then, http://www.example.dev/?sql=or%20uid%20in%20%281%29;delete%20from%20watc... - should delete everything from watchdog

#78 works for me as well.

Committed to 7.x - thanks!

I committed it as is since it seems important to get in, but I'm wondering why at least the tests (and possibly the REQUIREMENT_INFO part too) weren't backported from Drupal 8? Leaving the issue open for that.

I made some edits to the change notice at https://www.drupal.org/node/2463973 too, to make it apply to Drupal 7 and to add some explanation of why this is an (almost 100%) backwards-compatible change.

I left the change notice tagged for Drupal 8 too, in addition to Drupal 7. Not sure it matters, but maybe if someone is updating their code from an older Drupal 8 beta version they would still care about this.

Would have been nice if this had been implemented with some sort of option to bypass it. This change broke the Open Atrium quick-installer that uses the DB import code. It is doing a raw DB import that executes multiple lines at a time for performance.

Also sure would have been nice if this option in PHP generated a different error message other than the generic

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax

Took several hours to determine that our installer broke because of this core change. You'd think PHP could have added an error message for "Cannot execute more than more statement at a time" or something. Sigh.

Looks like doing a loop as suggested is still nice and fast. So the change notice is good, it's just going to be really hard for people to debug that they have this issue since it's PHP version dependent and doesn't give a good error message. Unfortunately not something Drupal can really help with. I certainly understand the motivation of this change.

This update has broke a lot of automatically built sites and it took me 3 hours to find out why. There should be a proper notification issued that there is a major API change!

#88: You can find the change notice here:

- https://www.drupal.org/node/2463973

It also is displayed as an API change here:

- https://www.drupal.org/drupal-7.40-release-notes

I however agree that API changes should have their own section instead of having API additions + changes together.

@alex.designworks, this is a behavior change, but I personally wouldn't consider it a major API change since the most common reason for doing multiple queries at once (multiple UPDATE, INSERT, or DELETE) was already forbidden by the db_query() API documentation anyway. Do you have some other use case?

I tested @mpotter's comments above and agree that the PHP error message that winds up being displayed in this case is not too helpful. I'll add that information to the change notice (and maybe to the release notes also) to help people find it.

Would have been nice if this had been implemented with some sort of option to bypass it.

As far as I know it was - see the documentation in settings.php about how to override PDO connection settings. For example putting this line of code in settings.php seemed to work for me to undo it.

$databases['default']['default']['pdo'][PDO::MYSQL_ATTR_MULTI_STATEMENTS] = TRUE;

I think this feature of the database API isn't commonly known; maybe the change notice should mention it also? On the other hand I'm not sure we really want to encourage people to turn this back on.... so I will leave it to someone else to edit the change notice and add something about that if they want to.

[#2573637] will probably go in, too. Then it fails reliably on all databases (and is overridable).

This issue is listed as critical "needs work" for 7.x. It's been committed to 8.x as the most recent automated comments show. It was also committed to 7.x in e575b47.

It seems like this is "Fixed" so I'm updating status. Thanks for the work on it, everyone!