Personal Blogs

Two things this post, first a part for almost-normal people, then one just for geeks, er, I mean, Moodle developers.

The almost-normal part: if you're reading this, you're seeing the OU blog activity module. So you might be interested in a change to OU blog in our upcoming release. In September we will launch public comments on OU blog, so that people without an OU login can leave comments on your post. If you turn this on, you (the blog author) will have to approve public comments, so that there isn't any spam. Want to learn more in harder-to-understand detail? Read this exciting functionality document (PDF). It hasn't been tested yet; hopefully the system doesn't completely fall over and get turned off before we release it.

Now the geek bit. Basically I wanted to warn about a Moodle coding antipattern. Here it is:

// $thing is some object we previously got 
// from the mdl_thing table. It has the
// current values of a row from that table.
$thing->numericfield = $newvalue;
update_record('thing', $thing);

Don't do that!

For those who didn't immediately go 'yeah duh, of course not' - here's why. Let's say the $thing has some other field you didn't change. When you call update_record, Moodle doesn't know you didn't change it, so it'll update that as well. Which means if it contains any apostrophes, it will create a database error; and you've just made an opportunity for an SQL injection attack.

Even if $thing currently doesn't have other fields (or they aren't text fields, or you manually addslashes to them), another field might be added to that table later - et voila! A lovely security hole in code you didn't even change.

So, the only correct way to use update_record is this pattern:

$update = (object)array('id' => $thing->id);
$update->numericfield = $newvalue;
update_record('thing', $update);

Well, you could use different syntax (and should probably check the return value), but basically, what I'm getting at is that you should always call update_record with an entirely new object that only contains the ID and the fields you are actually changing (which should have slashes added if required).

As you might have guessed, I'm not writing about this just for fun This precise hole was recently discovered in part of our VLE code (actually a part written by outsourcers, but I'm not saying we wouldn't have done it in house). I waited to make this post about it until our live server has been patched, so this security hole isn't in our system any more...

This hole wasn't discovered by developers or testers here, but by a student who reported something broken (which turned out to be related to an apostrophe in another data item). Luckily I don't think they realised it was a security hole! Anyway, many thanks going out to Jeanette Stephenson');UPDATE mdl_grade SET totalscore=100 WHERE userid=157790021;--!

(Yes that is an old joke. And no the person who found the bug wasn't really called Jeanette Stephenson, and didn't really have that user id, I just made both up. But, if you do recognise yourself - I think rather unlikely since I was cagey about where this problem actually was - thanks. Really.

In reality there are some extra layers which might protect against actual attacks of this nature, but even so, I think Moodle 2 (which doesn't require you to get slashes right) will be a big security improvement. Still good practice not to update_record with all the fields, though!