33 stories
·
3 followers

Disclosure: WordPress WPDB SQL Injection - Technical

1 Share

Today, a significant SQL-Injection vulnerability was fixed in WordPress 4.8.3. Before reading further, if you haven’t updated yet stop right now and update.

The foundations of this vulnerability was reported via Hacker-One on September 20th, 2017.

This post will detail the technical vulnerability as well as how to mitigate it. There is another post which deals with the background and time-lines.

What Site Owners Should Do

Simply upgrade to 4.8.3 and update any plugins that override $wpdb (like HyperDB, LudicrousDB , etc). That should be enough to prevent these sorts of issues.

What Hosts Should Do

Upgrade wp-db.php for clients.

There may be some firewall rules in the mean time that you could implement (such as blocking %s and other sprintf() values), but your mileage may vary.

What Plugin Developers Should Do

To prevent this issue? Nothing, it’s been mitigated at the WP layer.

In general however, go through and remove all user input from the $query side of ->prepare(). NEVER pass user input to the query side. Meaning, never do this (in any form):

$where = $wpdb->prepare(" WHERE foo = %s", $_GET['data']);$query = $wpdb->prepare("SELECT * FROM something $where LIMIT %d, %d", 1, 2);

This is known as “double-preparing” and is not a good design.

Also, don’t do this:

$where = "WHERE foo = '" . esc_sql($_GET['data']) . "'";$query = $wpdb->prepare("SELECT * FROM something $where LIMIT %d, %d", 1, 2);

This is also conceptually unsafe.

Instead, build your queries and arguments separately, and then prepare in one shot:

$where = "WHERE foo = %s";$args = [$_GET['data']];$args[] = 1;$args[] = 2;$query = $wpdb->prepare("SELECT * FROM something $where LIMIT %d, %d", $args);

Let’s look at why:

The Original Vulnerability

Many months ago, a vulnerability was reported dealing with how WPDB internally prepares vulnerable code. Let’s talk about the original vulnerability.

To understand it, you need to first understand the internals of WPDB::prepare. Let’s look at the source (before 4.8.2):

public function prepare( $query, $args ) {    if ( is_null( $query ) )        return;    // This is not meant to be foolproof -- but it will catch obviously incorrect usage.    if ( strpos( $query, '%' ) === false ) {        _doing_it_wrong( 'wpdb::prepare', sprintf( __( 'The query argument of %s must have a placeholder.' ), 'wpdb::prepare()' ), '3.9.0' );    }    $args = func_get_args();    array_shift( $args );    // If args were passed as an array (as in vsprintf), move them up    if ( isset( $args[0] ) && is_array($args[0]) )        $args = $args[0];    $query = str_replace( "'%s'", '%s', $query ); // in case someone mistakenly already singlequoted it    $query = str_replace( '"%s"', '%s', $query ); // doublequote unquoting    $query = preg_replace( '|(?<!%)%f|' , '%F', $query ); // Force floats to be locale unaware    $query = preg_replace( '|(?<!%)%s|', "'%s'", $query ); // quote the strings, avoiding escaped strings like %%s    array_walk( $args, array( $this, 'escape_by_ref' ) );    return @vsprintf( $query, $args );}

Notice three things. First, it uses vsprintf (which is basically identical to sprintf) to replace placeholders with values. Second, it uses str_replace to quote placeholders properly (even unquoting first to prevent double quotes). Third, if passed a single argument and that argument is an array, then it will replace the arguments with the value of that array. Meaning that calling $wpdb->prepare($sql, [1, 2]) is identical to calling $wpdb->prepare($sql, 1, 2). This will be important later.

The original reported vulnerability (months ago, not by me) relied on the following theoretical (well, many plugins had this pattern) server-side code:

$items = implode(", ", array_map([$wpdb, '_real_escape'], $_GET['items']));$sql = "SELECT * FROM foo WHERE bar IN ($items) AND baz = %s";$query = $wpdb->prepare($sql, $_GET['baz']);

The original reported vulnerability used a sneaky feature in vsprintf to allow you to “absolute reference” arguments. Let’s look at an example:

vsprintf('%s, %d, %s', ["a", 1, "b"]); // "a, 1, b"vsprintf('%s, %d, %1$s', ["a", 2, "b"]); // "a, 2, a"

Notice that %n$s will not read the next argument, but the one at the position specified by n.

We can use this fact to inject into the original query. Imagine that we instead passed the following information to the request:

$_GET['items'] = ['%1$s'];$_GET['baz'] = "test";

Now, the query will be changed to SELECT * FROM foo WHERE bar IN ('test') AND baz = 'test'; Not good (we’ve successfully changed the meaning of the query), but also not incredibly bad on the surface.

There’s one other key piece of information that the original report included to change this into a full-blown SQL Injection. sprintf also accepts another type of parameter: %c which acts like chr() and converts a decimal digit into a character. So now, the attacker can do this:

$_GET['items'] = ['%1$c) OR 1 = 1 /*'];$_GET['baz'] = 39;

Checking an ASCII table, 39 is the ASCII code for ' (a single quote). So now, our rendered query becomes:

SELECT * FROM foo WHERE bar IN ('') OR 1 = 1 /*' AND baz = 'test';

Which means that it’s injected.

This sounds like a long shot. It requires passing in attacker-controlled input to the query parameter of prepare. But as it turns out, this exists in core in /wp-includes/meta.php:

if ( $delete_all ) {  $value_clause = '';  if ( '' !== $meta_value && null !== $meta_value && false !== $meta_value ) {    $value_clause = $wpdb->prepare( " AND meta_value = %s", $meta_value );  }  $object_ids = $wpdb->get_col( $wpdb->prepare( "SELECT $type_column FROM $table WHERE meta_key = %s $value_clause", $meta_key ) );}

The Original Fix

When 4.8.2 was released, it included a “fix” for the above issue. The “fix” was entirely contained in WPDB::prepare(). The attempt to fix was basically the addition of a single line:

$query = preg_replace( '/%(?:%|$|([^dsF]))/', '%%\\1', $query );

This does two fundamental things. First, it removes any sprintf token other than %d, %s and %F. This should nullify the original vulnerability since it relied on %c (or so it seemed). Second, it removed the ability to do positional substitutions (meaning %1$s was no longer valid).

This caused a massive outrage. WordPress originally (years ago) documented that you should only use %d, %s and %F. In fact, here’s the quote from their docs:

This function only supports a small subset of the sprintf syntax; it only supports %d (integer), %f (float), and %s (string). Does not support sign, padding, alignment, width or precision specifiers. Does not support argument numbering/swapping.

Even though it was documented as undocumented, several million queries in third party code (millions of lines of affected code) used the former syntax (securely I may add).

WordPress’s response to the outrage was won’t fix, sorry. They cited security as the reason and refused to elaborate.

The First Issue With The Fix.

Looking at the fix, something felt wrong. To me, the vulnerability was with passing user-input to the query side of prepare, even if passed through a “escaper” before.

The original proof-of-concept I shared was the following. Given the formerly secure query:

$db->prepare("SELECT * FROM foo WHERE name= '%4s' AND user_id = %d", $_GET['name'], get_current_user_id());

With the change made in 4.8.2, the %4s will be rewritten to %%4s (in other words a literal % followed by a literal 4s. No substitution will be done). This will mean the %d would be rebound to $_GET['name'], giving the attacker control over the user id. This could be used for privilege escalations, etc.

The response (a day later) was thank you followed by a close as “we don’t support that”. I replied a few times and got no response.

The full breach

So I went back and crafted a different proof of concept that leveraged another key fact to prove the vulnerability wasn’t %1$s but in fact passing user input to the query side of prepare:

Given the meta.php file cited before:

if ( $delete_all ) {  $value_clause = '';  if ( '' !== $meta_value && null !== $meta_value && false !== $meta_value ) {    $value_clause = $wpdb->prepare( " AND meta_value = %s", $meta_value );  }  $object_ids = $wpdb->get_col( $wpdb->prepare( "SELECT $type_column FROM $table WHERE meta_key = %s $value_clause", $meta_key ) );}

Given input of:

$meta_value = ' %s ';$meta_key = ['dump', ' OR 1=1 /*'];

Will generate the following query:

SELECT type FROM table WHERE meta_key = 'dump' AND meta_value = '' OR 1=1 /*'

And there we have it. We have successfully injected core. (It’s worth noting that both $meta_value and $meta_key come directly from user input).

This works, because the value clause will be generated as:

 AND meta_value = ' %s '

Remember that unquoted %s are replaced by a quoted '%s' via prepare. So the second call to ->prepare() turns the clause into AND meta_value = ' '%s' ' and enables the injection.

I stress that this vulnerability cannot be fixed in WPDB::prepare() but instead was a problem in meta.php. Yes, you could mitigate it by preventing “double prepare calls”, but you wouldn’t fix the original issue (which didn’t use prepare, but _real_escape()).

The Simple Fix

The simple fix is to not pass user input to the $query parameter to WPDB::prepare() in meta.php.

Passing user input to $query is always wrong. Full stop.

The Mitigation Fix

The next step would be to somehow “quote” placeholders in prepared queries and then restore the placeholders before executing the query. This patch also exists.

Basically, the fix would modify WPDB::prepare() (and all of the escape functions such as _real_escape()) to do a replacement of any % placeholder with a random string. Something like:

$query = str_replace('%', "{$this->placeholder_escape}", $query );

Then, in WPDB::_do_query() remove the placeholder to restore the original user input.

This “works” by preventing this specific vector.

I still stand by that passing user input to the query side of prepare is potentially dangerous and fundamentally unsafe, even if “escaped”. And double-preparing a string (by passing the output of one “prepare” into another) is extremely dangerous and will always be unsafe, even if you may mitigate known vulnerabilities.

Note: It’s worth noting that this looks similar to my original suggestion of Add a check in prepare to check for and reject double-prepares (using a comment to indicate prior prepares) The important difference is that I suggest bailing out if you detect a double-prepare and showing the developer an error, rather than “trying to make it work”.

This is precisely how 4.8.3 “fixes” the vulnerability. It still doesn’t address the root issue though (passing user input to the query side of prepare)…

The Correct Fix

The correct fix is to ditch this whole prepare mechanism (which returns a string SQL query). Do what basically everyone else does and return a statement/query object or execute the query directly. That way you can’t double-prepare a string.

It’s worth saying that this would be a major breaking change for WP. One that many other platforms have done successfully (PHPBB did this exact thing, and went from having massive SQL Injection vulnerabilities to almost none).

It doesn’t need to be (and in practice shouldn’t) overnight - they can do it in parallel with the existing API, deprecating the old one and removing in time - but it does need to happen.

The current system is insecure-by-design. That doesn’t mean it’s always hackable, but it means you have to actively work to make it not attackable. It’s better to switch to a design that’s secure-by-default and make the insecure the exceptional case.

The best path forward would be to switch to PDO/MySQLi and use real prepared statements and not emulate them in PHP land. That’s the best path forward.

But if that’s not acceptable, then at least move to a statement object style system where prepare returns an object which is then executed. And for the love of god get rid of escape_by_ref/esc_sql as well as the still-existent _weak_escape (which calls addslashes() and has been “deprecated” for 4 years and still somehow exists)…

These changes won’t prevent misuse, but it will make it far harder. It will make the default usage secure making developers go out of their way to make it insecure (where today is precisely the opposite).

Also

It’s also worth noting that with this mitigation technique, support for positional placeholders was added back in (though a subset of what was possible, it should be the vast majority of use-cases).

Read the whole story
Pudge601
20 days ago
reply
Share this story
Delete

How We Built r/Place

1 Share
submitted by /u/bsimpson to r/programming
[link] [comments]
Read the whole story
Pudge601
221 days ago
reply
Share this story
Delete

Chat Systems

13 Comments and 31 Shares
I'm one of the few Instagram users who connects solely through the Unix 'talk' gateway.
Read the whole story
Pudge601
252 days ago
reply
Share this story
Delete
11 public comments
fmeggers
226 days ago
reply
Chaos communications

iPhone: 47.398945,8.541090
tante
252 days ago
reply
The Internet will connect us all ... just not really
Oldenburg/Germany
bitofabother
252 days ago
reply
Too real.
Portland, OR
francisga
252 days ago
reply
I love that AIM users are not reachable any other way.
Lafayette, LA, USA
adamgurri
252 days ago
reply
THIS
New York, NY
mrobold
252 days ago
reply
The struggle is real.
Orange County, California
JayM
253 days ago
reply
No bubble for: Email-SMS-Jabber-iMessage-Skype-IRC-TwitterDM-LinkedIn-PrivateForums-NewsBlurComments
Atlanta, GA
mindspillage
253 days ago
reply
Also, NewsBlur comments.
Mountain View, California
jth
253 days ago
reply
POST /inbox/new&msg=Hi!%20How%20have%20you%20been%3F%20It%27s%20been%20years%20since%20I%27ve%20seen%20you%20around.
Saint Paul, MN, USA
drchuck
253 days ago
reply
Nobody uses Wikipedia talk pages?
Long Island, NY
alt_text_bot
253 days ago
reply
I'm one of the few Instagram users who connects solely through the Unix 'talk' gateway.
HarlandCorbin
253 days ago
I found me on the diagram, i seem to be in a lonely group.

Code Quality 2

6 Comments and 19 Shares
It's like you tried to define a formal grammar based on fragments of a raw database dump from the QuickBooks file of a company that's about to collapse in an accounting scandal.
Read the whole story
Pudge601
522 days ago
reply
Thankfully I haven't at to review anything this bad... which means it must be me writing code like this?
letssurf
521 days ago
LOL, no it means you work in an awesome team! :D
Brstrk
521 days ago
Try Minecraft modding. It's a multimillion-dollar burning bus.
srsly
521 days ago
this ^^ Notch started Minecraft as a personal project / toy, and it really started ballooning out of control when he incorporated other people's mods, also made as personal projects.
Brstrk
519 days ago
I know, right? I feel sorry for him. The game was just something to do with his spare time. Crazy stuff.
srsly
518 days ago
I literally cannot feel sorry for a person who has two billion dollars and has outbid Beyonce on a house.
Brstrk
518 days ago
Fair enough. Dude's not doing poorly at all. It's just that we rib on that fine spaghetti of a code a lot.
Share this story
Delete
5 public comments
reconbot
522 days ago
reply
Adding "runs like a burning bus" to my repertoire
New York City
Covarr
522 days ago
reply
It looks like you were trying to write a letter and you let Clippy help.
Moses Lake, WA
llucax
522 days ago
reply
She is talking about my code... (except for the javascript, I would never touch javascript)
Berlin
drchuck
522 days ago
reply
She didn't mention "lyric sheet from a Captain Beefheart album," so that's a plus.
Long Island, NY
zippy72
521 days ago
My smile is stuck, I cannot go back to your Frownland.
alt_text_bot
522 days ago
reply
It's like you tried to define a formal grammar based on fragments of a raw database dump from the QuickBooks file of a company that's about to collapse in an accounting scandal.

When Cats Travel Too Fast!

1 Share
When Cats Travel Too Fast! submitted by /u/exg
[link] [comments]
Read the whole story
Pudge601
524 days ago
reply
Share this story
Delete

Musical Marble Machine. MIND BLOWN! Man builds real life Animusic music box. (Wintergatan, Martin Molin).

1 Share
Musical Marble Machine. MIND BLOWN! Man builds real life Animusic music box. (Wintergatan, Martin Molin). submitted by /u/quitthecircuit to /r/videos
[link] [comments]
Read the whole story
Pudge601
629 days ago
reply
Share this story
Delete
Next Page of Stories