Use mysql_real_escape_string instead of addslashes

[hakre]

Good news for security: Since about 20 days 2.9 is now released which raised the minimum PHP requirements to version 4.3. A benefit of that version is that it provides and important function to prevent SQL Injections:

​mysql_real_escape_string()

Writing those lines as of today might look a bit akward, but until today there were already multiple tries to get escaping data for the database properly done incl. the use of mysql_real_escape_string. A first try was done in [2684] as a fix for #1394. I can not say it better in my own words then the ticket's description:

add_slashes() does not escape all database input correctly

That was for WordPress Version 1.5 that time 5 years ago by now. But those changes have been reverted in [2737] where matt described his own code as "It falls back to funky escaping that causes problems and is not reversible, so temporarily disabling.". There is no ticket available related to that changeset so this is the only documentation we have why that is removed.

I strongly doubt that mysql_real_escape_string() is broken and I see absolutely no argument to not use it from now on whenever something needs to be escaped for database queries and a resource link to the MySQL connection is available.

[hakre]

Kudos to nacin who ​compiled a history of related changesets in #11605.

[nacin]

We bumped MySQL to 4.1.2. We've been requiring PHP 4.3 since, I think, WP 2.5.

As the history shows (thanks for the kudos), whenever we've switched over to real_escape, we've quickly reverted to addslashes(). I doubt the core devs will want to attempt this again. At the very least, this belongs nowhere near a maintenance release.

[ryan]

We do use it if mysql_set_charset() is available and the charset is set. It is done with prepare(), insert(), and update() which covers all core queries. It is not done in escape() for plugin compat reasons. Plugins should use prepare(), insert() or update() to get real escaping.

[Denis-de-Bernardy]

Replying to ryan:

We do use it if mysql_set_charset() is available and the charset is set. It is done with prepare(), insert(), and update() which covers all core queries. It is not done in escape() for plugin compat reasons. Plugins should use prepare(), insert() or update() to get real escaping.

Plugins also use escape() because prepare() has a messy/buggy syntax. Please consider re-opening this.

[hakre]

Replying to ryan:

We do use it if mysql_set_charset() is available and the charset is set. It is done with prepare(), insert(), and update() which covers all core queries. It is not done in escape() for plugin compat reasons. Plugins should use prepare(), insert() or update() to get real escaping.

Which just means that for installations running below PHP 5.2.3 will not have propper SQL escaping [mysql_set_charset (PHP 5 >= 5.2.3)].

I see no technical reason why to not use it with a standard database connection as well regardless of the usage of mysql_set_charset(). If you can provide arguments which does actually prevent a usage in those cases, please name those. If it's too technically, please link them at least.

I think it's a bad Idea to have that no in based only on assumptions years ago. The changes-history does not show a clear picture here.

[hakre]

Replying to nacin:

We bumped MySQL to 4.1.2. We've been requiring PHP 4.3 since, I think, WP 2.5.

Yeah, right, my fault. This is then a case since 2008-03-29.

As the history shows (thanks for the kudos), whenever we've switched over to real_escape, we've quickly reverted to addslashes().

Might be but the question is why? Is mysql_real_escape() broken? Does it not work? Or was it just a mistake to revert the change years ago? From what I can find documented it does not say a lot and it's years ago (5 years or so), the remove was about two and a half year before even WP 2.5 was released. And that was the release which actually offered that function.

[hakre]

I reopen since the closing argumentation by ryan does not apply for many installations reg. PHP and MYSQL version. Additionally as Denis pointed to, the actual usage of those functions should be taken into account.

Correctly spoken, this ticket is a regression of what was reported in #1394. To be fair, the old ticket should be reopened (because the changes which fixed the issue were removed) and this ticket should then be closed as a duplicate. Just as a sidenote.

[ryan]

mysql_real_escape_string() is not safe without mysql_set_charset(). Getting this all working right took years. Let's not retread this ground yet again.

[Denis-de-Bernardy]

@Ryan: fair enough. In this case, could we add a check in $wpdb->escape(), that makes it use $wpdb->_escape() accordingly?

[ryan]

I'll provide more background in hopes I can point to this ticket the next time it comes up.

mysql_set_charset()and its counterpart mysql_set_character_set() in MySQL are needed to properly set the internal encoding used by mysql_real_escape_string() and other functions. SET NAMES is not sufficient.

[ryan]

I don't think we can do any real escaping in escape() because it will reopen #9189. mysql_real_escape_string() is not reversible like addslashes() is. Due to sordid history, most WP functions expect slashed data. Those slashes are then stripped and prepare is used. If data is passed real escaped, unslashing won't necessarily work. We can expose real_escape() or something similar though. Plugins have to to keep in mind that this should be used only when doing its own queries, not when passing things to WP API functions.

[Denis-de-Bernardy]

I'd like to see escape() using real_escape() when it's available, and magic quotes ought to be using addslashes().

[miqrogroove]

We can expose real_escape() or something similar though

It's already in there. I think it's called _real_escape().

[hakre]

Replying to ryan:

I'll provide more background in hopes I can point to this ticket the next time it comes up.

mysql_set_charset()and its counterpart mysql_set_character_set() in MySQL are needed to properly set the internal encoding used by mysql_real_escape_string() and other functions. SET NAMES is not sufficient.

I had the fear that you refer to it, but I could not find any source that is backing this up. Can you please name precisely the root cause why mysql_real_escape_string() should not be save by using "SET NAMES"?

---

Regarding to the usage of database escape functions as a tool of general use by some users. I think this is the fault of those users and not of the API. I mean we are talking here about the DB class and it's not a general escaping class.

[hakre]

Refering to my own hakre and SET NAMES:

Okay I found the reference regarding ​SET NAMES and mysql_real_escape_string(), it's in the MySQL manual (​Version 3.23/4.0/4.1 Information). Quote:

If you need to change the character set of the connection, you should use the mysql_set_character_set() function rather than executing a SET NAMES (or SET CHARACTER SET) statement. mysql_set_character_set() works like SET NAMES but also affects the character set used by mysql_real_escape_string(), which SET NAMES does not.

Note: This is not the true arumentation why mysql_real_escape_string() is not used within the core code. As a proof you can compare to the usage of addslashes: It has the same (and even more) deficiencies and it is used.

From what I can say so far is, that there were problems like in #9189 which motivated the switch back to addslashes. Ryan pointed to that as well. Maybe there is an easier fix for #9189 which can pass all DB tests as well which will help us to get more safety by escaping database queries in the future.

BTW, I just wonder why #9189 does not regress on those installations that do use mysql_real_escape_string() as of today... .

[Denis-de-Bernardy]

Well, in this case we merely need to set the necessary variables:

mysql> show variables like 'char%';
+--------------------------+-----------------------------------------+
| Variable_name            | Value                                   |
+--------------------------+-----------------------------------------+
| character_set_client     | utf8                                    |
| character_set_connection | utf8                                    |
| character_set_database   | utf8                                    |
| character_set_filesystem | binary                                  |
| character_set_results    | utf8                                    |
| character_set_server     | utf8                                    |
| character_set_system     | utf8                                    |
| character_sets_dir       | /opt/local/share/mysql5/mysql/charsets/ |
+--------------------------+-----------------------------------------+
8 rows in set (0.09 sec)

mysql> set names 'latin1';
Query OK, 0 rows affected (0.33 sec)

mysql> show variables like 'char%';
+--------------------------+-----------------------------------------+
| Variable_name            | Value                                   |
+--------------------------+-----------------------------------------+
| character_set_client     | latin1                                  |
| character_set_connection | latin1                                  |
| character_set_database   | utf8                                    |
| character_set_filesystem | binary                                  |
| character_set_results    | latin1                                  |
| character_set_server     | utf8                                    |
| character_set_system     | utf8                                    |
| character_sets_dir       | /opt/local/share/mysql5/mysql/charsets/ |
+--------------------------+-----------------------------------------+
8 rows in set (0.01 sec)

I take it that character_set_server is the one that we want to override in addition?

[Denis-de-Bernardy]

Just for reference, and best I'm aware, character_set_database is for use base create table statements that lack an explicit charset, and character_set_system is the charset used by the OS.

[hakre]

Well, to answer Denis questions then finally a look into the mysql sourcecode must be taken, but this is far out of my scope. I collected most of the interesting info ​on my blog now so if you like to have proper escaping, just use PHP 5.2.3 and MySQL 5.0.7 combined with already named wpdb::functions.

[secondv]

I would imagine that a query such as:

SET character_set_results = 'utf8', character_set_client = 'utf8', character_set_connection = 'utf8', character_set_server = 'utf8';

Would work.

[miqrogroove]

Replying to ryan:

I don't think we can do any real escaping in escape() because it will reopen #9189. mysql_real_escape_string() is not reversible like addslashes() is. Due to sordid history, most WP functions expect slashed data. Those slashes are then stripped and prepare is used. If data is passed real escaped, unslashing won't necessarily work. We can expose real_escape() or something similar though. Plugins have to to keep in mind that this should be used only when doing its own queries, not when passing things to WP API functions.

I think this reasoning became obsolete in [12961]. Mark helped me get that committed because some of the like_escape() logic was looking unpossible without a clean input path. According to Mark, slashed inputs were never intended to be touched by any DB logic, and never will be. They are now officially separate.

This might be the key to moving forward with SQL sanity.

[Denis-de-Bernardy]

@microgroove: not quite obsolete yet. addslashes_gpc() might also need to be fixed.

It would be really sweet if this were fixed in WP 3.0. there's going to be an increasing number of WPMU installs, and many may eventually be subject to SQL injections because wpdb->escape() is a mere alias for addslashes().

[miqrogroove]

I don't see a problem with addslashes_gpc(). It's being used in wp-query to haphazardly escape some SQL variables. Whatever damage can be done there, it appears to be confined to DB logic.

[dd32]

Moving this to 3.1 due to changing it this late in a dev cycle is just asking for trouble.

[wonderboymusic]

This is another escape / slash convo that hopefully be closed or resolved in 3.6 slashing inspection

[rmccue]

We want to fix this via #21663. Will still be required if we fall back to mysql though, I'd say.

[rmccue]

Given that #21663 is getting bumped, any thoughts on this for 3.6?

[ryan]

This has strayed well past the original subject. I think this plus #21767 document the reasons why we're not going to turn all weak escapes into real well enough. #21767 and #22325 document how the other issues raised here are going to be addressed. Resolving this as a wontfix for the original report.