WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 6 months ago

Last modified 7 weeks ago

#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)

ticket29750.diff (834 bytes) - added by kitchin 6 months ago.
Untested patch.

Download all attachments as: .zip

Change History (15)

This ticket was mentioned in Slack in #core by jorbin. View the logs.


22 months ago

#2 @jorbin
22 months ago

@maxcutler or @josephscott - Would one of you be able to take a look here and see what is going on?

#3 @markoheijnen
22 months ago

  • Owner set to markoheijnen
  • Status changed from new to assigned

I will look into it.

#4 follow-up: @wonderboymusic
18 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Owner markoheijnen deleted

Anyone is welcome to patch this

#5 @Howdy_McGee
9 months 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.

#6 @rpayne7264
6 months ago

I am running into this issue due to distributed XML-RPC attacks, as well.

#7 in reply to: ↑ 4 @rpayne7264
6 months ago

Replying to wonderboymusic:

Anyone is welcome to patch this

How can I take ownership and work on a patch?

#8 @kitchin
6 months 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];
}

@kitchin
6 months ago

Untested patch.

#9 follow-up: @kitchin
6 months ago

Maybe you can work with that!

#10 in reply to: ↑ 9 @rpayne7264
6 months 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 @rpayne7264
6 months 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 @SergeyBiryukov
6 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.7

#13 @SergeyBiryukov
6 months ago

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

In 38600:

XML-RPC: Check the minimum number of arguments in ::wp_getUsersBlogs() and ::blogger_getUsersBlogs().

Props kitchin for initial patch.
Fixes #29750.

#14 @galbaras
7 weeks ago

#38300 was marked as a duplicate.

Note: See TracTickets for help on using tickets.