

WordPress 2.8.3 Remote admin reset password - sucuri2
http://blog.sucuri.net/2009/08/wordpress-283-remote-admin-reset.html

======
mdasen
The exploit has WordPress change the password and email the new password to
the user. You aren't able to get access to someone else's blog through this
and they shouldn't lose access as long as they still have access to the email
account (and check it regularly to see that the password has been changed),
but it's definitely an annoying bug.

~~~
jgrahamc
How does it actually work? It looks like reset_password needs to get a valid
key to be able to get the user's details from the database.

    
    
      	$user = $wpdb->get_row($wpdb->prepare("SELECT * FROM $wpdb->users
                WHERE user_activation_key = %s", $key));
    

How does that line of code retrieve a valid $user? I looked at the bug fix
which filters out the $key being an array, and that will prevent you from
passing an array, but I don't see how the array gets you over the hurdle
above.

~~~
mdasen
I'm guessing that WordPress has user_activation_key set to '' (empty string)
when the user hasn't requested a password reset or similar email-confirmed
operation. So, it checks to see if it is empty, it isn't and so it checks to
find the user(s) who have empty user_activation_key values. It's using a
get_row() method which indicates that it will just return one and not an array
which would have caused a type error.

UPDATE: yes, user_activation_key is an empty string when WordPress hasn't
received a request from a user to do something that would require it to be
set.

~~~
jgrahamc
Nasty stuff. I notice from the code that once user_activation_key has been set
it doesn't get reset. So this vulnerability will not work for someone who has
previously asked for password reset.

------
ionfish
As I said on the other thread...

<http://news.ycombinator.com/item?id=755244>

It's worth noting that this has already been patched on trunk and the latest
stable branch (2.8), so if you run svn update now you'll get this fix.

<http://core.trac.wordpress.org/timeline>

The patches there are also more extensive. I would recommend getting it direct
from the WordPress svn repository rather than using random third-party
patches.

These are the commits so far, in sequence. As we can see, the erroneous
blacklist approach has been replaced in [11799] by a whitelist one (is_array
replaced by !is_string).

<http://core.trac.wordpress.org/changeset/11797>

<http://core.trac.wordpress.org/changeset/11799>

<http://core.trac.wordpress.org/changeset/11801>

<http://core.trac.wordpress.org/changeset/11802>

<http://core.trac.wordpress.org/changeset/11803>

(Note that these are just the changes on trunk, not the commits to the 2.8
stable branch, which are identical.)

------
Jem
Linked site appears to be an uncredited duplicate of:
<http://www.milw0rm.com/exploits/9410>

~~~
qeorge
The author linked the fulldisclosure URL right at the top of the post, and the
relevant text is enclosed in a blockquote tag.

That said, I missed it on the first pass. He could have prepended "Source:" to
the link to further clarify.

~~~
Jem
I don't remember seeing the link the first time and I did look - but I could
have missed it. Thanks for the heads up. :)

------
paulgb
A few notes on PHP's behaviour to anyone trying to follow the code:

In PHP, GET and POST arguments that look like arrays are automatically
converted to arrays. So "key[]=" in the query string has the result that
$_GET['key'] = array('');

empty(array('')) = false, because the array has one element.

$wpdb->prepare uses the PHP function vsprintf, which has undocumented
behaviour when it gets an array as an argument. The (undocumented) behaviour
is (apparently) that if the contents of the array are concatenated together
and treated as a single string.

~~~
paulgb
Actually, I was wrong about the last one. vsprintf does what it is supposed
to. The issue is that $wpdb->prepare can take an array of strings as one
argument (as in $wpdb->prepare("%s - %s", array("a","b"))), or as multiple
arguments ($wpdb->prepare("%s - %s", "a", "b")).

The code that calls $wpdb->prepare in this exploit intends to do the latter,
but because there is only one argument which happens to be an array, it ends
up that the former is used.

------
brown9-2
Checking out the trac link in the article to the Wordpress source code shows
some other interesting things in the PHP source:

    
    
      do_action('retreive_password', $user_login);  // Misspelled and deprecated
      do_action('retrieve_password', $user_login);
    

Isn't this a verbatim example from the "Signs you are a bad programmer" list
from a few days ago? Code that does nothing but is left in anyway to "make
sure it all works".

~~~
ionfish
It's not code that does nothing, it's there to support a deprecated public
API. Someone misspelled 'retrieve' once upon a time, and it made it into the
public API, now they have to support it. It's unfortunate, but this is not the
wrong way to behave now the issue is there.

~~~
aw3c2
Things like this are probably what made the Internet Explorer. Mistakes should
not be carried with you, eliminate them as soon as you can.

------
trezor
Things like this makes me once again appreciate languages with a static type-
system. Not in any way claiming they guarantee bug- or exploit-free code, but
these kind of bugs/vulnerabilities simply does not exist.

~~~
pixelbath
Which language with a static type system accepts statically-typed variables
via querystring?

~~~
trezor
The query-string is (obviously) a string and there is no way to treat it as
anything else in a static type system.

But in a static type system you will have to cast or convert query-string data
to whatever you want them processed as before you can use them as anything
besides plain strings.

If you want it to be an int, convert it. If you want it to be a datetime,
convert it. If you want it to be an array of whatever, convert it.

If the data is not in the expected format (like in this case), the conversion
will fail.

In this example we saw how modifying _one_ query-string parameter had effects
down to the _signature_ of a function. Something like that will never happen
in a static type system.

