# Security review checklist for generated shell scripts > Source: https://learn-bash.net/review/security/ > Part of Learn Bash, free to read. Shell's security model is that there isn't one. A script runs with everything its invoker has, every expansion is textual substitution before execution, and there is nothing between a mistake and the filesystem. Generated shell is written in the style of the shell in its training data — tutorials and READMEs, none of which was expected to run unattended, all of which omits guards for brevity. That is a bad starting point for anything running in CI or as a cron job. ## First: `shellcheck`, then the header ```bash shellcheck -S warning scripts/*.sh ``` `shellcheck` catches most of the injection-adjacent bugs (quoting, word splitting, unsafe globbing) with very few false positives. Everything below is what it cannot see. ```bash #!/usr/bin/env bash set -Eeuo pipefail IFS=$'\n\t' trap 'echo "failed at line $LINENO: $BASH_COMMAND" >&2' ERR ``` `set -u` is a security control, not a style choice — it is the difference between `rm -rf "$DIR/"` deleting a build directory and deleting from the filesystem root. ## Injection ### 1. Unquoted expansion is code injection ```bash grep $pattern file.txt # pattern="-r /" now recurses from root rm $file # file="a b" removes two files tar -xf $archive # archive="--checkpoint-action=exec=sh evil.sh" ``` Word splitting means an attacker who controls a variable controls your argument list. The `tar` example is real: certain options execute commands, and passing an unquoted variable lets someone inject them. **Correct:** quote every expansion and use `--` before anything user-controlled. ```bash grep -- "$pattern" file.txt rm -- "$file" ``` `--` marks the end of options, so a value starting with `-` is treated as data. Three characters, and generated scripts almost never include them. ### 2. `eval` on anything interpolated ```bash eval "config_$key=$value" # key or value contains ; rm -rf ~ ``` There is no safe way to do this. Use an associative array: ```bash declare -A config config["$key"]="$value" ``` ### 3. Command substitution in a string that becomes a command ```bash cmd="ssh $host 'tail -n $lines /var/log/app.log'" $cmd # re-splits, re-globs, and executes ``` Build an array instead, which never re-splits: ```bash ssh_args=(ssh -- "$host" "tail -n ${lines} /var/log/app.log") "${ssh_args[@]}" ``` ### 4. Data interpolated into another interpreter ```bash psql -c "SELECT * FROM users WHERE email = '$email'" # SQL injection, via shell curl -d "{\"name\": \"$name\"}" "$url" # broken JSON, or worse ``` Shell scripts are a common and overlooked route into SQL and JSON injection. Use the tool's own parameterisation, and build JSON with `jq`: ```bash jq -n --arg name "$name" '{name: $name}' | curl -d @- "$url" ``` `jq -n --arg` escapes correctly. String-concatenating JSON in shell is always wrong and generated scripts do it constantly. ## The environment is attacker-controlled ### 5. PATH hijacking ```bash #!/bin/bash curl -sS "$url" | tar -xz # which curl? which tar? ``` If your script runs setuid, from cron with an odd environment, or in CI with a writable directory early in `PATH`, "which binary" is not a question you control. ```bash export PATH=/usr/local/bin:/usr/bin:/bin readonly PATH ``` Set it explicitly at the top of any privileged script. For the highest-value calls, use absolute paths. ### 6. IFS ```bash IFS=$'\n\t' # in the standard header for a reason ``` An inherited `IFS` changes how every unquoted expansion splits. Setting it explicitly removes a class of attack that depends on your caller's environment. ### 7. Inherited environment into a child `sudo` without `-i`, or `exec` passing the current environment, forwards `LD_PRELOAD`, `PYTHONPATH`, `NODE_OPTIONS` and everything else. For anything privileged, use `env -i` and pass only what you need: ```bash env -i PATH=/usr/bin HOME="$HOME" /usr/bin/some-tool ``` ## Files ### 8. Predictable temporary files ```bash tmp="/tmp/build-$$" # PID is predictable; symlink attack echo "$data" > "$tmp" ``` ```bash tmp="$(mktemp -d)" || exit 1 trap 'rm -rf -- "$tmp"' EXIT ``` `mktemp` creates atomically with safe permissions. The `trap` is what stops you leaving secrets in `/tmp` when the script fails halfway. ### 9. umask ```bash umask 077 # before writing anything sensitive echo "$token" > "$tmp/creds" ``` Without it you inherit the caller's umask, which may well be `022` — world-readable credentials. ### 10. Following symlinks into somewhere else ```bash rm -rf -- "$dir" # $dir is a symlink to somewhere important ``` Check before destructive operations: ```bash [[ -L "$dir" ]] && { echo "refusing to operate on a symlink" >&2; exit 1; } ``` ### 11. `cd` without a check ```bash cd "$build_dir" # if this fails, the next line runs where you started rm -rf ./* ``` This is the most common way real damage happens, and it is not exotic — it is a typo plus a missing `||`. ```bash cd -- "$build_dir" || exit 1 [[ -f ./build.marker ]] || { echo "not the build directory" >&2; exit 1; } rm -rf -- ./* ``` A marker-file check before anything destructive is cheap and it is the control that would have prevented most published incidents of this kind. ## Secrets ### 12. Secrets in argv ```bash ./deploy.sh --token "$SECRET" # visible in `ps` to every user on the box curl -H "Authorization: Bearer $TOKEN" # also visible ``` `/proc/*/cmdline` is world-readable on Linux. Anything on a command line is visible to every user on that machine, and it lands in shell history. ```bash curl -H @<(printf 'Authorization: Bearer %s\n' "$TOKEN") "$url" # via a fd curl --config <(printf 'header = "Authorization: Bearer %s"\n' "$TOKEN") "$url" ``` Or read from a file with `0600` permissions, or an environment variable — which is visible in `/proc//environ` only to the owner, so meaningfully better than argv. ### 13. `set -x` leaking secrets ```bash set -x # every expansion is printed, including secrets ``` Enormously useful for debugging and a disclosure risk in CI logs, which are often more widely readable than you think. Turn it off around anything sensitive: ```bash set +x authenticate "$TOKEN" set -x ``` ### 14. Secrets in the trap An `ERR` trap that prints `$BASH_COMMAND` will print the command including its expanded arguments. Worth knowing before you put a token on a command line. ## Downloads and execution ### 15. `curl | sh` ```bash curl -sSL https://example.com/install.sh | sh ``` You are executing code you have not seen, over a channel that could be intercepted or the endpoint compromised, and a partial download can execute a truncated script that does something different from the whole one. ```bash curl -fsSL -o /tmp/install.sh https://example.com/install.sh sha256sum -c install.sha256 &2; exit 1; } ``` Refusing root when you do not need it is a one-line control that limits the blast radius of every other bug in the script. For scripts that genuinely need privilege, do the privileged part in a small separate script and call it with `sudo`, rather than running the whole thing as root. ## The review ```bash shellcheck -S warning scripts/*.sh grep -rnE '\beval\b|`|\$\(' scripts/ # eval and command substitution grep -rnE 'rm -rf|mv |>\s*/' scripts/ # destructive, check the paths grep -rnE 'curl[^|]*\|\s*(ba)?sh|wget[^|]*\|' scripts/ # pipe to shell grep -rnE 'tmpnam|/tmp/\$\$|/tmp/[a-z]+\$' scripts/ # predictable temp files grep -rnE '\$[A-Za-z_]+[^"]' scripts/ | grep -v '\[\[' # unquoted expansions grep -rniE 'token|secret|password|api_key' scripts/ # secrets on command lines ``` :::verdict The four that matter most 1. **`set -Eeuo pipefail` and quote everything.** Removes most of the injection surface. 2. **`--` before user-controlled arguments.** Three characters, whole class of bug. 3. **Marker-file check before anything destructive.** The control that prevents the worst outcome. 4. **Never pipe a download into a shell.** Block it in a hook so it cannot happen by accident. ::: ## Common questions ### Is shell inherently insecure? It is inherently *unforgiving*: expansion happens before execution, so a variable's contents can become syntax. That is manageable with quoting discipline and `shellcheck`, but it means shell deserves more review attention per line than a language where data and code are separate. ### Should scripts run as root? Only the part that needs to. A whole script running as root means every bug in it — including the ones in the parts that had nothing to do with privilege — has maximum blast radius. Split the privileged operation into its own small, reviewable script. ### How do I pass a secret to a command safely? Environment variable or a file descriptor, never argv. `/proc/*/cmdline` is world-readable, so anything on a command line is visible to every user on the machine and often ends up in logs and shell history as well. ### Is `set -x` safe in CI? Only if you are confident nothing sensitive passes through the script, and CI logs are frequently more widely readable than people assume. Turn it off around authentication and secret handling, or use a targeted `PS4` and enable it only for the sections you are debugging.