like_escape() should escape backslashes too

[miau_jp]

The like_escape() function doesn't escape backslashes.

source:trunk/wp-includes/formatting.php@11518#L2260​

	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, '%_');
	}

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)

this is probably good enough as a fix:

return addcslashes($text, '%_');

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

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

We can fix this early in the 3.0 dev cycle.

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

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

#12123 closed as dupe. See also [12961].

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].

2. 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.

3. 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.

4. Most instances of LIKE do not currently use like_escape(). A partial list was included in #12060.

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 '|'

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.

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

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.

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

Westi, if I were to write a full diff based on the strategy in my previous reply, would you commit it?

tiny note: I think like_escape() should expect *unslashed* data.

see also #12402

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.

Early 3.1 per the dev chat today.

No traction, feedback, or testing. Future for now.

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.

Updated the patch so it applies against trunk without dying - this can be perused as part of the overall slashing inspection

10041.6.diff would octuple-slash the search value in taxonomy.php. Please correct that.