Charset SQL Injection Vulnerability

[pishmishy]

Mis-escaping of queries in a non-utf8 encoding, can cause an opportunity for SQL injection attacks. I believe the problem is supposed to be in escape().
(See ​http://packetstormsecurity.org/0712-exploits/wordpresscharset-sql.txt)

[pishmishy]

​http://www.abelcheung.org/advisory/20071210-wordpress-charset.txt is a better description of the vulnerability.

[pishmishy]

I'd have thought that switching escape() to use mysql_real_escape_string() would fix this but it doesn't appear to do so (perhaps I'm doing something wrong), I still get the error.

/index.php?exact=1&sentence=1&s=%b3%27 gets me

WordPress database error: [You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '�\))) AND post_type = 'post' AND (post_status = 'publish') ORDER BY post_dat' at line 1]

SELECT SQL_CALC_FOUND_ROWS wp_posts.* FROM wp_posts WHERE 1=1 AND (((post_title LIKE '�\) OR (post_content LIKE '�\))) AND post_type = 'post' AND (post_status = 'publish') ORDER BY post_date DESC LIMIT 0, 10

See also #3286.

[pishmishy]

It appears that mysql_real_escape_string() ignores any change of character set during an established mysql session and continues to use the first character set.
(See ​http://ilia.ws/archives/103-mysql_real_escape_string-versus-Prepared-Statements.html)
The general fix to this problem appears to be prepared statements.
Or perhaps someone can code a better escaping function?

[pishmishy]

See also ​http://dev.mysql.com/doc/refman/5.0/en/mysql-real-escape-string.html

[pishmishy]

See escape_string_for_mysql() in mysys/charset.c of the MySQL 5.0.51 source for how multibyte characters are handled.

[ryan]

We tried mysql_real_escape_string() some time ago and it caused lots of problems. That was before mysql_set_charset() came along, however. I think if mysql_set_charset() is available and DB_CHARSET is set, we can safely use mysql_real_escape_string().

Let's try something like this. In wpdb::construct(), call mysql_set_charset(), if it exists, instead of SET NAMES. Flag the fact that we've called mysql_set_charset(). Check this flag in wpdb::escape() and call mysql_real_escape_string() if the charset was set with mysql_set_charset(). If the charset was set with SET NAMES or not set at all, use addslashes().

[pishmishy]

Problem: set_charset() never exists in WordPress as it's only available through the improved mysqli interface not mysql. Even so; won't your suggestion still result in the vulnerability being present for people not using the later versions of PHP and MySQL?

Further notes that may help... (mb_detect_encoding($string)!="ASCII") will detect multibyte strings, ($this->charset != mysql_client_encoding($this->dbh)) detects the mismatch between WordPress and db session's character sets.

[ryan]

There are both mysql_set_charset() and mysqli_set_charset flavors, I believe. I think you have to have fairly recent versions of MySQL and PHP for these things to work as they should. set_charset() is a necessity for us.

It looks like drupal uses mysql_real_escape_string() and SET NAMES without using mysql_set_charset(). I wonder how they get away with that. I think they upgrade their tables so that they are in UTF-8. Maybe they force UTF-8 everywhere?

[pishmishy]

Replying to ryan:

There are both mysql_set_charset() and mysqli_set_charset flavors, I believe. I think you have to have fairly recent versions of MySQL and PHP for these things to work as they should. set_charset() is a necessity for us.

Woops so there is. How did that sneak in there.

It looks like drupal uses mysql_real_escape_string() and SET NAMES without using mysql_set_charset(). I wonder how they get away with that. I think they upgrade their tables so that they are in UTF-8. Maybe they force UTF-8 everywhere?

I'm not sure but I'll take a look. I'm not sure that the character set of the tables effects the problem but my knowledge starts to run out at this point :-)

[pishmishy]

Drupal appears to give you either UTF-8 or nothing. Perhaps drupal's unicode.inc is the key?

[darkdragon]

Has a relationship to #5424, if mysql_real_escape_string() is used again.

[filosofo]

Replying to ryan:

We tried mysql_real_escape_string() some time ago and it caused lots of problems. That was before mysql_set_charset() came along, however. I think if mysql_set_charset() is available and DB_CHARSET is set, we can safely use mysql_real_escape_string().

Let's try something like this. In wpdb::__construct(), call mysql_set_charset(), if it exists, instead of SET NAMES. Flag the fact that we've called mysql_set_charset(). Check this flag in wpdb::escape() and call mysql_real_escape_string() if the charset was set with mysql_set_charset(). If the charset was set with SET NAMES or not set at all, use addslashes().

Now that PHP is at 4.3 or newer we can use mysql_real_escape_string. I've attached a patch that employs it when the charset is utf8 and that sets the charset using mysql_set_charset for those that have it, following your suggestion.

Can we get this in? It would help address complaints like ​this one.

[pishmishy]

We still leave things open for sites which don't have mysql_set_charset() though.
but...
It's better than doing nothing. =)

[pishmishy]

Oh and in your patch I think that

function escape($string)

Can just be

function escape($string) { return mysql_real_escape_string( $string, $this->dbh );  }

The problem isn't that we can't use mysql_real_escape_string() for some character sets - just that it doesn't always do what's expected. Since addslashes() doesn't either - I think we might well just use mysql_real_escape_string() and make the code simple. I hope that makes sense =)

[filosofo]

Ryan said that there were problems using mysql_real_escape_string when the charset was not utf8, so I was working off that assumption (although I haven't been able to find a trac ticket or discussion about what exactly those problems are). Ryan, can you elaborate?

[pishmishy]

filosofo, I believe the problem isn't UTF8 specific, but was the problem that occurs if you change to UTF8 after starting the connection in another character set. The one you handled with checking for mysql_set_charset() :-)

[ryan]

If mysql_set_charset() exists and MySQL >= 5.0.7, then call mysql_set_charset() and use mysql_real_escape_string(). Otherwise we SET NAMES (If MySQL >= 4.1.0) and continue to escape with slashes. That seems conservative enough for 2.5. We can put that it and ask those on the polyglots list to try it out and let us know if there are encoding issues.

Perhaps someday we can do as Drupal and enforce UTF-8 everywhere.

[filosofo]

Ryan, would you explain the nature of the problems we're trying to avoid?

[ryan]

Last time we tried to switch to mysql_real_escape_string(), it stomped characters for lots of people. Part of that was because of bugs in mysql_real_escape_string(), IIRC, some of which were addressed by mysql_set_charset(). To safely use mysql_real_escape_string(), I think we have to have mysql_set_charset() and MySQL 5.0.7 and the user needs to define DB_CHARSET to match his tables. There's also the possibility I don't know what I'm talking about.

​http://ilia.ws/archives/103-mysql_real_escape_string-versus-Prepared-Statements.html
​http://dev.mysql.com/doc/refman/5.0/en/news-5-0-22.html
​http://dev.mysql.com/doc/refman/5.0/es/mysql-real-escape-string.html

[ryan]

​http://dev.mysql.com/doc/refman/5.0/en/mysql-real-escape-string.html

[pishmishy]

See also #3286

[mrmist]

Is this still an issue? I tried the test query pishmishy used above and now it returns a search result rather than a hacked-query error.

[ryan]

(In [10597]) Use real escape in environments that support it. see #5455

[Denis-de-Bernardy]

fixed?

[Nicholas91]

Replying to Denis-de-Bernardy:

fixed?

I did the same thing as mrmist, tested the link in the 3rd comment and it returns a search page. Ticket closed for now.

[hakre]

Related: #14672