Fix the unit test and open a giant hole everywhere

Fix the unit test and open a giant hole everywhere

Several years back, I was doing my thing in a corporate environment, working on a project with some friends. By virtue of being in a very large company, we had quite the assortment of libraries which could be brought in to do stuff. Given that we had a "would be nice" item on our feature list that involved doing something with the filesystem, I went looking for a matching feature, and it turned into a security dumpster fire. Here's what happened.

Our program was going down a road where it might need to create a series of paths. This is where you have "/my/path" and you need a couple of directories nested underneath it, so you end up with "/my/path/project/staging" and "/my/path/project/prod" and things like that.

When you're in that situation, you can't just mkdir() the final path. It won't work. You have to start from the top and work your way down. The ones which already exist will probably fail with -EEXIST, so sometimes it makes more sense to look at what's there, work your way down, and then start the mkdir calls when you find that point.

So, you can either do this:

1. mkdir("/my") : -EEXIST
2. mkdir("/my/path") : -EEXIST
3. mkdir("/my/path/project") : this one works fine
4. mkdir("/my/path/project/staging") : this works fine too
5. mkdir("/my/path/project/prod") : and this one is also okay

Or, you can do something more like this:

1. lstat("/my") : works, and is a directory, carry on
2. lstat("/my/path") : works, is a directory, carry on
3. lstat("/my/path/project'") : -ENOENT, okay, switch to calling mkdir!
4. mkdir("/my/path/project") : this succeeds

... and so on down the line. You get the idea.

As you can see, this is the kind of thing that takes a little work to get right, and once someone does that, it would be nice to share it with the rest of the company. For that reason, I went looking into the "common" part of our code base to see what we had in terms of "directory creation utilities". Maybe we'd have something which did this already!

What I found was... disturbing. I found a function that claimed to create directories, but it wasn't just a simple passthrough to the usual C library mkdir() function. It also didn't do anything like the above. It basically did this (in pseudocode):

void create_a_directory(path) {
  system("mkdir -p " + path);
}

Yep, that's right, it was kicking off a subprocess to run the mkdir program, that is, the thing that's probably sitting in your /bin directory right now if you are using something vaguely Unixy right now.

If you're on a Mac running Monterey, it's this thing:

-rwxr-xr-x  1 root  wheel  134144 Dec  7 15:39 /bin/mkdir

Why would someone change this function to do this? Well, the "-p" switch is the magic part. The actual 'mkdir' command-line tool supports that to "create intermediate directories as required". It handles all of that stuff I mentioned earlier, and you can see why someone might want to "leverage" that.

Unfortunately, in doing this, it created an enormous security hole, too. The C system() call runs subcommands *through a shell*, and whatever you pass to the shell is subject to all kinds of fun shell expansion rules.

These shell expansion rules include a semicolon to split commands. This means if you can get a semicolon and another command in there, it will happily run it right after the "mkdir -p /whatever".

Worse, since this was changed in a common function that all kinds of programs at the company used, it meant that anything that created a directory in that language (with that library) was now vulnerable to this kind of injection.

Imagine it. At some point, you write something that creates temporary paths based on a username or something like this. You call your company's common directory thing, and it just calls mkdir() which takes a path and nothing more. If someone passes in some username like "foo;touch you.are.owned", that just becomes part of the directory name. Really. It's just a goofy name on the filesystem, and nothing bad happens.

Then, one day years later, someone changes that common tool to system() out to something, and now it creates a directory called "foo" and then creates an empty file called "you.are.owned". It's trivial to change it to do something far nastier at that point, like opening a shell to somewhere else, exfiltrating data, or whatever. Your program is now wide open to this kind of attack and you've changed nothing.

I guess it should not surprise you that we had to undo this, and then had to get everyone who used this function to recompile and ship a new version of their stuff. That was "fun".

So finally, you're probably wondering why this happened, and why someone would change a function that called a C library function into something that ran a $(*^&$( subprocess. Ah, well, that's easy. Someone had a unit test that called that directory creation function with a complicated path that had several elements. Sometimes, not all of the intermediate directories existed, and then it would fail. They knew that "mkdir -p" would what they'd do by hand, but they needed the program to do it for them. They changed the common library, checked it in, reran their unit test, and now it started passing, so they were done.

That's how it happens: a tiny little change flings the door wide open. Someone solves their own local problem and misses the bigger picture.

May your holidays be free from this kind of fixing.

from Hacker News https://ift.tt/3FuaxvQ