WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 38 hours ago

#10041 reopened defect (bug)

like_escape() should escape backslashes too

Reported by: miau_jp Owned by:
Milestone: Future Release Priority: high
Severity: normal Version: 2.8
Component: Formatting Keywords: 4.0-early has-patch
Focuses: Cc:

Description

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

Attachments (8)

10041.diff (425 bytes) - added by Denis-de-Bernardy 5 years ago.
10041.2.diff (436 bytes) - added by Denis-de-Bernardy 4 years ago.
10041.3.diff (1.2 KB) - added by Denis-de-Bernardy 4 years ago.
10041.4.diff (2.2 KB) - added by Denis-de-Bernardy 4 years ago.
10041.5.diff (1.5 KB) - added by ampt 3 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.
10041.6.diff (1.6 KB) - added by wonderboymusic 15 months ago.
miqro-10041.patch (28.1 KB) - added by miqrogroove 7 days ago.
miqro-10041.2.patch (28.1 KB) - added by miqrogroove 7 days ago.
Fixed a missing arg in schema.php.

Download all attachments as: .zip

Change History (39)

comment:1 Denis-de-Bernardy5 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)

comment:2 Denis-de-Bernardy5 years ago

  • Owner set to Denis-de-Bernardy
  • Status changed from new to accepted

comment:3 Denis-de-Bernardy5 years ago

this is probably good enough as a fix:

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

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

comment:4 Denis-de-Bernardy5 years ago

  • Keywords has-patch added; needs-patch removed

Denis-de-Bernardy5 years ago

comment:5 Denis-de-Bernardy5 years ago

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

comment:6 westi4 years ago

  • Keywords has-unit-tests added
  • 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.

comment:7 westi4 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.

comment:8 westi4 years ago

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

comment:9 westi4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:10 miqrogroove4 years ago

#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].
  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 15 months ago by miqrogroove (previous) (diff)

comment:11 miqrogroove4 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 15 months ago by miqrogroove (previous) (diff)

comment:12 miqrogroove4 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.

Version 0, edited 4 years ago by miqrogroove (next)

comment:13 Denis-de-Bernardy4 years ago

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

comment:14 miqrogroove4 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 15 months ago by miqrogroove (previous) (diff)

comment:15 miqrogroove4 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().

comment:16 miqrogroove4 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?

comment:17 follow-up: Denis-de-Bernardy4 years ago

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

see also #12402

comment:18 in reply to: ↑ 17 miqrogroove4 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-Bernardy4 years ago

comment:19 Denis-de-Bernardy4 years ago

  • Keywords has-patch added

Denis-de-Bernardy4 years ago

Denis-de-Bernardy4 years ago

comment:20 nacin4 years ago

  • Milestone changed from 3.0 to 3.1

Early 3.1 per the dev chat today.

comment:21 nacin3 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.

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

wonderboymusic15 months ago

comment:22 wonderboymusic15 months 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

comment:23 miqrogroove15 months ago

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

comment:24 ryan12 months ago

  • Milestone changed from 3.6 to Future Release

comment:25 miqrogroove4 weeks 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.

comment:26 miqrogroove3 weeks 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++.

comment:27 follow-up: miqrogroove3 weeks 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.

comment:28 in reply to: ↑ 27 Denis-de-Bernardy3 weeks 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 weeks ago by Denis-de-Bernardy (previous) (diff)

comment:29 follow-up: Denis-de-Bernardy3 weeks 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 weeks ago by Denis-de-Bernardy (previous) (diff)

miqrogroove7 days ago

comment:30 in reply to: ↑ 29 miqrogroove7 days 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.

miqrogroove7 days ago

Fixed a missing arg in schema.php.

comment:31 miqrogroove38 hours ago

  • Keywords has-patch added; dev-feedback removed
Note: See TracTickets for help on using tickets.