LinLog

You know that weird jump in auto-incremented IDs we saw in our database at work the other day? Well, we discovered the cause: apparently we were "hacked".

I use the work "hacked" in the loosest sense of the word. The culprit contacted us the next day, making a number of demands and issuing veiled threats. I may get into the details in another post (it's a long story), but the point is that he appeared to have, at best, a script kiddie level understanding of security. He pointed out a number of flaws in the site, but the ID incrementing was the only one that could actually have caused us any real trouble - and he didn't even mention it! We assume that he actually did that by accident and didn't realize the potential implications if we ran out of ID numbers.

The flaw allowed the ID incrementing was actually quite simple. No SQL injection or cross-site scripting. It was a simple case of passing too much data into a function.

Here's how it works. Our site, which runs on PHP and MySQL, is built on a custom MVC framework. It uses a custom Active Record-style ORM, which is where the flaw lies. The ORM performs database updates and inserts by validating the object's fields against the database schema. Basically, it reads the schema from the database, compares that with the data to be updated, and constructs the SQL accordingly. So when you execute the save() method on an object, it will save the values for the fields that appear in the table schema, but ignore any other fields in the object. The ORM also has static update() and insert() methods that take an associative array, mapping the indexes to field names and performing this same validation. So if you have an array of data, only some of which maps to actual columns in the underlying table, you can just pass the whole thing and not have to go through and separate out the fields you need to save.

That last point is where we got in trouble. We have a method that adds items to our media table. It takes an array of data, does some sanitizing and validation, calls the insert() method to add it to the media table, and adds appropriate records to other tables. The problem was that, in the place where this was called most frequently, we were passing in $_POST as the data array. And while this method did sanitize the fields that we wanted to add to the database, it didn't check for extra fields that just happen to be valid fields in the media table. So, to make a long story short, if you were to put an "id" field in the POST and assign it an integer value, our ORM would happily add that field and value to the INSERT statement it sent to the database.

Of course, this was easily fixed. In fact, it wasn't even hard to find. I did a fair amount of work on our ORM at the beginning of the year, so once I made the connection between the "hacker" and the ID number jump, the source of the bug was immediately obvious. It's just one of those things that nobody ever thought to check until it became a problem.

So the moral of the story is: security is all about attention to detail. Following the "rules" is all well and good, but it's not enough. In our case, we were sanitizing data to protect against XSS attacks and using PDO prepared statements to protect against SQL injection attacks, but it wasn't enough. By forgetting to check for unexpected additional input, we left ourselves open to a completely different type of attack. Of course, it's a significantly less serious class of attack - maxing out our auto-incrementing IDs is recoverable, if annoying - but it's still an issue.

With any luck some good will come out of this. I think we've all learned to be a little more mindful of such issues. And perhaps this will act as a cue to management that maybe - just maybe - it would be better to do some actual testing and review of new code before it's released, rather than just pushing things into production and hoping they work.