Otto on WordPress

We get a lot of submissions to the WordPress.org plugin repository, and so there is often a lot of dangerous code submitted. Usually this isn’t malicious, it’s just by people who honestly don’t know that their code has problems. Understanding those problems is the first step to fixing them.

So here’s one common vulnerability we see in code submissions a lot: SQL Injection

To understand SQL Injection, let’s quote Wikipedia for a moment:

SQL injection is a code injection technique, used to attack data driven applications, in which malicious SQL statements are inserted into an entry field for execution

Here’s a piece of code made for WordPress, which is querying the database for a post:

// bad code, do not use
$results = $wpdb->get_results( "SELECT * FROM $wpdb->posts WHERE ID = $id" );

If you don’t see the problem with this code right away, then you should continue reading this post.

(Yes, this article shows the basics of the prepare() function. If you already know about the prepare() function, you might be shocked at the number of people who do not.)

The problem with SQL Injection vulnerabilities is that sometimes they can be hard to spot. The issue with the above code is actually context-dependent. The question you must answer is “What are the possible contents of the $id variable?”.

If $id = 123, then all is well.

But, if it is at all possible for $id = “-1; SELECT * from wp_users;” then you might have a real problem.

Sometimes we see code like this in submitted plugins:

// bad code, do not use
$results = $wpdb->get_results( "SELECT * FROM $wpdb->posts WHERE ID = ". $_GET['id'] );

Now, we have no idea what “id” contains, and in fact, we’re leaving that entirely up to the visitor of the site. Or the hacker of the site, in this case.

There should be no case where user-input can make it into an SQL statement without being first checked for sanity.

Sometimes, that check is easy. In this case, the ID should always be a number. So we can secure the query like so:

// kinda bad code, still, do not use
$id = (int) $_GET['id'];
$results = $wpdb->get_results( "SELECT * FROM $wpdb->posts WHERE ID = $id" );

This is relatively safe, but not the recommended solution. It’s the naive approach, because we’re thinking that hey, we can just check the value ourselves and handle it accordingly. For integers, sure, but for more complex cases we can’t. Sometimes we can’t even do it for integers, so it’s best to avoid this sort of thinking entirely.

Don’t try to sanitize your inputs to SQL functions yourself. Let the sanitization functions do it for you. WordPress includes a function called prepare() to handle this safely.

The right way:

// good code
$results = $wpdb->get_results( $wpdb->prepare( "SELECT * FROM $wpdb->posts WHERE ID = %d", $id ) );

The prepare() function goes through a number of various checks and such, but eventually it ends up replacing the %d with the integer value of $id. Sure you could have done that yourself, but with prepare, you don’t have to think about how it’s doing it.

Because what if $id wasn’t an integer? If it’s a string, then prepare eventually ends up calling a function named mysql_real_escape_string(). This is a core PHP function that does the necessary escaping for you. It needs to always be called for data inputs to SQL, so the upshot is that you must always use prepare.

That bit is important, so read it again: Whenever you make a direct SQL call, any variable inputs to that query must go through a prepare() cycle. Not sometimes, always ¹.

Here’s another example using a string:

// good code
$results = $wpdb->get_results( $wpdb->prepare( "SELECT * FROM $wpdb->postmeta WHERE meta_key = %s", $metakey ) );

I’m selecting all the rows from the postmeta table with a specific meta key. Note that meta_key is a string, so I used the %s instead of the %d (string vs decimal). Because of this, prepare will take care of the proper quoting of the string for us, we don’t need (or want) to add quotes around it ourselves.

Given this, no matter what $metakey is, it should be safe and properly escaped. SQL Injection should not be possible, barring a bug deep in the mysql library itself or some sort of configuration error. It’s as safe as we can reasonably make it, and certainly safer than any code we try to write ourselves to handle sanitizing it for SQL.

You can use more than one argument if needed:

// good code
$results = $wpdb->get_results( $wpdb->prepare( "SELECT * FROM $wpdb->postmeta WHERE meta_key = %s and meta_value = %s", $metakey, $metavalue ) );

So that’s SQL Injection and how to avoid it. Just always use prepare().

See how simple that is?

So go forth, examine your SQL, and please make sure you prepare’d it properly. Let’s reduce the amount of bad plugin code out there. And if you find plugin authors not using prepare(), email them about it. But be nice, they probably just didn’t know.

1. Rules are there so that we think before we break them for special cases…