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.