#29750 closed defect (bug) (fixed)
array_unshift() PHP Warning when wp.getUsersBlogs is attacked (poorly)
Reported by: | kitchin | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 4.7 | Priority: | normal |
Severity: | normal | Version: | 4.0 |
Component: | XML-RPC | Keywords: | has-patch |
Focuses: | Cc: |
Description
An error_log is getting cluttered with these due to a distributed XML-RPC attack:
PHP Warning: array_unshift() expects parameter 1 to be array, string given in /../wp-includes/class-wp-xmlrpc-server.php on line 518
I tracked it down to this attack, username without password:
<?xml_version] => "1.0" encoding="iso-8859-1"?> <methodCall><methodName>wp.getUsersBlogs</methodName><params> <param><value>someuser</value></param> <param><value></value></param> </params></methodCall>
What's happening: as in Ticket #16980 untyped empty values are ignored by IRX and the expected 2-element array becomes a singleton. Then the singleton becomes a string (line 455 of class-IXR.php), and hence the array_unshift() warning.
This all ends up at class-wp-xmlrpc-server.php Line 223:
$user = wp_authenticate( 'o', 'm')
using the [1]
and [2]
elements of the string someuser
.
It would be easy to bail at the first line of function wp_getUsersBlogs if $args is not an array. We could issue the same warning as a login failure if we don't want to alert the script kiddies to their errors (that's the current behavior).
There are a few other functions with the same problem, if it is a problem. We could try to use the signature interface of IXR or just do it the easy way.
Attachments (1)
Change History (15)
This ticket was mentioned in Slack in #core by jorbin. View the logs.
9 years ago
#4
follow-up:
↓ 7
@
9 years ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to Future Release
- Owner markoheijnen deleted
Anyone is welcome to patch this
#5
@
8 years ago
This has become a big issue recently on my sites. Could we not just verify at the beginning that we're working with an array? Most the issues I've seen in my debug logs is that it's either being passed null
or a string instead of the expected array.
#7
in reply to:
↑ 4
@
8 years ago
Replying to wonderboymusic:
Anyone is welcome to patch this
How can I take ownership and work on a patch?
#8
@
8 years ago
I see you have commits in plugins. For core WP, my experience has been to checkout trunk or develop in SVN and make a patch. The owner will review it.
Also I have some new notes.
- Like you say, it's an easy fix if we ignore trying to get fancy with the IXR interface - telling it what to expect, etc.
- Might also want to check that trunk still has this bug!! After the current release branch 4.6.x, the IXR classes were relocated from a single file to a directory. I don't know what went into that, or if it was meant to fix anything.
- Feeding test XML into WP is not that hard but I can't recall if it's covered by unit tests - apparently not. That might slow you down if the bug owner asks for tests. I did my testing manually.
- Best practice is to use the develop tree and run command line unit tests on your patched tree.
- If anyone is tracing IXR code, this is where the code was relocated:
from
wp-includes/class-IXR.php Line 455 (in WP 4.0)
to
wp-includes/IXR/class-IXR-server.php Line 103 (in trunk)
It looks like this:
// Perform the callback and send the response
if (count($args) == 1) {
// If only one parameter just send that instead of the whole array
$args = $args[0];
}
#10
in reply to:
↑ 9
@
8 years ago
Replying to kitchin:
Maybe you can work with that!
Your patch looks fine, to me, and should be applied, ASAP.
I have addressed my situation by turning off XML RPC completely, by placing the following line in a MU plugin:
add_filter( 'xmlrpc_enabled', '__return_false' );
#11
@
8 years ago
I just realized that in the case of a call to wp_getUsersBlogs, the xmlrpc_enabled filter will not stop the error that is the subject of this ticket.
#12
@
8 years ago
- Keywords has-patch added; needs-patch removed
- Milestone changed from Future Release to 4.7
@maxcutler or @josephscott - Would one of you be able to take a look here and see what is going on?