WordPress 2.8.3 Remote admin reset password | Hacker News Reader

WordPress 2.8.3 Remote admin reset password

blog.sucuri.net

32 pointssucuri220 comments

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.

Seems kind of embarrassing, though, as far as bugs go...

so long as your email account isn't hacked either.

If someone has access to your email account, they always have access to your WordPress since that's how the password reset functionality works.

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.

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.

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.

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

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

Both were taken from the original advisory posted at Full-disclosure...

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.

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. :)

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.

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.

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

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.

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

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.

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

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.

Reply on news.ycombinator.com