After discussing this on the Security list and on IRC, I am posting this report publicly as requested:

I am reporting a potential issue in the constructor of
DatabaseConnection_mysql for Drupal 7.x.

https://github.com/drupal/drupal/blob/7.x/includes/database/mysql/databa...

The DSN is built using the following code:

     $dsn = 'mysql:host=' . $connection_options['host'] . ';port=' .
(empty($connection_options['port']) ? 3306 :
$connection_options['port']);

And then, later the connection character set is set by calling SET NAMES.

This method does not update the internal client encoding that's used
for escaping by libmysql or mysqlnd. The resulting situation could be
an improperly escaped string being fed to the server.

Suggested Fix:

Add the charset DSN parameter:
http://us3.php.net/manual/en/ref.pdo-mysql.connection.php

This will have no effect < 5.3.6, but it will secure it > 5.3.6 (even
with an additional SET NAMES call).

The issue can happen when the server defaults to a multi-byte
character set such as GBK or BIG5. The client will think it's
escaping GBK when the server is expecting UTF8. So you can get a
situation where an invalid byte stream is sent to the server (causing
an SQL error).

References:

http://blog.ircmaxell.com/2011/06/slides-from-recent-presentations-on-sq...

http://news.php.net/php.bugs/160272

http://bugs.php.net/bug.php?id=47802

http://framework.zend.com/security/advisory/ZF2011-02

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Version: 7.x-dev » 8.x-dev
Priority: Minor » Normal
Issue tags: +Security improvements

Yep, we should definitely do that, even if the potentially affected user base is likely really small.

Triaging, this needs to be fixed in 8.x first. Also marking as vetted by the security team.

mgifford’s picture

FileSize
728 bytes

I think this addresses it. The PHP version requirement for D8 will ensure that this is useful too.

Berdir’s picture

Status: Active » Needs review

Crell’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to 7.x

Seems safe. Also tagging for backport.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -61,6 +61,7 @@ public static function open(array &$connection_options = array()) {
     }
+    $dsn .= ';charset=utf8';

I think a comment would not got amiss here.

Heine’s picture

Status: Needs work » Needs review
FileSize
941 bytes

Comment added.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed ca38ade and pushed to 8.0.x. Thanks!

  • alexpott committed ca38ade on
    Issue #1201452 by Heine, mgifford: Potential Vulnerability In...
mgifford’s picture

Status: Patch (to be ported) » Needs review
FileSize
1009 bytes

Ok, I did a wee bit more than just reroll this patch. It felt wrong not to include the next line to keep it inline with D8 too while I was at it.

I can reroll it if this isn't good.

alexpott’s picture

Status: Needs review » Needs work

@mgifford imo that is out of scope. This will prevent something that would currently cause a php notice in D7. That if was added to D8 for #203955: Create database at installation time - there are discussion about backporting that issue but we should not be doing this here.

mgifford’s picture

Status: Needs work » Needs review
FileSize
864 bytes

Drat.. Oh, what about just doing the backport then. :)

@alexpott sorry about sneaking in that other chunk of code. It just looks messy without it. Still worth doing.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: do1201452-d7-14.patch, failed testing.

Status: Needs work » Needs review
David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

That was a bogus test failure, so back to RTBC for now. (I didn't review the patch myself.)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: do1201452-d7-14.patch, failed testing.

Status: Needs work » Needs review
David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

Testbot fluke.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: do1201452-d7-14.patch, failed testing.

Status: Needs work » Needs review
David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

Testbot fluke.

xjm’s picture

Issue tags: -Needs backport to 7.x +Needs backport to D7
David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

  • David_Rothstein committed 3329a70 on 7.x
    Issue #1201452 by mgifford, Heine, ircmaxell: Improve security on newer...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.