Learning from False Positives

False positives are a common aspect of security research; there is no way to avoid them. No matter your hacking technique, tools, or style, you'll eventually encounter a piece of code that appears vulnerable, yet the exploit may not work for some reason.

False Positives

At that point, you have two choices:

  1. Move on to the next vulnerability and forget about it
  2. Dig into it and understand why your payload is not firing, what's the missing piece, what's hiding in there

In my opinion, for passionate hackers #2 is the only viable option, because curiosity is what really drives security research. Years ago, I was reviewing a WordPress plugin and I came across a function containing this code:

$transaction_id = $_GET["transaction_id"];

$meta = $wpdb->get_results( "SELECT post_id FROM " . $wpdb->postmeta ." WHERE meta_key = '_transaction_id' AND meta_value= '" . $transaction_id  . "'" );

And of course, I immediately thought I was in front of a SQL injection vulnerability, because an untrusted user input (coming from $_GET) was passed to the SQL query without any form of sanification, and the SQL statement was not prepared. But when I tried to exploit the injection even with the most basic of the payloads, it didn't fire. Ok, maybe it's me I thought, let's try to exploit it with a SQL-specific automation tool: SQLmap.

# request.txt contains the raw HTTP request intercepted with a proxy
sqlmap -r request.txt -p transaction_id --risk=3 --level=5

Still no luck! I had to understand what was going on.

I modified the code to give me visibility in the parameter passed to the query, and finally got a hint: the payload was escaping the quotes, so ' or sleep(5)= was turned into \' or sleep(5)= making the single quote ineffective to "close" the statement and make the injection effective. But yet, why?

Bad software design somehow is helping secuirty

The reason why the $_GET["transaction_id"] parameter was modified apparently without any explicit code transforming it, is because WordPress doing something at the "root" level which in my opinion can be considered bad software design, is escaping PHP superglobals. There is a function called wp_magic_quotes() that literally turns $_GET into add_magic_quotes( $_GET ) and add_magic_quotes() itself calls the native PHP addslashes() function.

function wp_magic_quotes() {
  // Escape with wpdb.
  $_GET    = add_magic_quotes( $_GET );
  $_POST   = add_magic_quotes( $_POST );
  $_COOKIE = add_magic_quotes( $_COOKIE );
  $_SERVER = add_magic_quotes( $_SERVER );

  // Force REQUEST to be GET + POST.
  $_REQUEST = array_merge( $_GET, $_POST );
}

Of course, no PHP programmer would be happy having the underlying framework to modify superglobals and I am not even sure why this feature was introduced some time ago, but the only reason I can see is to mitigate in bulk hundreds of security vulnerabilities that plugin developers may introduce by using $_GET, $_POST insecurely.

Trying to explore more WordPress plugins, confirms that many times unsafe SQL methods like get_results(), get_row(), get_var() are called insecurely potentially opening the door to really bad security issues.

Is wp_magic_quotes() a final solution?

Not really. While it mitigates the risk in a significant way, there are many scenarios where is not effective. Let's see some practical examples:

The code is executed before wp_magic_quotes() is loaded by the core

A few weeks ago, a really dangerous unauthenticated SQL injection was discovered in WP Fastest Cache, happened on a variable ($_COOKIE) that is supposed to be passed through wp_magic_quotes():

foreach ((array)$_COOKIE as $cookie_key => $cookie_value){
    if(preg_match("/wordpress_logged_in/i", $cookie_key)){
        $username = preg_replace("/^([^\|]+)\|.+/", "$1", $cookie_value);
        break;
    }
}

$res = $wpdb->get_var("SELECT ...ON `$wpdb->users`.`user_login` = \"$username\"...);

But the function containing the code was called at plugin load time, which is before wp_magic_quotes() has been called on the request data and therefore vulnerable.

The injection point is unslashed

If the injectable parameter is passed through a function that removes the backslashed (eg. wp_unslash()), it neutralizes the wp_magic_quotes() action and makes the query vulnerable again, as soon on CVE-2020-5768.

Multiple parameter encoding

As seen in one of the most infamous SQL injection vulnerabilities affecting WooCommerce, multiple encoding layers on parameters can allow an attacker to get around escaping mechanism with a multiple-encoded payload.

Edge cases on less common encodings

addslashes() is not considered bullet-proof for SQL injection, because with some character encodings the system can be tricked by a multi-byte character that ends in 0x5c. More details on this evasive technique on this old but gold blog post. Note that UTF-8 is not vulnerable to this attack scenario, making this last point an edge case, especially on WordPress.

So, how do we write secure SQL statements in WordPress?

The general rule is to always use the wpdb::prepare method, which as the name suggests, prepares a SQL query for a safe execution.

$wpdb->prepare(
    "SELECT * FROM `table` WHERE `column` = %s AND `field` = %d OR `other_field` LIKE %s",
    array( 'foo', 1337, '%bar' )
);

$wpdb->prepare(
    "SELECT DATE_FORMAT(`field`, '%%c') FROM `table` WHERE `column` = %s",
    'foo'
);

I also personally like to explicitly enforce the type, and further restrict parameters to what I expect to be, for example:

function validate_id($id) {
    return preg_match('/^\d+$/', $id);
}

And return early if the condition is not met, maybe even logging the incident to detect and investigate any intrusion attempts.

Not an isolated case

As we have seen, what's considered an uncommon design pattern (escaping PHP superglobals) has somehow protected WordPress and millions of sites from a huge wave of SQL injection vulnerabilities. And exploring the WordPress codebase we can find other weird code like this one:

...
/*
 * Double serialization is required for backward compatibility.
 * See https://core.trac.wordpress.org/ticket/12930
 * Also the world will end. See WP 3.6.1.
 */
if ( is_serialized( $data, false ) ) {
  return serialize( $data );
}
...

They say it's backward compatibility, but I have a feeling that the double serialization is also protecting from a serious risk of PHP Object Injection vulnerabilities. But maybe this is a topic for another blog post.

Conclusion

I started this blog post highlighting how a false positive, which is basically a mistake, can lead us to very interesting discoveries and a deeper understanding of our target and our codebase.

This is not only true with hacking, I believe is very good life advice.

Now, look back at your false positives, what did they teach you?