The Grumpy Troll

Ramblings of a grumpy troll.

Shell anti-pattern security hole

I consider myself proficient in shell programming (POSIX sh, with variants). Today, I learnt of a surprising behaviour, which I then realised meant that some error-handling code wasn't firing when it should, which led to spotting why this is a rather common problem and, in certain circumstances, a security hole resulting from an anti-pattern.

The anti-pattern:

die() { echo >&2 "$0: $*"; exit 1; }
foo() {
local tmpdir=$(mktemp -d -t foo.XXXXXXXX) || die "mktemp failed"
# ...

The problem:

foo=$(false) || echo bleh
local foo=$(false) || echo bleh

Those two statements are not equivalent. The first is an assignment, which assigns to foo a string, which is formed by running a command. The command fails and $? is set non-zero, so the short-circuiting OR continues on and "bleh" is echo'd.

The second is a command, "local", which does an assignment. The command-substitution sets $? as before, but then "local" sets $? back to 0 because it has successfully created a variable as local, and assigned it a value, albeit empty, so the error case is not invoked.

In "real" programming languages, "local" would be some kind of scope qualifier and this wouldn't happen. In shell, "local" is a command. A built-in, yes, but a command, which sets $?.

So if you carefully used mktemp(1) to create a temporary file or directory, and carefully checked $? to be sure it succeeded, with error-handling to abort if it failed (ie, when you're under active attack) then your error-handling will never fire and you'll continue on with an empty string.

Why is this sometimes not a security hole? Well, let's look at the common cases:
  1. tmpdir is empty, thus { cd "$tmpdir" } is likely to chdir to your home directory permissions on your home directory are likely tight enough that the problem is solved although if you were trying to lock down permissions for holding sensitive data, then you do still have a problem.
  2. if you made a temporary directory, then your clean-up step of { rm -rf "$tmpdir" } is going to be a no-op and content will hang around. You'll get litter, but not a security issue unless the content is private.

Now, if you used mktemp without -t, so that you weren't using $TMPDIR or /tmp, then you're likely doing "/some/path/$tmpdir/foo" work, and a final clean-up step of { rm -rf "/some/path/$tmpdir" } is going to be a problem.

For this to be a hole at all, someone must be attacking you to make mktemp(1) fail, but since that's the whole reason you put in error-checking in the first case, you must care about that.

So, in some circumstances it will be a security hole but luck and other factors limits the impact.

  1. Shell is harder than you think
  2. Don't declare the variable and assign at the same time.
  3. Use ${tmpdir:?} so that if the variable is unset or empty you'll get a shell error and forced exit.