WordPress's idea of SQL injection security: addslashes

In the spirit of improving the quality of open source software, I’ve decided to launch a whole new category here on wonko.com. I’m calling it Crappy Code. Whenever I come across a really crappy piece of code in an open source application, I’ll write a cantankerous, insult-laden post all about how crappy it is and then, when applicable, I’ll offer up a suggestion for improving said code.

For our first installment in this series, we’ll turn to WordPress. If I had to list all the things I hate about WordPress, I’d probably kill myself instead. But here’s an example of the sorts of things that might be on that list if my suicide attempt were to fail:

  1. They have a stupid motto. “Code is poetry”. Blah blah. I’d be less annoyed by this if their code was actually good, but the fact that it sucks just annoys me even more.
  2. WordPress is one of those applications that uses 100 lines of code to accomplish a task that could have been accomplished in 10 lines if the developers had bothered to put some fucking thought into it.
  3. Nobody who uses WordPress actually gives a shit about how crappy the code is, so it’s insanely popular. Wound: check. Salt: check.

So now that we’ve established that I’m insanely biased, let’s get to the part where I make fun of some code. It was pretty tough finding just one thing to make fun of. I have a feeling WordPress is going to show up quite a lot in future Crappy Code posts. But to get things started I wanted something short, sweet, and simple. Lo and behold, I found it.

WordPress 1.5.2; wp-includes/wp-db.php; line 76:

// ====================================================================
//  Format a string correctly for safe insert under all PHP conditions

function escape($str) {
  return addslashes($str);
}

Oh dear God.

The comment is what really gets me. What genius came to the conclusion that addslashes would make all possible strings safe for use in SQL queries under all possible conditions? The addslashes function just puts a backslash (\) in front of single-quotes (‘), double-quotes (“) and NUL characters. And depending on the value of PHP’s magic_quotes_sybase setting, it might escape single-quotes with another single-quote rather than a backslash. What it doesn’t do is escape newlines, carriage-returns, or the control character \x1a, all of which need to be escaped when sending string values to MySQL.

Instead of addslashes, they should be using mysql_real_escape_string, which actually does escape every unsafe character “for safe insert under all PHP conditions”. Their code should look like this:

function escape($str) {
  return mysql_real_escape_string($str, $this->dbh);
}

WordPress is one of the most widely used PHP applications in the world. They have a history of SQL injection vulnerabilities. You’d think they’d know by now which function to use for escaping strings being sent to MySQL.

Update: A helpful reader has pointed out that the issue was fixed way back in July and the fix will be in the 1.6 release.