Secure shell scripts, the secure way

#What goes wrong

Bash is forgiving in exactly the wrong places. By default a script keeps going after a command fails, treats an unset variable as an empty string, and ignores a failure in the middle of a pipeline.

Put those together and a single typo becomes a disaster. If OUTPUT_SUBDIR is not set, rm -rf $DIR/$OUTPUT_SUBDIR/* becomes rm -rf $DIR//*. If DIR is empty too, it becomes rm -rf //*, the whole filesystem.

Unquoted variables add a second class of bugs. A file name with a space becomes two arguments; a name starting with - becomes an option. Scripts that run as root, from cron or from sudo, turn these bugs into security problems: a user who controls a file name controls part of a root command.

Predictable temporary files are the third. /tmp/cleanup.$$ can be guessed, and another user can create it first as a symlink to a file you care about.

#What the docs say

If set, the return value of a pipeline is the value of the last (rightmost) command to exit with a non-zero status, or zero if all commands in the pipeline exit successfully. This option is disabled by default.

Source: GNU Bash manual, The Set Builtin (pipefail)

The shell does not exit if the command that fails is part of the command list immediately following a while or until reserved word, part of the test in an if statement

Source: GNU Bash manual, The Set Builtin (-e)

If STEAMROOT is empty, this will end up deleting everything in the system's root directory.

Source: ShellCheck wiki, SC2115

The second quote matters: set -e has many exceptions. It is a safety net, not a guarantee. Commands whose failure matters still need an explicit check (|| die "...").

#The secure configuration

A template for any script that runs unattended or as root:

#!/usr/bin/env bash
# cleanup.sh: remove old build output (the safe version)
set -Eeuo pipefail          # stop on errors, unset variables, and failed pipeline stages
IFS=$'\n\t'                 # split words only on newlines and tabs
umask 077                   # files this script creates are private

die() { printf 'error: %s\n' "$*" >&2; exit 1; }

[[ $# -eq 1 ]] || die "usage: $0 BUILD_DIR"
build_dir=$(realpath -e -- "$1") || die "no such directory: $1"
# Refuse anything outside the one tree this script is meant to touch.
[[ $build_dir == /srv/builds/* ]] || die "refusing to clean $build_dir"
output_subdir=${OUTPUT_SUBDIR:?OUTPUT_SUBDIR must be set}

tmp=$(mktemp -d)            # unpredictable name, mode 0700
trap 'rm -rf -- "$tmp"' EXIT

# ${var:?} makes rm fail instead of expanding to "/" when a variable is empty.
rm -rf -- "${build_dir:?}/${output_subdir:?}/"*

curl -fsS --proto '=https' -o "$tmp/latest.tar.gz" https://example.com/latest.tar.gz
tar -xzf "$tmp/latest.tar.gz" -C "$tmp"

# Globs, not ls: file names with spaces or newlines stay intact.
for f in "$tmp"/*.conf; do
  [[ -e $f ]] || continue
  install -m 0640 -- "$f" /etc/app/
done

Run ShellCheck on every script in CI, and fail the build on warnings:

shellcheck --severity=warning scripts/*.sh

#Prove it

An unset variable in an rm path (printed with echo, not run). Without protection it collapses to /srv/builds//*. set -u and ${var:?} stop it:

bash -c 'dir=/srv/builds; echo rm -rf $dir/$OUTPUT_SUBDIR/*'
bash -c 'set -u; dir=/srv/builds; echo rm -rf $dir/$OUTPUT_SUBDIR/*'
bash -c 'dir=/srv/builds; echo rm -rf "${dir:?}/${OUTPUT_SUBDIR:?}/"*'

rm -rf /srv/builds//*
bash: line 1: OUTPUT_SUBDIR: unbound variable
bash: line 1: OUTPUT_SUBDIR: parameter null or not set

A failed first stage in a pipeline, without and with pipefail:

bash -c 'false | sort >/dev/null; echo exit status: $?'
bash -c 'set -o pipefail; false | sort >/dev/null; echo exit status: $?'

exit status: 0
exit status: 1

A file named my notes.conf, looped over with ls and with a glob:

bash -c 'for f in $(ls /tmp/q/*.conf); do echo "item: $f"; done'
bash -c 'for f in /tmp/q/*.conf; do echo "item: $f"; done'

item: /tmp/q/my
item: notes.conf
item: /tmp/q/my notes.conf

A guessable temporary name, and mktemp -d:

bash -c 'echo /tmp/cleanup.$$'; bash -c 'd=$(mktemp -d); stat -c "%a %n" "$d"'

/tmp/cleanup.146
700 /tmp/tmp.Og2sRLDP1w

ShellCheck on the unsafe version of the script, then on the safe one:

shellcheck -f gcc bad.sh
shellcheck good.sh && echo "shellcheck: no findings"

/t/bad.sh:4:1: warning: Use 'cd ... || exit' or 'cd ... || return' in case cd fails. [SC2164]
/t/bad.sh:4:4: note: Double quote to prevent globbing and word splitting. [SC2086]
/t/bad.sh:5:8: warning: Use "${var:?}" to ensure this never expands to /* . [SC2115]
/t/bad.sh:5:8: note: Double quote to prevent globbing and word splitting. [SC2086]
/t/bad.sh:5:19: note: Double quote to prevent globbing and word splitting. [SC2086]
/t/bad.sh:7:10: error: Iterating over ls output is fragile. Use globs. [SC2045]
/t/bad.sh:7:35: note: Double quote to prevent globbing and word splitting. [SC2086]
shellcheck: no findings

The safe script refuses a missing argument, a path outside its tree, and a missing setting:

bash good.sh
bash good.sh /etc
bash good.sh /srv/builds/app1

error: usage: /t/good.sh BUILD_DIR
error: refusing to clean /etc
/t/good.sh: line 13: OUTPUT_SUBDIR: OUTPUT_SUBDIR must be set

ShellCheck did not flag the predictable /tmp/cleanup.$$ name in the unsafe script. Linters catch a lot, not everything. The test script and both sample scripts are in secure-tests/secure-shell-scripts/.

#Mistakes people make

#Trusting set -e to catch everything

It does not trigger inside if tests, && and || lists, or most command substitutions used as arguments. Check the result of commands that matter explicitly.

#Unquoted variables

Quote every expansion: "$var", "$@", "$(cmd)". The rare cases where you want word splitting deserve an array instead.

#Parsing ls

ls output is for people. Use globs, or find ... -print0 | xargs -0 for large trees.

#Secrets in arguments and logs

curl -u user:password shows the password in ps output to every user on the host, and set -x writes it to the log. Read secrets from a file or an environment variable, and turn tracing off around them.

#eval and unchecked input

eval "$user_input" runs whatever the user typed. Check input against a fixed pattern ([[ $name =~ ^[a-z0-9-]+$ ]]) and never build commands as strings.

#Checklist

• Every script starts with #!/usr/bin/env bash and set -Eeuo pipefail.
• Every variable expansion is quoted.
• Every path given to rm -rf uses ${var:?}.
• Temporary files and directories come from mktemp and are removed by a trap.
• Loops use globs or find -print0, never ls output.
• Arguments are checked against an allowed pattern or path prefix.
• No secrets appear in command arguments or set -x output.
• ShellCheck runs in CI and fails the build on warnings.