Opened 15 years ago
Closed 10 years ago
#10041 closed defect (bug) (fixed)
like_escape() should escape backslashes too
Reported by: | miau_jp | Owned by: | wonderboymusic |
---|---|---|---|
Milestone: | 4.0 | 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 (19)
Change History (90)
#1
@
15 years ago
- Keywords needs-patch early added
- Milestone changed from Unassigned to 2.9
- Version set to 2.8
#3
@
15 years ago
this is probably good enough as a fix:
return addcslashes($text, '%_');
It's like... WP isn't mb_string safe anyway.
#6
@
15 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.
#10
@
15 years ago
#12123 closed as dupe. See also [12961].
In the course of discussing the input paths in IRC, we came to some conclusions:
- Super-global variables should be addslashes() instead of wpdb->escape(). That was changed in [12961].
- 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.
- 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.
- Most instances of LIKE do not currently use like_escape(). A partial list was included in #12060.
#11
@
15 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 '|'
#12
@
15 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.
#13
@
15 years ago
@robert: the issue isn't quotes, which are properly handled. the issue is backslashes and percent chars.
#14
@
15 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.
#15
@
15 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
@
15 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
@
15 years ago
tiny note: I think like_escape() should expect *unslashed* data.
see also #12402
#18
in reply to:
↑ 17
@
15 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.
#21
@
14 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.
@
14 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.
#22
@
12 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
@
12 years ago
10041.6.diff would octuple-slash the search value in taxonomy.php. Please correct that.
#25
@
11 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
@
11 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
@
11 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
@
11 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.
#29
follow-up:
↓ 30
@
11 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), '%_'); }
#30
in reply to:
↑ 29
@
11 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.
This ticket was mentioned in IRC in #wordpress-dev by miqrogroove. View the logs.
10 years ago
#33
@
10 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.
10 years ago
#35
@
10 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.
#36
in reply to:
↑ 28
;
follow-up:
↓ 37
@
10 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
@
10 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
@
10 years ago
Starting to see multiple conflicted files in trunk. Nacin, is there any chance of getting this committed?
#41
@
10 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
@
10 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))), $wpdb->get_var("SELECT '".esc_sql($val)."' LIKE '".esc_sql(esc_like2($val))."'") ); }
In the above, esc_like1()
(suggested in the patch) and esc_like2()
(suggested earlier) cover the bases as needed, in contrast to the current implementation (esc_like_orig()
) and esc_like_bad()
which was incorrectly suggested in the original report.
I'd add the keyword "commit", but I don't seem to be able to.
#43
@
10 years ago
I still think it should be renamed 'quote_like()` to avoid any potential for plugin or theme devs to assume that it's secure in any way, that said.
#44
@
10 years ago
One last note, in the interest of highlighting differences between MySQL and Postgres, as the latter is oftentimes much stricter than the former.
Postgres will only resolve the LIKE with backslashes escaped one more time as below — which none of the suggested functions compute:
select E'foo\'"\\b\\%_a_%\\\'"r' like E'foo\'"\\\\b\\\\\%\_a\_\%\\\\\'"r'; select 'foo''"\\b\\%_a_%\\''"r' like 'foo''"\\\\b\\\\\%\_a\_\%\\\\''"r';
MySQL, incidentally, is also happy with the above:
select 'foo\'"\\b\\%_a_%\\\'"r' like 'foo\'"\\\\b\\\\\%\_a\_\%\\\\\'"r'; select 'foo''"\\b\\%_a_%\\''"r' like 'foo''"\\\\b\\\\\%\_a\_\%\\\\''"r';
#45
@
10 years ago
Oops, nevermind, I forgot to escape it in my Postgres tests. This implementation yields the correct output after getting SQL escaped:
return addcslashes($str, '_%\\');
#46
@
10 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.
This ticket was mentioned in IRC in #wordpress-dev by miqrogroove. View the logs.
10 years ago
#49
follow-ups:
↓ 50
↓ 51
@
10 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
@
10 years ago
Replying to wonderboymusic:
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
@
10 years ago
Replying to wonderboymusic:
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.
#54
@
10 years ago
- Owner set to wonderboymusic
- Resolution set to fixed
- Status changed from reopened to closed
In 28714:
#55
@
10 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.
#58
@
10 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?
#61
follow-up:
↓ 63
@
10 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
@
10 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
#63
in reply to:
↑ 61
;
follow-up:
↓ 65
@
10 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 generalesc_like_sql()
function that does the global touch for us, and makinglike_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
@
10 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
@
10 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
@
10 years ago
Replying to johnjamesjacoby:
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
@
10 years ago
Replying to miqrogroove:
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. :)
This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.
10 years ago
#69
@
10 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
@
10 years ago
Here's a description of the issue for developers:
http://www.miqrogroove.com/blog/2014/like-escape-deprecated/
You are welcome to copy that for wordpress.org.
Confirming...