# WordPress.org

## Make WordPress Core

Opened 8 years ago

Closed 3 years ago

# like_escape() should escape backslashes too

Reported by: Owned by: miau_jp wonderboymusic 4.0 high normal 2.8 Formatting 4.0-early has-patch

### Description

The like_escape() function doesn't escape backslashes.

	return str_replace(array("%", "_"), array("\\%", "\\_"), $text);  should be ...  return str_replace(array("\\", "%", "_"), array("\\\\", "\\%", "\\_"),$text);


or simply ...

	return addcslashes($text, '%_');  Considering multi-byte characters ...  if (function_exists('mb_ereg_replace')) {$text = mb_ereg_replace('\\\\', '\\\\', $text);$text = mb_ereg_replace('%', '\\%', $text);$text = mb_ereg_replace('_', '\\_', $text); return$text;
} else {
return addcslashes($text, '%_'); }  ### Attachments (19) ### Change History (90) ### #1 @Denis-de-Bernardy 8 years ago • Keywords needs-patch early added • Milestone changed from Unassigned to 2.9 • Version set to 2.8 Confirming... mysql> select '\\' like '\\'; +----------------+ | '\\' like '\\' | +----------------+ | 1 | +----------------+ 1 row in set (0.00 sec) mysql> select '\\' like '\\\\'; +------------------+ | '\\' like '\\\\' | +------------------+ | 1 | +------------------+ 1 row in set (0.00 sec) mysql> select '\\' like '\\%'; +-----------------+ | '\\' like '\\%' | +-----------------+ | 0 | +-----------------+ 1 row in set (0.00 sec) mysql> select '\\' like '\\\\%'; +-------------------+ | '\\' like '\\\\%' | +-------------------+ | 1 | +-------------------+ 1 row in set (0.00 sec)  ### #2 @Denis-de-Bernardy 8 years ago • Owner set to Denis-de-Bernardy • Status changed from new to accepted ### #3 @Denis-de-Bernardy 8 years ago this is probably good enough as a fix: return addcslashes($text, '%_');


It's like... WP isn't mb_string safe anyway.

### #4 @Denis-de-Bernardy 8 years ago

• Keywords has-patch added; needs-patch removed

### #5 @Denis-de-Bernardy 8 years ago

• Owner Denis-de-Bernardy deleted
• Status changed from accepted to assigned

### #6 @westi 8 years ago

• Milestone changed from 2.9 to 3.0

I've added these test cases to wordpress-tests.

We can fix this early in the 3.0 dev cycle.

### #7 @westi 8 years ago

• Resolution set to fixed
• Status changed from assigned to closed

(In [12460]) Improve like_escape to also escape '\'. Fixes #10041 props miau_jp and Denis-de-Bernardy.

### #8 @westi 8 years ago

(In [12957]) Revert [12460] for now while we consider a better fix. See #10041

### #9 @westi 8 years ago

• Resolution fixed deleted
• Status changed from closed to reopened

### #10 @miqrogroove 8 years ago

In the course of discussing the input paths in IRC, we came to some conclusions:

1. Super-global variables should be addslashes() instead of wpdb->escape(). That was changed in [12961].
1. I discovered the list of like-special chars is not limited to \ % _ Apparently single and double quotes are also special at the LIKE layer, so searching literally for hello\\'world becomes an interesting test case. This contradicts the MySQL manual, but appears to be accurate. I also confirmed \x, \0, \r, and \n are not special at the LIKE layer.
1. We did not yet decide on a new pattern for calling like_escape(). Any time a super-global variable is passed in, it must be stripslashes() first. We could make that a requirement, or we could create a second function like_escape_global() that includes the call to stripslashes(). In any case, escape() or prepare() must still be used after like_escape(), unless there are more changes.
1. Most instances of LIKE do not currently use like_escape(). A partial list was included in #12060.
Last edited 5 years ago by miqrogroove (previous) (diff)

### #11 @miqrogroove 8 years ago

I posted these two examples at the mysql.com string comparison page. There's no need to support both of them, but it is an interesting exercise.

SELECT 'hello\\\\\'world' LIKE 'hello\\\\\\\\\\\'world'

or

SELECT 'hello\\\\\'world' LIKE 'hello|\\|\\|\'world' ESCAPE '|'

Last edited 5 years ago by miqrogroove (previous) (diff)

### #12 @miqrogroove 8 years ago

I guess we need to ask ourselves, what's the difference between these two queries?

SELECT 'hello\'world' LIKE 'hello\'world';

SELECT 'hello\'world' LIKE 'hello\\\'world';


If you say there's no difference, then we can safely ignore quotes in like_escape(). If you are confused and think this makes no sense, then the extra slashes help explain what's going on.

Last edited 5 years ago by miqrogroove (previous) (diff)

### #13 @Denis-de-Bernardy 8 years ago

@robert: the issue isn't quotes, which are properly handled. the issue is backslashes and percent chars.

### #14 @miqrogroove 8 years ago

Yes of course. The problem is that quotes are unavoidable when discussing slashes. Earlier, I was trying to explain to Mark the difference between \% \\% \' and \\\' in LIKE values. Unfortunately, those last 2 examples turned out to be identical, screwing up my point about the need to double-escape slashes and percent chars.

In any case, the focus should be on how to implement stripslashes() and escape/prepare so that the like_escape() function doesn't create vulnerabilities.

Last edited 5 years ago by miqrogroove (previous) (diff)

### #15 @miqrogroove 8 years ago

Here's one way to simplify the issue:

The 3 precedents for using like_escape() in WordPress all have this pattern:

$like = like_escape($var);
$sql = "SELECT whatever LIKE '%$like%';


So, one way to make that secure is...

//$stringin expected slashed function like_escape($stringin) {
global $wpdb;$stringout = stripslashes($stringin); // Get raw value$stringout = addcslashes($stringout, '\\%_'); // Make value like-safe$stringout = $wpdb->_real_escape($stringout); // Make value sql-safe
return $stringout; }  Then in places like canonical.php, the call to prepare() would have to be removed when implementing like_escape(). ### #16 @miqrogroove 8 years ago • Keywords has-patch removed • Priority changed from low to high • Severity changed from minor to normal Westi, if I were to write a full diff based on the strategy in my previous reply, would you commit it? ### #17 follow-up: ↓ 18 @Denis-de-Bernardy 7 years ago tiny note: I think like_escape() should expect *unslashed* data. see also #12402 ### #18 in reply to: ↑ 17 @miqrogroove 7 years ago Replying to Denis-de-Bernardy: tiny note: I think like_escape() should expect *unslashed* data. In this situation, the choice to go with slashed or unslashed will have no security impact. like_escape() has an algorithmic requirement for unslashed data, therefore it is more conservative to explicitly stripslashes() within the function. Since it is also the case that all super global values are slashed by WP at load, it makes no sense to require most like_escape() calls to be preceeded by a call to stripslashes(). I think the strategy I proposed above holds to that argument. ### @Denis-de-Bernardy 7 years ago ### #19 @Denis-de-Bernardy 7 years ago • Keywords has-patch added ### @Denis-de-Bernardy 7 years ago ### @Denis-de-Bernardy 7 years ago ### #20 @nacin 7 years ago • Milestone changed from 3.0 to 3.1 Early 3.1 per the dev chat today. ### #21 @nacin 7 years ago • Keywords 3.2-early added; early removed • Milestone changed from Awaiting Triage to Future Release No traction, feedback, or testing. Future for now. ### @ampt 7 years ago Updated patch 10041.4.diff to apply cleanly against trunk, the changes to /wp-admin/ms-sites.php no longer apply. The unit tests for like_escape pass. ### @wonderboymusic 5 years ago ### #22 @wonderboymusic 5 years ago • Milestone changed from Future Release to 3.6 Updated the patch so it applies against trunk without dying - this can be perused as part of the overall slashing inspection ### #23 @miqrogroove 5 years ago 10041.6.diff would octuple-slash the search value in taxonomy.php. Please correct that. ### #24 @ryan 4 years ago • Milestone changed from 3.6 to Future Release ### #25 @miqrogroove 3 years ago Is the general direction in 10041.6.diff agreeable to everyone? The input is a raw string, no slashes added or removed. The output is a raw LIKE phrase, which is not SQL safe. The phrase can then be used as a string in prepare() or real_escape() as part of a query. I can pick this up and get it ready for the 4.0 milestone, but this ticket needs a direction. ### #26 @miqrogroove 3 years ago Important: Convolution of slashes in MySQL: SELECT 'a' a SELECT '\a' a SELECT '\\a' \a SELECT '\\\a' \a SELECT '\\\\a' \\a SELECT '%' % SELECT '\%' \% SELECT '\\%' \% SELECT '\\\%' \\% SELECT '\\\\%' \\%  The main thing to notice here is the potentially confusing result of a LIKE '\\\%' statement. You might expect this string literal to represent \% which is a literal % at the LIKE layer. That is not the case. At the DML layer, this string literal represents \\% which is a literal slash followed by a wildcard at the LIKE layer. Just always keep this in mind when testing things for this ticket. It's very different from string literals in PHP or even C++. ### #27 follow-up: ↓ 28 @miqrogroove 3 years ago • Keywords dev-feedback 4.0-early added; has-patch has-unit-tests 3.2-early removed I'm working on a proper patch that will take about 700 lines to get this fixed and add the needed unit tests. Rather than posting a work in progress, I'm waiting for some guidance from the lead developers. They are super busy with the 3.9 milestone right now, so it will be some weeks before there is more progress. I'm thinking like_escape() needs to be deprecated at this point. The problem with tweaking the existing function is that we can't very well patch all of the plugins that are using it. I think this is why slashes were never escaped during all this time. Changing the existing function would affect how and where it can be used, and everywhere else it would be broken. ### #28 in reply to: ↑ 27 ; follow-up: ↓ 36 @Denis-de-Bernardy 3 years ago Replying to miqrogroove: I'm thinking like_escape() needs to be deprecated at this point. If you do, can I suggest naming the new function as quote_like() or quote_sql_like() or quote_like_sql() rather than esc_like() or esc_sql_like() or esc_like_sql(), to make it clearer that it's not actually escaping anything, but merely quoting LIKE special chars. Last edited 3 years ago by Denis-de-Bernardy (previous) (diff) ### #29 follow-up: ↓ 30 @Denis-de-Bernardy 3 years ago Also, and btw, addclashes() doesn't seem to double-escape occurrences of escaped quotes, so this works fine:  var_dump( mysqli_escape_string($wpdb->dbh, "Hello'\"\\ World"),
mysqli_escape_string($wpdb->dbh, addcslashes(addslashes("Hello'\"\\ World"), "_%")) ); Yields: string 'Hello\'\"\\ World' (length=17) string 'Hello\\\'\\\"\\\\ World' (length=23) And then: MariaDB [(none)]> select 'Hello\'\"\\ World', 'Hello\'\"\\ World' like 'Hello\\\'\\\"\\\\ World'; +----------------+----------------------------------------------------+ | Hello'"\ World | 'Hello\'\"\\ World' like 'Hello\\\'\\\"\\\\ World' | +----------------+----------------------------------------------------+ | Hello'"\ World | 1 | +----------------+----------------------------------------------------+ 1 row in set (0.00 sec)  Which probably means that this three-liner would be an adequate replacement that passes all of the tests: function quote_sql_like($str) {
return addcslashes(addslashes($str), '%_'); }  Last edited 3 years ago by Denis-de-Bernardy (previous) (diff) ### @miqrogroove 3 years ago ### #30 in reply to: ↑ 29 @miqrogroove 3 years ago Patch attached. Denis, thanks for the suggestions. I decided to start with esc_like() for consistency, and IMO the name is not going to help or hurt developers. I put some nice big doc blocks in there. If that isn't clear enough, then we would need something more drastic than a new name. Also, I agree with your earlier comment that quotes are not special at the like layer. We don't need addslashes(). It makes an already complicated situation that much worse. ### @miqrogroove 3 years ago Fixed a missing arg in schema.php. ### #31 @miqrogroove 3 years ago • Keywords has-patch added; dev-feedback removed ### This ticket was mentioned in IRC in #wordpress-dev by miqrogroove. ​View the logs. 3 years ago ### #33 @miqrogroove 3 years ago Status update: I'm hoping to get this patch on the 4.0 milestone ASAP. It needs to be early. miqro-10041.2.patch has these main features: in wp-includes/formatting.php correct the escaping function, rename it to esc_like(), and add documentation of appropriate usage. in wp-includes/deprecated.php make a new home for the old like_escape(). add tests/formatting/EscLike.php with real$wpdb tests instead of ambiguous I/O strings.

delete tests/formatting/LikeEscape.php

All other files involved have calls to $wpdb that were not correct before the patch. ### This ticket was mentioned in IRC in #wordpress-dev by johnbillion. ​View the logs. 3 years ago ### @miqrogroove 3 years ago Move new function into wpdb. ### #35 @miqrogroove 3 years ago Based on initial feedback from nacin, I've attached miqro-10041.3.patch. • Moved the new function to wpdb::esc_like() • Added 2 more calls to prepare() in wp-includes/query.php for consistency. • Refreshed all calls to the new function. • Refreshed the new unit tests. ### @miqrogroove 3 years ago Fixed missing wildcards from original patch of query.php ### #36 in reply to: ↑ 28 ; follow-up: ↓ 37 @miqrogroove 3 years ago Replying to Denis-de-Bernardy: make it clearer that it's not actually escaping anything, but merely quoting LIKE special chars. I considered this again. Remember, escape is the verb used in SQL, i.e. LIKE '25\\% of \'people\'.' is the default equivalent of LIKE '25\\% of \'people\'.' ESCAPE '\\' but we could specify LIKE '25@% of \'people\'.' ESCAPE '@' so yes, we are actually escaping things in this context. ### #37 in reply to: ↑ 36 @Denis-de-Bernardy 3 years ago Replying to miqrogroove: Replying to Denis-de-Bernardy: make it clearer that it's not actually escaping anything, but merely quoting LIKE special chars. I considered this again. Remember, escape is the verb used in SQL, i.e. (...) so yes, we are actually escaping things in this context. Of course... but the intent of my remark was to avoid the potential for confusion in a not-so-competent plugin dev's mind. If it's called esc_like(), it looks like esc_attr() or esc_sql() and the implicit message is "secure". If we call it quote_like(), it quotes -- without it sending any kind of implicit message that may give a false sense of security. ### #38 @miqrogroove 3 years ago Starting to see multiple conflicted files in trunk. Nacin, is there any chance of getting this committed? ### @miqrogroove 3 years ago Refresh ### #39 @Denis-de-Bernardy 3 years ago Can I suggest an extra two tests with quotes? 'howdy\'' 'howdy\"'  ### #40 @Denis-de-Bernardy 3 years ago And the same tests with quotes for the data_like_query() test cases. ### #41 @miqrogroove 3 years ago I'll add tests on the next refresh, sure. I was thinking it would make sense to also add like-special chars from other SQL flavors so that the unit tests can give accurate results on exotic servers. In those situations the MySQL tests might pass, but an asterisk or a [ could break our flavor of escaping. ### #42 @Denis-de-Bernardy 3 years ago +1 to suggested alternative implementation in the latest patch. Quick and dirty tests: function esc_like_orig($str) {
return str_replace(array("%", "_"), array("\\%", "\\_"), $str); } function esc_like_bad($str) {
return addcslashes($str, '_%'); } function esc_like1($str) {
return addcslashes($str, '_%\\'); } function esc_like2($str) {
return addcslashes(addslashes($str), '%_'); } foreach (array( 'foo%bar', 'foo_bar', 'foo\\bar', 'foo\'bar', 'foo"bar', 'foo\'"\\b\\%_a_%\\\'"r', ) as$val) {
echo "<h1>$val</h1>"; var_dump( esc_like_orig($val),
$wpdb->get_var($wpdb->prepare("SELECT %s LIKE %s", $val, esc_like_orig($val))),
$wpdb->get_var("SELECT '".esc_sql($val)."' LIKE '".esc_sql(esc_like_orig($val))."'"), esc_like_bad($val),
$wpdb->get_var($wpdb->prepare("SELECT %s LIKE %s", $val, esc_like_bad($val))),
$wpdb->get_var("SELECT '".esc_sql($val)."' LIKE '".esc_sql(esc_like_bad($val))."'"), esc_like1($val),
$wpdb->get_var($wpdb->prepare("SELECT %s LIKE %s", $val, esc_like1($val))),
$wpdb->get_var("SELECT '".esc_sql($val)."' LIKE '".esc_sql(esc_like1($val))."'"), esc_like2($val),
$wpdb->get_var($wpdb->prepare("SELECT %s LIKE %s", $val, esc_like2($val))),
`

### @miqrogroove 3 years ago

Also test chars not special in MySQL.

### #46 @miqrogroove 3 years ago

Anyone know if addcslashes was binary safe in PHP 5.2.4?

According to sandbox.onlinephpfunctions.com that function works in 5.2.16 but could give unexpected results in 5.1.6. The versions in between are not available there.

### #47 @miqrogroove 3 years ago

Found it in the 5.2.2 changelog. Looks like we can use it.

### #49 follow-ups: ↓ 50 ↓ 51 @wonderboymusic 3 years ago

• Milestone changed from Future Release to 4.0

10041.7.diff gets rid of some fuzz. I think this patch might need to be broken up into smaller pieces. There is a lot of stuff to test here. If done across multiple commits, each thing it touches can be looked at properly.

### #50 in reply to: ↑ 49 @miqrogroove 3 years ago

10041.7.diff gets rid of some fuzz.

That isn't fuzz, sir. You've removed the escaping of several strings that are not properly escaped.

### #51 in reply to: ↑ 49 @miqrogroove 3 years ago

I think this patch might need to be broken up into smaller pieces. There is a lot of stuff to test here. If done across multiple commits, each thing it touches can be looked at properly.

Just to make it more digestible, I will put the function changes in one patch, and all of the function calls in a separate patch. The function call patch will be large and touch many files, but each file could be committed separately without breaking anything.

### #52 @wonderboymusic 3 years ago

In 28711:

LIKE escape sanity:

• Deprecate like_escape()
• Add a method to $wpdb, ->esc_like(), and add unit tests$wpdb::esc_like() is not used yet. As such, many unit tests will throw Unexpected deprecated notice for like_escape. Subsequent commits will alleviate this.

Props miqrogroove.
See #10041.

In 28712:

Replace all uses of like_escape() with $wpdb->esc_like(). Props miqrogroove. See #10041. ### @miqrogroove 3 years ago Unit test for deprecated code. ### #54 @wonderboymusic 3 years ago • Owner set to wonderboymusic • Resolution set to fixed • Status changed from reopened to closed In 28714: Fix a unit test for the now deprecated function like_escape(). Props miqrogroove. Fixes #10041. ### #55 @miqrogroove 3 years ago • Resolution fixed deleted • Status changed from closed to reopened I'm re-checking everything before I get yelled at. :) comment.php needs some attention. I'll patch it and any other oversights. ### @miqrogroove 3 years ago Fixes a mistake and improves style. ### #56 @wonderboymusic 3 years ago In 28720: In WP_Comment_Query::get_search_sql(), don't double-like-escape. Props miqrogroove. See #10041. ### #57 @wonderboymusic 3 years ago That commit message is inaccurate - it adds wildcards. I spaced. ### #58 @miqrogroove 3 years ago It's okay :) In terms of style, I could make the same change in user.php. Pull the function calls out of the loop, because the arguments to prepare() are not changing. No bugs there though. Change it or close it? ### #59 @wonderboymusic 3 years ago Patch it if it's better ### @miqrogroove 3 years ago Style tweak. ### #60 @wonderboymusic 3 years ago • Resolution set to fixed • Status changed from reopened to closed In 28722: Set a variable for like-escaped string before looping in WP_User_Query::get_search_sql(). Props miqrogroove. Fixes #10041. ### #61 follow-up: ↓ 63 @johnjamesjacoby 3 years ago A note from Boone about deprecating like_escape() for a class method that's newly introduced in 4.0, and repercussions it will have for plugin authors: Given that the function is deprecated *and* its replacement is introduced in 4.0, plugin authors can't just swap it out and still support earlier versions of WP. So either we: • do nothing and just live with the deprecated notices, at least until we drop support for WP < 4.0 • create a wrapper function that does the necessary check and find/replace deprecated calls As much as I find the latter option unpleasant, I think it's probably the right way to go. In the past, we would go a few releases before adding deprecated notices to functions similar to this one (see: r13096 and #11388) – worth doing the same here? Consider also that using like_escape() did not previously require a touch to the$wpdb global that's now required using the new $wpdb->esc_like() method. Is there any objection to a general esc_like_sql() function that does the global touch for us, and making like_escape() a wrapper for it? ### #62 @wonderboymusic 3 years ago • Resolution fixed deleted • Status changed from closed to reopened Good questions - deprecation notice is a little brutal, if one of you wants to patch up a proposed solution/change/etc, would be amaze ### @johnjamesjacoby 3 years ago Remove deprecated notice. Introduce esc_like_sql() wrapper. ### #63 in reply to: ↑ 61 ; follow-up: ↓ 65 @miqrogroove 3 years ago Replying to johnjamesjacoby: Consider also that using like_escape() did not previously require a touch to the$wpdb global that's now required using the new $wpdb->esc_like() method. Is there any objection to a general esc_like_sql() function that does the global touch for us, and making like_escape() a wrapper for it? Using esc_like() without a database is a rare situation that would not be encountered by novice programmers. There's nothing wrong with adding another function, but it's unnecessary. Regarding deprecation: Seems like a lot of fuss over a one-line function. It won't be used in core anymore, and however that's signaled is great. ### #64 @miqrogroove 3 years ago The problem with 10041.8.diff is that we can't call esc_like() from like_escape(). They have different and incompatible output. ### #65 in reply to: ↑ 63 ; follow-up: ↓ 66 @johnjamesjacoby 3 years ago Replying to miqrogroove: Using esc_like() without a database is a rare situation that would not be encountered by novice programmers. There's nothing wrong with adding another function, but it's unnecessary. Not "without a database" – without defining the$wpdb global in the current PHP scope.

Regarding deprecation: Seems like a lot of fuss over a one-line function. It won't be used in core anymore, and however that's signaled is great.

Deprecated notices are good things for developer optics, but they're bad for plugins that wish to support older WordPress versions in newer plugin versions. We end up writing our own versions of these functions just to handle the function_exists() checks.

The problem with 10041.8.diff is that we can't call esc_like() from like_escape(). They have different and incompatible output.

There are a few places in r28712 where they are swapped out directly. If they are indeed two functions doing two separate things, deprecating like_escape() without an equal alternative isn't deprecation, it's elimination. If that's truly the intent, I'd suggest a post to make.wordpress.org/core explaining what the original problem is and what steps plugin authors need to take to secure their code and comply with this new approach.

(Just read the comments backlog; sorry for not having done so further. Noticed my patch goes against the decisions made above, but hopefully highlights the confusion that plugin authors will run into without documentation and a migration plan.)

### #66 in reply to: ↑ 65 ; follow-up: ↓ 67 @miqrogroove 3 years ago

There are a few places in r28712 where they are swapped out directly.

Indeed. And in a few places it wasn't possible. Even in core code.

The problem wasn't just with usage. The docs actually said like_escape() was SQL safe even though it was not. So we have to anticipate that message resulted in the function being used in many strange ways in plugins.

### #67 in reply to: ↑ 66 @johnjamesjacoby 3 years ago

The problem wasn't just with usage. The docs actually said like_escape() was SQL safe even though it was not. So we have to anticipate that message resulted in the function being used in many strange ways in plugins.

You're probably right. We audited usages in BuddyPress last year, and wrote tests for them, which then all broke when like_escape() was deprecated. :)

### #69 @boonebgorges 3 years ago

The problem wasn't just with usage. The docs actually said like_escape() was SQL safe even though it was not. So we have to anticipate that message resulted in the function being used in many strange ways in plugins.

I did a quick survey of plugins.svn.wordpress.org. It looks like maybe half the plugins using like_escape() are doing separate SQL sanitization (this includes BuddyPress, fwiw :) ), while the other half is not (or is doing it wrong). It's likely that the incorrect docs for like_escape() had something to do with this. Given the security implications, throwing a deprecated notice to get the attention of developers is probably prudent, and I think this ticket can probably be reclosed.

I strongly second jjj's suggestion above that we get a post somewhere (make.wordpress.org/plugins/) seems like an appropriate place) that explains the change and describes what they need to do to fix. If others agree that this is the right way to go, I can take the reins on that task.

### #70 @miqrogroove 3 years ago

Here's a description of the issue for developers:

You are welcome to copy that for wordpress.org.

### #71 @wonderboymusic 3 years ago

• Resolution set to fixed
• Status changed from reopened to closed
Note: See TracTickets for help on using tickets.