Rule 6 – shell expansion injection attack edition

Say it with me everyone, cleverness is the mother of regret. Rachelbythebay with yet another tale of turn-by-turn navigation down the road to ruin in Fix the unit test and open a giant hole everywhere

Like most dumpster fires, it starts with a mundane business problem:

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.

…that someone solves in a clever way

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 was kicking off a subprocess to run the mkdir program

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

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”…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.

And now for the best part, why it happened at all

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” is 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.

To paraphrase Homer Simpson: To unit tests! The cause of, and solution to, all of dev’s problems

Simple rules for keeping dev teams out of trouble

I’ve been leading small software development teams on big projects since 2004. Painful experiences taught me some simples rules to make it less painful. Here they are, presented in order of discovery:

      1. If it can be null, it will be null
      2. The clipboard is a cruel mistress
      3. Size really does matter
      4. Inside every small problem is a larger one, struggling to get free
      5. Every bug you fix creates 2 more
      6. The law of the motorcycle shop is non-negotiable (whoever touched it last is who broke it)
      7. Cleverness is the mother of regret
      8. Just in time requirements are neither
      9. Don’t solve problems you don’t have yet, Nostradamus
      10. Later == never, temporary == forever, shipped > not shipped
      11. Only change 1 thing at a time
      12. Always redo, never fix
      13. Finish the most important feature first
      14. Tomorrow you hates the code you write today, so don’t plan too far ahead
      15. Character data is nvarchar(max) until you can prove otherwise (see rule 2)
      16. Schrodinger’s Spec – you can know what the client wants, or what they actually need, just not at the same time
      17. Solve the toughest problem first
      18. Legacy == proven. Try out the newest thing on your own time and dime.
      19. Naming stuff is hard
      20. You’re not going to reuse that
      21. Process: never fix, always redo. Code: never rewrite, always fix
      22. Fix problems upstream
      23. Never use a black box when a text file will do
      24. Normalize until it hurts, denormalize until it works
      25. The original sin of code is writing it
      26. An unbound task is the Devil’s workshop
      27. Developer time is vastly more expensive than CPU time