Yesterday I went over some code in a project I’m currently working on with a colleague of mine.

It’s an administrative tool, where we allow the user to save his own icon for an item to have better visual hints in the items list. This icon is stored directly in the database, which greatly reduces problems with inconsistencies compared to storing it on disk directly.

I should also mention that we go to great lengths on rights checking to ensure that users can only reach data, they’re allowed to.

We did a major refactoring to the database schema, where one part of it was, to move that icon out from the master items table into another table. (In other words: bringing the schema into the 3rd normal form – if I get that right…)

To still allow editing of the now normalised icon, my colleague introduced a hidden form field on the edit page of the web app, which stores the ID of the icon.
The edit form has also a check box „icon_delete“, so the user can remove the icon again.

In the controller of the item edit page (yes, we use an MVC Framework), my colleague added the following (in pseudo code):


if ($param{icon_delete} and $param{icon_id}) {
$icon_table->find( $param{icon_id} )->delete();
}


(yes, we also use an OR mapper)

Now, what’s wrong with that?

Let me say one thing first: This is not about blaming my colleague. In fact, I removed the hidden field right away, because I had an uneasy feeling about it, but it was only 2 hours later working on the controller code, when it finally dawned on me, why exactly I did the right thing.

When building web applications, we always have to remember this: Don’t trust the user!

But the problem here is exactly that: this code trusts the user!

There’s no check, if $param{icon_id} really belongs to the user, therefore any user with a valid session and the right to edit one game can delete any icon he wishes. And (just for the records) since debug tools like FireBug came along, POSTing arbitrary values became easier than ever.

The Solution

Unfortunately <input type="hidden" /> fields make it all too easy to forget about them – one can’t see it in the rendered page, so one does not think about their security implications.

And all too often, security holes are introduced using this technique while refactoring.

The best thing to avoid these kind of problems therefore is, to ditch hidden form fields once and for all!

Don’t use them any more, avoid them where you can – in modern web app programming environments there is absolutely no necessity and no excuse for using them.

Use the session store instead or re-calculate dependencies again on every request.

Because it is all too easy to forget, that between requests, hidden form data can (and will!) be forged!