diff --git a/README.md b/README.md index 538bea8..62a0767 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ Together they: * require the `CONSOLE_USER` and `CONSOLE_REASON` environment variables * reject compound statements and redirections, while allowing the `--exit-code` marker the Heroku CLI appends * permit only unqualified `rails`, `rake` and `bundle exec rails|rake` invocations, minus an - explicit deny list + explicit deny list, and allowlist the options those may be given * warn if [dyno metadata](https://devcenter.heroku.com/articles/dyno-metadata) is not enabled * export `CONSOLE_AUDIT_ENABLED=true` @@ -116,8 +116,9 @@ duplicates every rule rather than delegating. | `rails credentials:*`, `rails encrypted:*` | Spawns `$EDITOR`, which the operator controls — a shell escape. `EDITOR` and `VISUAL` are also unset | | `rails runner -` (a bare `-` in any argument position) | Reads the program from **stdin**, so the executed code appears neither in the dyno command string nor in an `ARGV` capture inside the app. The session still produces a complete record with a correct user, reason and dyno UUID, while the code that ran is unrecorded | | `rails runner --file `, or any `runner` argument that exists on disk | Same shape — the command string names a file rather than the code that runs | +| Any argument beginning with `-` that is not on the option allowlist | See [Option allowlist](#option-allowlist) below | | `-c` in any argument position | Reaches a shell (`bash -c`). No legitimate `rails`/`rake` invocation uses it. `rails c` — the console shorthand — is unaffected, because that argument is `c`, not `-c` | -| `rails console --sandbox` / `-s` (console only) | The sandbox transaction is rolled back on exit, and a database-backed ActiveJob queue on the primary database puts the audit enqueue inside it — so the rollback discards the audit trail and the session runs entirely unlogged ([console1984#91](https://github.com/basecamp/console1984/issues/91)). Scoped to `console`/`c`, because `-s` is `rake`'s silent flag; `--no-sandbox` is unaffected | +| `rails console --sandbox` / `-s` (console only) | The sandbox transaction is rolled back on exit, and a database-backed ActiveJob queue on the primary database puts the audit enqueue inside it — so the rollback discards the audit trail and the session runs entirely unlogged ([console1984#91](https://github.com/basecamp/console1984/issues/91)). Scoped to `console`/`c`, because `-s` is `rake`'s silent flag. Thor also takes `--sandbox=`, so the `=` forms are refused whatever they carry, `false` included — `--no-sandbox` is the spelling that opts out, and is unaffected | Because these are checked after expansion, the quoted, variable and glob spellings of each are blocked too: `rails "dbconsole"`, `rails "credentials:edit"`, `rails runner "-"` and @@ -139,6 +140,52 @@ The `runner` file check tests whether the argument **exists on disk**, which is Rails itself makes. There is no heuristic on how the argument looks, so `rails runner 'Model.where(x: 1).rb'` is permitted and `rails runner ~/script` is not. +### Option allowlist + +Arguments beginning with `-` are **allowlisted, not screened**. Anything not named below is +refused. + +The reason is `rake -e/-p/-E CODE` (`--execute`, `--execute-print`, `--execute-continue`). Rake +evaluates `CODE` inside its own option parser — before the Rakefile is loaded, without booting +Rails — and then exits. Nothing the code does reaches the console audit hook, which makes it +weaker than the `rails runner 'system("bash")'` case [below](#limitations), where Rails at least +boots and the invocation is recorded. `rails` is affected too: it hands any command it does not +recognise to that same parser with the whole argv, so `rails -e CODE` and `rails db:migrate -e +CODE` reach it. + +A deny list would have to model which of Rake's short options take an argument, in order to know +where a bundle such as `-Ne` or `-se` stops being flags. Get that wrong for one option — in this +version of Rake or a later one — and the bundle hides an `-e`. An allowlist fails the other way: +an unlisted option is refused, so being wrong costs a denial rather than an unlogged shell. It +also refuses things nobody had to think of, such as `-g`/`--system`, which loads tasks from +`$HOME/.rake` — `/app/.rake` on a dyno. + +Every command gets a list; none is exempt. `rails console` and `rails runner` parse their own +options and never reach Rake, so `-e` there is the *environment* — but they get a list of their +own rather than being waved through, because a mistake in a list is a denial while a mistake in +an exemption is a silent bypass. + +| After | Permitted | +|---|---| +| `rails console` / `c` | `-e`/`--environment`, `--no-sandbox`, `-h`/`--help` | +| `rails runner` / `r` | `-e`/`--environment`, `-w`/`--skip-executor`, `-h`/`--help` | +| everything else (Rake's parser) | `-T`/`--tasks`, `-D`/`--describe`, `-W`/`--where`, `-P`/`--prereqs`, `-A`/`--all`, `--comments`, `--rules`, `-t`/`--trace`, `--backtrace`, `--job-stats`, `-s`/`--silent`, `-q`/`--quiet`, `-n`/`--dry-run`, `-v`/`--verbose`, `-V`/`--version`, `-m`/`--multitask`, `-j`/`--jobs`, `-B`/`--build-all`, `-X`/`--no-deprecation-warnings`, `-h`/`-H`/`--help` | + +Task names, task arguments (`some:task[a,b]`) and `VAR=value` assignments are not options and are +not screened, so `rake db:rollback STEP=99` is unaffected. + +Two consequences worth knowing before you hit them: + +- **Short options are matched whole**, so `-sq` is refused where `-s -q` is permitted. This is + what makes `-se CODE` refusable without reasoning about bundling at all. +- **Abbreviated long forms are refused.** Rake accepts `--task` for `--tasks`; the allowlist does + not. Denials list the permitted set, so this is self-service. + +Deliberately absent from the Rake list: `-e`/`-E`/`-p` (evaluate code), `-f`/`-r`/`-I`/`-R`/`-C` +(name a path — the same shape as a bare `-`), and `-g`/`-G`/`-N` (change which Rakefile is found). +Exploiting the path-naming options needs a file already in the slug, i.e. the same deploy-access +trust boundary as the `BASH_ENV` limitation below. + ### Blocked outright (non-Rails commands) These all fall through the allowlist. Named here because they are the cases most likely to come up: diff --git a/guard/shim.sh b/guard/shim.sh index 7c91f0a..b98b430 100755 --- a/guard/shim.sh +++ b/guard/shim.sh @@ -193,20 +193,26 @@ done # Scoped to `console`/`c` rather than applied to every argv, because `-s` is # `rake`'s silent flag and legitimate there. `--no-sandbox` must keep working. # +# Thor parses `--sandbox=true` as well as the bare flag, so the `=` forms are +# denied whatever value they carry. Enumerating Thor's boolean vocabulary would +# be modelling the parser, which is the thing the option allowlist below exists +# to avoid; `--no-sandbox` is the spelling that opts out. +# # The console_audit gem sets Rails' own `config.disable_sandbox = true` when # auditing is active, which is a second layer over the same dynos: it holds even # if the command never reaches this wrapper. if [[ "$_cg_policy_prog" == "rails" && ( "$_cg_sub" == "console" || "$_cg_sub" == "c" ) ]]; then for _cg_arg in "${_cg_policy_args[@]:1}"; do case "$_cg_arg" in - --sandbox|-s) + --sandbox|--sandbox=*|-s|-s=*) _cg_deny "\`rails ${_cg_sub} ${_cg_arg}\` is not permitted on one-off dynos." \ "" \ "A sandboxed console rolls back its transaction on exit, which" \ "discards the queued audit records with it -- the session would" \ "run entirely unlogged." \ "" \ - "Use \`rails ${_cg_sub}\` instead. It is audited." + "Use \`rails ${_cg_sub}\` instead. It is audited." \ + "\`--no-sandbox\` is permitted and means the same thing." ;; esac done @@ -240,4 +246,138 @@ if [[ "$_cg_policy_prog" == "rails" && ( "$_cg_sub" == "runner" || "$_cg_sub" == done fi +# ---------- option allowlist ---------- +# `rake -e/-p/-E CODE` evaluates CODE inside Rake's own option parser -- before +# the Rakefile is loaded and without booting Rails -- and then exits. Nothing the +# code does reaches the console audit hook, so it is weaker even than the +# `rails runner 'system("bash")'` case the README accepts as best effort, where +# Rails at least boots and the invocation is recorded. `-f/-r/-I/-R/-C/-g` name a +# path rather than the code that runs, which is what blocks a bare `-` above. +# +# Rails hands any command it does not recognise to that same parser with the +# whole argv, so `rails -e CODE` and `rails db:migrate -e CODE` reach it too. +# +# Allowlisted rather than screened. A deny list has to model which of Rake's +# short options take an argument, in order to know where a bundle such as `-Ne` +# stops being flags -- get that wrong for one option, in this version of Rake or +# a later one, and the bundle hides an `-e`. An allowlist fails the other way: an +# option nobody listed is refused, so the cost of being wrong is a denial rather +# than an unlogged shell. +# +# Every command gets a list; none is exempt. The two Rails commands below parse +# their own options and never reach Rake, so `-e` there is the environment -- +# but they are given a list of their own rather than being waved through, +# because a mistake in a list is a denial while a mistake in an exemption is a +# bypass, silently and with no failing test. +# +# _cg_allow_exact options taking no value: the token must match exactly, so +# `-se` is refused rather than read as a bundle +# _cg_allow_value options taking one: exactly, or with the value attached +# (`-T db`, `-Tdb`, `--tasks=db`) +_cg_allow_why=( + "Options are allowlisted here. Rake evaluates \`-e/-p/-E CODE\` in" + "its own option parser, before the Rakefile is loaded and without" + "booting Rails, so nothing that code does reaches the console audit" + "hook -- and Rails hands any command it does not recognise to that" + "same parser. Options naming a path are excluded for the reason a" + "bare \`-\` is." +) +_cg_allow_extras="task names, VAR=value assignments, and:" + +case "${_cg_policy_prog}/${_cg_sub}" in + rails/console|rails/c) + _cg_allow_exact="--no-sandbox -h --help" + _cg_allow_value="-e --environment" + _cg_allow_why=( + "Options are allowlisted here, so one nobody vetted is refused" + "rather than passed through to Rails." + ) + _cg_allow_extras="" + ;; + rails/runner|rails/r) + _cg_allow_exact="-w --skip-executor -h --help" + _cg_allow_value="-e --environment" + _cg_allow_why=( + "Options are allowlisted here, so one nobody vetted is refused" + "rather than passed through to Rails. The code to run is not an" + "option and needs no entry." + ) + _cg_allow_extras="" + ;; + *) + # Rake's read-only and output-shaping options. Absent, deliberately: + # -e/-E/-p (evaluate code), -f/-r/-I/-R/-C (name a path), and -g/-G/-N, + # which change which Rakefile is found -- `--system` loads tasks from + # $HOME/.rake, and $HOME is /app on a dyno. + _cg_allow_exact="-A -B -m -n -P -q -s -t -v -V -X -h -H" + _cg_allow_exact+=" --all --build-all --multitask --dry-run --prereqs" + _cg_allow_exact+=" --quiet --silent --verbose --version --comments --rules" + _cg_allow_exact+=" --no-deprecation-warnings --help" + _cg_allow_value="-T -D -W -j" + _cg_allow_value+=" --tasks --describe --where --jobs --trace --backtrace" + _cg_allow_value+=" --job-stats" + ;; +esac + +_cg_arg_permitted() { + local _cg_tok="$1" _cg_name + # Unquoted on purpose: the lists are space-delimited and must word-split. + for _cg_name in $_cg_allow_exact; do + [[ "$_cg_tok" == "$_cg_name" ]] && return 0 + done + for _cg_name in $_cg_allow_value; do + [[ "$_cg_tok" == "$_cg_name" ]] && return 0 + case "$_cg_name" in + --*) [[ "$_cg_tok" == "$_cg_name="* ]] && return 0 ;; + *) [[ "$_cg_tok" == "$_cg_name"?* ]] && return 0 ;; + esac + done + return 1 +} + +# Wrap the permitted set for the denial banner. Derived from the lists above +# rather than written out again, so the two cannot drift apart. +_cg_wrap_allowed() { + local _cg_line="" _cg_word + # shellcheck disable=SC2086 # deliberate word splitting, as above + for _cg_word in $_cg_allow_value $_cg_allow_exact; do + if (( ${#_cg_line} + ${#_cg_word} + 1 > 58 )); then + echo " $_cg_line" + _cg_line="$_cg_word" + else + _cg_line="${_cg_line:+$_cg_line }$_cg_word" + fi + done + [[ -n "$_cg_line" ]] && echo " $_cg_line" +} + +for _cg_arg in ${_cg_policy_args[@]+"${_cg_policy_args[@]}"}; do + # A bare `-` is handled above, with a message about stdin that says more than + # this one would. + [[ "$_cg_arg" == "-" ]] && continue + [[ "$_cg_arg" == -* ]] || continue + _cg_arg_permitted "$_cg_arg" && continue + + # Only when it names a command; for `rake -e 1` the "subcommand" is the + # rejected option itself. + _cg_context="$_cg_policy_prog" + [[ -n "$_cg_sub" && "$_cg_sub" != -* ]] && _cg_context+=" $_cg_sub" + + _cg_allowed_lines=() + while IFS= read -r _cg_line; do + _cg_allowed_lines+=("$_cg_line") + done < <(_cg_wrap_allowed) + + _cg_deny "\`${_cg_policy_prog} ${_cg_arg}\` is not permitted on one-off dynos." \ + "" \ + "${_cg_allow_why[@]}" \ + "" \ + "Permitted after \`${_cg_context}\`:${_cg_allow_extras:+ $_cg_allow_extras}" \ + "${_cg_allowed_lines[@]}" \ + "" \ + "" \ + "Short options are matched whole, so pass them separately rather" \ + "than bundled into one argument." +done + exec "$_cg_real" "$@" diff --git a/test/run_tests.sh b/test/run_tests.sh index 7409dcf..2c1ff8a 100755 --- a/test/run_tests.sh +++ b/test/run_tests.sh @@ -201,13 +201,134 @@ assert_blocked 'rails console -s' 'unlogged' assert_blocked 'bundle exec rails c --sandbox' 'unlogged' assert_blocked 'rails c "--sandbox"' 'unlogged' cg_env 'S=--sandbox' ; assert_blocked 'rails c $S' 'unlogged' +# Thor takes `--flag=value` for a boolean, so the `=` forms have to be denied by +# this rule and not merely by the option allowlist below -- otherwise adding a +# sandbox-ish entry to the console's allowlist reopens the bypass with the whole +# suite green. Denied whatever the value is, `false` included: deciding which +# values Thor reads as true is modelling the parser. +assert_blocked 'rails c "--sandbox=true"' 'unlogged' +assert_blocked 'rails console "--sandbox=true"' 'unlogged' +assert_blocked 'rails c "--sandbox=1"' 'unlogged' +assert_blocked 'rails c "--sandbox=false"' 'unlogged' +assert_blocked 'rails c "-s=true"' 'unlogged' +assert_blocked 'bundle exec rails c "--sandbox=true"' 'unlogged' +cg_env 'S=--sandbox=true' ; assert_blocked 'rails c $S' 'unlogged' +# The denial points at the spelling that works. +assert_output 'the sandbox denial names --no-sandbox' \ + 'rails c "--sandbox=true"' '`--no-sandbox` is permitted' # Scoped to the console: -s is rake's silent flag, and --no-sandbox is the safe # direction. Neither is collateral damage. assert_ran 'rake -s some:task' assert_ran 'rails c --no-sandbox' -assert_ran 'rails runner -s' assert_ran 'rails c' +cg_section "rake evaluates code in its own option parser" +# -e/-p/-E eval and exit inside rake's option parser, before the Rakefile is +# loaded, so nothing boots and the audit hook records nothing at all. +assert_blocked 'rake -e 1' 'allowlisted' +assert_blocked 'rake --execute 1' 'allowlisted' +assert_blocked 'rake -p 1+1' 'allowlisted' +assert_blocked 'rake -E 1' 'allowlisted' +assert_blocked 'rake --execute-print 1' 'allowlisted' +assert_blocked 'rake --execute-continue 1' 'allowlisted' +assert_blocked 'rake "--execute=1"' 'allowlisted' +assert_blocked 'bundle exec rake -e 1' 'allowlisted' +cg_env 'P=-e' ; assert_blocked 'rake $P 1' 'allowlisted' +# Short options bundle in rake, so the allowlist matches whole tokens: `-s` is +# permitted but `-se` is a different token and is refused. +assert_blocked 'rake -Ne 1' 'allowlisted' +assert_blocked 'rake -se 1' 'allowlisted' +assert_blocked 'rake -qsNe 1' 'allowlisted' +# ...which also means a bundle of two permitted flags is refused. Cheap: -s -q. +assert_blocked 'rake -sq some:task' 'matched whole' +# Abbreviated long forms, which rake accepts and the allowlist does not. +assert_blocked 'rake --exec 1' 'allowlisted' +assert_blocked 'rake --ex 1' 'allowlisted' +assert_blocked 'rake --task' 'allowlisted' +# Options that name a path rather than the code that runs. +assert_blocked 'rake -f Rakefile some:task' 'allowlisted' +assert_blocked 'rake -r ./payload some:task' 'allowlisted' +assert_blocked 'rake -I /app some:task' 'allowlisted' +assert_blocked 'rake -R /app some:task' 'allowlisted' +assert_blocked 'rake -C /app some:task' 'allowlisted' +assert_blocked 'rake --require ./payload' 'allowlisted' +assert_blocked 'rake --rakefile Rakefile' 'allowlisted' +# `--system` loads tasks from $HOME/.rake, and $HOME is /app on a dyno. The +# allowlist refuses these without anyone having had to think of them. +assert_blocked 'rake -g some:task' 'allowlisted' +assert_blocked 'rake -G some:task' 'allowlisted' +assert_blocked 'rake -N some:task' 'allowlisted' +assert_blocked 'rake --system some:task' 'allowlisted' +assert_blocked 'rake --suppress-backtrace x' 'allowlisted' +assert_blocked 'rake --no-such-option' 'allowlisted' +# Rails hands a command it does not recognise to rake's option parser, argv and +# all, so the same options arrive by way of `rails`. +assert_blocked 'rails -e 1' 'allowlisted' +assert_blocked 'rails db:migrate -e 1' 'allowlisted' +assert_blocked 'bundle exec rails -e 1' 'allowlisted' +# The denial names what is permitted, so a false positive is self-service. +assert_output 'the denial lists the permitted options' \ + 'rake --no-such-option' '--tasks' +assert_output 'the denial names the command it applies to' \ + 'rails db:migrate --no-such-option' 'Permitted after `rails db:migrate`' + +cg_section "the permitted rake options still work" +assert_ran 'rake -T' 'rake -T' +assert_ran 'rake -T db' +assert_ran 'rake -Tdb' 'rake -Tdb' +assert_ran 'rake "--tasks=db"' +assert_ran 'rake -D db' +assert_ran 'rake -W some:task' +assert_ran 'rake -P' +assert_ran 'rake -s some:task' +assert_ran 'rake -q some:task' +assert_ran 'rake -n some:task' +assert_ran 'rake -t some:task' +assert_ran 'rake -v some:task' +assert_ran 'rake -V' +assert_ran 'rake -A -T' +assert_ran 'rake -B some:task' +assert_ran 'rake -m some:task' +assert_ran 'rake -j 4 some:task' +assert_ran 'rake -j4 some:task' +assert_ran 'rake -X some:task' +assert_ran 'rake --trace some:task' 'rake --trace some:task' +assert_ran 'rake "--trace=stderr" some:task' +assert_ran 'rake --backtrace some:task' +assert_ran 'rake --dry-run some:task' +assert_ran 'rake --all --tasks' +assert_ran 'rake --comments --tasks' +assert_ran 'rake --rules' +assert_ran 'rake --job-stats some:task' +assert_ran 'rake --silent some:task' +assert_ran 'rake --version' +# Task names, task arguments and VAR=value assignments are not options and are +# not screened. +assert_ran 'rake db:rollback STEP=99' 'rake db:rollback STEP=99' +assert_ran 'rake "some:task[a,b]"' +assert_ran 'rake -s db:migrate STEP=1' + +cg_section "the commands Rails parses itself get their own list" +# `-e` is the environment here, not rake's execute. These commands never reach +# rake's parser -- but they are allowlisted rather than exempted, so a mistake +# in the list is a denial rather than a silent bypass. +assert_ran 'rails runner -e production Model.foo' +assert_ran 'rails runner --environment production Model.foo' +assert_ran 'rails runner -w Model.foo' +assert_ran 'rails c -e production' +assert_ran 'rails console --environment production' +assert_ran 'rails c --no-sandbox' +assert_ran 'rails c' +assert_ran 'bundle exec rails c -e production' +# Options neither Rails command takes are refused rather than passed through. +assert_blocked 'rails c --no-such-option' 'allowlisted' +assert_blocked 'rails runner -f Model.foo' 'allowlisted' +assert_blocked 'rails c -w' 'allowlisted' +# regression: `-s` reaches neither parser as anything useful. The rule that +# matters -- the sandbox denial is scoped to the console -- is pinned by +# `rake -s` above and `rails c -s` below. +assert_blocked 'rails runner -s Model.foo' 'allowlisted' + cg_section "regression (finding 2): parameter expansion must not defeat policy" cg_env 'P=-' ; assert_blocked 'rails runner $P' 'stdin' cg_env 'P=-' ; assert_blocked 'rails runner "$P"' 'stdin'