CodeSOD: A Learning Opportunity

At various points in my career, I spent my days delivering technical training. If your company wanted to adopt a new technology, or upskill your developers, you'd hire me and I'd do crash course sessions. I generally didn't design course material: the books, slide decks, and labs. I just delivered the course. Most of the course material I worked with was pretty good, but there were some which were clearly written by people who were just trying to get a course out the door fast while a new technology was buzzwordy enough that nobody knew what they were buying.

I hated those courses.

All this means that we're going to shift gears a little bit into something we don't usually look at: the kinds of code that may be used as examples in training material.

The first one comes from Jan, who was just trawling through JavaScript guides, and stumbled across one article that really stressed the importance of comments. For example, this incredibly complex method needs this comment attached to it:

/**
 Saves the user to the database.
 It receives a "User" object, it generates a random alphanumeric ID and then it tries to save it into the users table.
 If sucessfull, it'll return the new generated user ID.
 If it fails, it'll return "false" instead.
*/
function saveUser(usr) {
  usr.id = Math.random().toString(36).substring(2);
  let result = usr.save();
  if(!result) {
    return false;
  } 
  return usr.id;
}

There's no big WTF here, but this code is meant to be educational, which is why I'm picking on it.

There's the initial premise of this code being so complicated in needs a thorough comment that explains line-by-line what the code does. That's clearly not true, though this code definitely needs documentation, because one needs to understand what it returns when.

But that's also the problem. Error states generally shouldn't propagate through return values if your language has another option. A version of this method that simply returns the ID or throws an exception on failure would both be easier to understand and easier to document.

Of course, "a bad, lazy code sample," is par for the course in a lot of educational materials, so this is just our appetizer.

Steve Wahl signed up for some security training. He was quite happy to see C offered as an option, since he's used to seeing very web-focused security training and since he does Linux kernel and driver development, that's not exactly useful.

What wasn't clear was that they expected developers to be ready to delve into Windows APIs as well as Linux APIs. Many of the exercises took the form of "spot the vulnerability", so take a look at this block and see if you can guess the right answer:

// Do NOT forget to call '_freea(stackBuf);'
void* StackAllocate(const size_t size)
{
    if (size < 1U || size > 1024U * 1024U)
        return NULL;

    void* buf = NULL;

#ifdef _WIN32
    buf = _malloca(size);
#else
    buf = alloca(size);
#endif
    return buf;
}

What the training wanted you to spot was that on Windows, _malloca can raise a stack overflow error, which needs to be handled with Windows' "structured exception handling" dialect. This code doesn't handle that error correctly, and thus, that's the danger.

Which, sure, not handling exceptions is bad. But that's hardly the thing that we should be paying attention to.

First off, alloca always allocates memory on the current stack frame. Which means when StackAllocate returns, you're returning a pointer to memory you don't own. Don't do this. That is a bad thing to do.

But the Windows case, with _malloca gets weirder, because _malloca doesn't guarantee that it allocates memory on the stack. It might acquire heap memory. Which is probably why the comment warns you need to call _free- alloca is "automatically" cleaned up (along with the stack frame), while _malloca can't be automatically cleaned up.

Regardless, both of these methods might actually allocate stack memory, in which case you can't safely pass back a pointer from either of them. That's the actual bug in the code, not the missing exception handling.

If this code existed in production, it'd be a complete WTF. That it's used to illustrate mistakes made that might create security problems, it's missing the forest for the trees. This is a clear case that the person designing the training had a very specific understanding of very specific things, but didn't assemble them into a bigger picture to actually communicate the lesson to their students- students who, by their very nature, might not understand what's going on well enough to recognize how misleading the educational content is.

I promise not to pick on training materials too much, but especially the secure coding example really bugged me.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!

This post originally appeared on The Daily WTF.