Make WordPress Core

Opened 16 years ago

Closed 15 years ago

Last modified 8 years ago

#5455 closed defect (bug) (worksforme)

Charset SQL Injection Vulnerability

Reported by: pishmishy's profile pishmishy Owned by:
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.5
Component: Security Keywords:
Focuses: Cc:

Description

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)

Attachments (1)

use_mysql_real_escape_string.diff (1.1 KB) - added by filosofo 16 years ago.

Download all attachments as: .zip

Change History (31)

#1 @pishmishy
16 years ago

  • Status changed from new to assigned

#3 @pishmishy
16 years ago

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.

#4 @pishmishy
16 years ago

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?

#6 @pishmishy
16 years ago

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

#7 follow-ups: @ryan
16 years ago

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().

#8 in reply to: ↑ 7 @pishmishy
16 years ago

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.

#9 follow-up: @ryan
16 years ago

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?

#10 in reply to: ↑ 9 @pishmishy
16 years ago

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 :-)

#11 @pishmishy
16 years ago

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

#12 @darkdragon
16 years ago

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

#13 in reply to: ↑ 7 @filosofo
16 years ago

  • Keywords has-patch added

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.

#14 @pishmishy
16 years ago

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

#15 @pishmishy
16 years ago

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 =)

#16 @filosofo
16 years ago

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?

#17 @pishmishy
16 years ago

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() :-)

#18 @ryan
16 years ago

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.

#19 @filosofo
16 years ago

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

#20 @ryan
16 years ago

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

#23 @pishmishy
16 years ago

  • Owner changed from pishmishy to anonymous
  • Status changed from assigned to new

#24 @mrmist
15 years ago

  • Keywords dev-feedback added; has-patch removed

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.

#25 @ryan
15 years ago

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

#26 follow-up: @Denis-de-Bernardy
15 years ago

  • Milestone changed from 2.9 to 2.8

fixed?

#27 in reply to: ↑ 26 @Nicholas91
15 years ago

  • Cc dragos.nicholas@… added
  • Resolution set to worksforme
  • Status changed from new to closed

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.

#28 @hakre
14 years ago

Related: #14672

#29 @DrewAPicture
9 years ago

  • Milestone 2.8 deleted

#30 @ericlewis
8 years ago

  • Keywords dev-feedback removed
  • Milestone set to 2.8
Note: See TracTickets for help on using tickets.