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.
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.
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.
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.
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.
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).
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.
23 comments
[ 0.35 ms ] story [ 68.7 ms ] threadUPDATE: 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.
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.
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.)
That said, I missed it on the first pass. He could have prepended "Source:" to the link to further clarify.
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.
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.