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
Comment | File | Size | Author |
---|---|---|---|
#14 | do1201452-d7-14.patch | 864 bytes | mgifford |
#12 | do1201452-d7-12.patch | 1009 bytes | mgifford |
#8 | do1201452-8.patch | 941 bytes | Heine |
#2 | charset-db-1201452-2.patch | 728 bytes | mgifford |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedYep, 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.
Comment #2
mgiffordI think this addresses it. The PHP version requirement for D8 will ensure that this is useful too.
Comment #3
BerdirComment #6
Crell CreditAttribution: Crell commentedSeems safe. Also tagging for backport.
Comment #7
alexpottI think a comment would not got amiss here.
Comment #8
Heine CreditAttribution: Heine commentedComment added.
Comment #9
jibranBack to RTBC
Comment #10
alexpottThis 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!
Comment #12
mgiffordOk, 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.
Comment #13
alexpott@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.Comment #14
mgiffordDrat.. 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.
Comment #15
Crell CreditAttribution: Crell commentedComment #18
David_Rothstein CreditAttribution: David_Rothstein commentedThat was a bogus test failure, so back to RTBC for now. (I didn't review the patch myself.)
Comment #21
David_Rothstein CreditAttribution: David_Rothstein commentedTestbot fluke.
Comment #24
David_Rothstein CreditAttribution: David_Rothstein commentedTestbot fluke.
Comment #25
xjmComment #26
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!