Making a Change

I officially parted ways with my company today. I have serious appreciation for the opportunities and education I've gained through working with their developers, clients, and systems, but I've come to a point where I have made huge improvements there, and want to work on other things. I feel like, at this stage, my skills will be better put to use elsewhere, and it is time for me to move on.

I've been asked to join the team at Foreground Security. While I'm a natural skeptic, I am very excited about this opportunity. I'll be working with a very different set of applications, developers, and clients. The people over there have been great so far, and appear to be quite supportive of my continuing research in the WebAppSec field. I'm looking forward to some seriously cool stuff, and I couldn't be happier about the situation.

With this change, I'll also let you know there will be a shift in the material that I post here. Black Hat and Defcon had some really eye-opening moments about the state of the security community, what we're doing right, and the many things we're doing wrong. I still need to take some time to process all of the ideas that came out of the conferences, but suffice it to say, I'll be approaching future research very differently.

Finally, I want to throw out a thank you to all the people who have helped me out behind the scenes over the last few months on the job change, research, and a variety of other issues- I won't name names for fear of leaving somebody out (or publicizing somebody that doesn't want it), but you all know who you are, and I'm extremely appreciative. Even the opportunities that didn't work out resulted in me meeting bright people and having incredible discussions- things that I would love to one day follow up on.

The security community really is one of the greatest groups of people on the planet. Thank you all for being here.

PHP Casting Errors in Wordpress

Did you see the Wordpress vulnerability that was published last night?

While Wordpress vulns aren't really news, this one was fascinating. All it does is allow an attacker to reset the adminsitrator's password- locking him out of the account. Annoying, but not fatal. I'm interested in the vuln itself, as it stems from PHP's flexible casting. In particular, the following lines of code (paraphrased for context and clarity):

$key = preg_replace('/[^a-z0-9]/i', '', $_GET['key']);

if ( empty( $key ) ){
return new WP_Error('invalid_key', __('Invalid key'));
}

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

This code can be exploited by passing the vulnerable script an empty array rather than a string for $_GET['key']. You can pass an array in a query string using the [] syntax:

wp-login.php?action=rp&key[]=

Go back and step through the vulnerable code again. The value of $_GET['key'] would look like this in PHP form:

$_GET['key'][0]='';

The array is handed to preg_replace, which, when given an array to work with, will perform the regex substitution on each element of the array, then return that array. Again, this array contains one element, an empty string, so when it hits the "if(empty($key))" line, it returns false (as the array is not empty). This, in turn, is important because the wpdb->prepare() statement receives an array when it is expecting a string, flattens that array out, and gives you the following query:

SELECT * FROM `users` WHERE user_activation_key = ''

Since the admin user is the first row in the database, and his activation key is blank (assuming nobody has performed a password reset on the account yet), that user is returned, and his password is changed.

That probably wasn't the best explanation, but if you're still with me, that brings us to the question of "what did they do wrong?" The first problem was assuming that $_GET['key'] was a string. While the programmer checked that string for malicious characters (in the preg_replace), he didn't check the type of the variable. Flexible casting of variables is common in PHP apps, but as this example demonstrates, this can be dangerous. As part of your input validation process, you should be either checking or forcibly casting the type of that input. I suspect that the programmer thought he was forcibly casting the input with preg_replace(), but was not aware of how it handles arrays.

Here is the official fix:

if ( empty( $key ) || ( is_array( $key )){
return new WP_Error('invalid_key', __('Invalid key'));
}

Can you see the problem here? This is a classic bad approach to fixing security bugs- patching the exploit, not the vulnerability. Instead of checking whether the input is what you expect (is_string), they are making sure that it isn't what they don't expect (is_array). While this does fix the issue, it is a blacklist approach, not the whitelist approach that would be recommended. What will happen if an attacker figures out how to pass another data type to this function? It may break, it may not. Either way, it's not very fault-tolerant coding.

The reason I'm bringing it up is that this is an excellent case study of an underappreciated programming bug. As useful as PHP's loose casting and overloaded functions can be, you still need to validate your inputs and explicitly verify that they are what you expected.

Thanks to Rafal Los for making me look at this, and working through the mess that is the Wordpress codebase with me.