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:
- 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.
- 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.
- 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.
Comments
Already fixed
While not in the current stable release, it will be in 1.6.
Re: Already fixed
Glad to hear it.
crapcode
But then I realize: "eh - it works."
Its the same sorta feeling I get when I have to use windows for a day. All icky in my shorts.
Re: crapcode
The great thing about Windows is that I can't see the source, so it doesn't bother me as much.
Crappy IE?
Hey Wonko, have you looked at your main page in IE lately? The postings are pushed down the page, showing a lot of unnecessary white space. It doesn't happen in Firefox. Just an FYI.
Re: Crappy IE?
No, I have not looked at my main page in IE lately. I also have not looked at my main page in Mosaic lately.
No title
*snigger*
addslashes()
function getVar($var)
{
if (isset($_POST[$var]))
return $_POST[$var];
elseif (isset($_GET[$var]))
return $_GET[$var];
else
return "";
}
63
set_magic_quotes_runtime(0);
if (get_magic_quotes_gpc() == 0)
{
function magicSlashes($element)
{
if (is_array($element))
return array_map("magicSlashes", $element);
else
return addslashes($element);
}
// Add slashes to all incoming GET/POST data. We don't care about
// cookies.
$_GET = array_map("magicSlashes", $_GET);
$_POST = array_map("magicSlashes", $_POST);
}
Re: addslashes()
You have done exactly the same thing in Poseidon??? As i can see in the code above you have done exactly the same thing. All things you get via getVar() you put directly in the database.
You should see over your own code before you are whining at others code ;)
The getVar function comes from functions.php and line 1166 to 1174
The magic_quotes stuff comes from startup.php and line 47 to 63
I try to post everything again in the hope of stuff won't disappear again (that's why the previous post is incomplete).
Re: addslashes()
The code you're referring to in Poseidon was written two years ago, and yes, it's bad. I've written tons of shitty code, but I've learned from my mistakes.
Should I not be allowed to share what I've learned because I once made the same mistakes that I'm trying to teach others not to make?
Re: addslashes()
But i didn't think that Poseidon is 2years old. So i'm sorry.
// Jonas
IE view looks better now...
It seems you do look at your page in IE once in a while...
No title
Or perhaps you can show me an example of SQL injection using new lines as opposed to single or double quotes? ie. one that works on strings that have been passed through addslashes and not mysql_real_escape_string?
No title
source Anyway, given all this, I think you are just as misinformed as you claim the WordPress developers to be.
Re: No title
Allow me to quote myself:
If that comment hadn't been there, I probably would have moved on. But if you're going to write a comment that definitively declares your code to be bulletproof, your code had better actually be bulletproof or it's going to bite you in the ass eventually.
From an SQL injection standpoint, addslashes works fine for MySQL. But if you happen to be the system administrator and you have to occasionally work with MySQL query logs, you're going to be pretty annoyed to find queries with unescaped newlines mucking up the log and making it hard to read.
It takes no extra effort to use mysql_real_escape_string instead of addslashes. There's no excuse not to.
No title
I can agree with that - that given a choice between the two, it's preferable to use mysql_real_escape_string :)
Broken link
Hey there — looks like the link to "Crappy Code" in the first paragraph of your post is broken.
Re: Broken link
Oops. Thanks. This post was written when I was using the previous generation of my blog engine, which used a different URL scheme.
addslashes() -VS- mysql_real_escape_string()
Except that mysql_real_escape_string() needs a database connection in order to actually add slashes.
See: http://php.net/mysql_real_escape_string
Legend
Wonko you're a legend... this thread is pissfunny.
SQL injection IS STILL possible with addslashes()
Hello, you say that : "From an SQL injection standpoint, addslashes works fine for MySQL. But if you happen to be the system administrator and you have to occasionally work with MySQL query logs, you're going to be pretty annoyed to find queries with unescaped newlines mucking up the log and making it hard to read."
It seems like SQL injection is still possible with addslashes(). I found this webpage about this : http://shiflett.org/blog/2006/jan/addslashes-versus-mysql-real-escape-string There is many problem with multi-byte character that addslashes() don't handle correctly, so many quote can pass it.
This kind of strings can do it : # $_POST['username'] = chr(0xbf) . # chr(0x27) . # ' OR username = username /*';
SQL injection IS STILL possible with addslashes()
Sorry, I don't know why but the newline have been drop :) The strings is :
$_POST['username'] = chr(0xbf) .chr(0x27) .' OR username = username /*';
And this is a the clickable link ;)
Still an issue
Or is it just an issue again? I've only just started looking at wordpress-mu, and this is not encouraging stuff to see. Here's the first line of the current escape function:
return addslashes( $string ); // Disable rest for now, causing problemsWhat kind of 'problems' were being caused by mysql_real_escape_string?
How Dare You!
Wonko, until you are the lead architect of probably the world’s most popular piece of open source code I think you should keep your comments to yourself.
I am an experienced PHP developer working on Windows and coding in Dreamweaver. I am well recognised as a God among folk who can only code HTML and CSS. They find it really hard to modify WordPress because it is so advanced. People who code WP templates are highly regarded for the skill it takes to work around my PHP which bundles out elegantly concatenated HTML. The thing that makes WordPress great more than anything else is the fact that I have files with HTML, PHP, JavaScript, XML and RSS all mixed within each other. You’ve got to be shit hot to pull that off. Some people say its a miracle WordPress even works.
Looking down on your from the WordPress bummers fan wave,
Matt Bunglecode.