diff --git a/.github/workflows/v2-ci.yml b/.github/workflows/v2-ci.yml index 3ce086ee7..a7426898a 100644 --- a/.github/workflows/v2-ci.yml +++ b/.github/workflows/v2-ci.yml @@ -552,6 +552,17 @@ jobs: working-directory: . run: bash src/deploy/test_hive_discord_unit_contract.sh + # #5483: hive-snapshot.service is the same class as #5435 — it ran + # ExecStart=/tmp/hive/dashboard/publish-snapshot.sh, resolving the executed + # script out of the same world-writable, reboot-cleared parent. Lower + # severity (Type=oneshot on a timer, no EnvironmentFile) but the script + # reads a GitHub App token and pushes to kubestellar/docs. The same guard + # generalizes unmodified. Registered separately because these deploy tests + # are wired INDIVIDUALLY here — an unregistered test silently never runs. + - name: hive-snapshot.service does not execute planted code (#5483) + working-directory: . + run: bash src/deploy/test_hive_snapshot_unit_contract.sh + # #4440: the changelog reminder promises it "always succeeds" and "never # blocks a merge", and it did neither on a fork PR — GITHUB_TOKEN is # read-only there, the comment POST 403s, and `bash -e` made that a red diff --git a/CHANGELOG.md b/CHANGELOG.md index 0528a7ba3..bd7557c10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ Hive did not historically maintain a complete changelog. This file starts a prag ## Unreleased +### Security + +- `hive-snapshot.service` no longer executes code out of a path an unprivileged local user could have planted ([#5483](https://github.com/kubestellar/hive/issues/5483)). This is the same class as [#5435](https://github.com/kubestellar/hive/issues/5435) and the last remaining unit in `systemd/` that ran an `ExecStart` out of the checkout: `ExecStart=/tmp/hive/dashboard/publish-snapshot.sh` resolved the executed script out of a world-writable parent that is cleared on every reboot, so a local user who created `/tmp/hive/dashboard/publish-snapshot.sh` before `hive-deploy` repopulated the checkout got code execution as the service user. Severity is lower than #5435 — the unit is `Type=oneshot` driven by `hive-snapshot.timer` rather than `Restart=always`, so the planted code runs on the timer's 15-minute schedule rather than immediately at boot, and the unit carries no `EnvironmentFile`, so systemd hands it no credential — but it is not zero: the script itself reads a GitHub App token from `/var/run/hive-metrics/gh-app-token.cache` and pushes to `kubestellar/docs`. The unit now runs `bin/hive-checkout-guard.sh` as an `ExecStartPre`, unchanged from #5481, and refuses to start unless every directory from `/` down to the checkout is owned by the service user or root and not writable by anyone else (a sticky ancestor such as `/tmp` is accepted, the leaf is not), and unless `publish-snapshot.sh` exists, is a regular file rather than a symlink, and is not group- or world-writable — the file ships mode 755, so it is the write bits rather than the exec bit that matter here. `NoNewPrivileges`, `ProtectSystem=full`, `PrivateDevices` and `RestrictSUIDSGID` are set alongside it. **The checkout stays at `/tmp/hive` and no path changes**, so existing hosts keep working. `PrivateTmp` is deliberately *not* set and `ProtectSystem` is deliberately not `strict` — either would hide or freeze both the checkout and the `/tmp/kubestellar-docs-snapshot` clone the script commits from; the contract test pins both as absent. `ProtectHome` is deliberately *not* `read-only` here, unlike `hive-discord.service`, because this script drives `git` and `gh`, which write under `$HOME`; that difference is pinned by the test as well so a later consistency pass cannot silently break the snapshot push. + ## 2026-09-01 (v4.0.2) ### Fixed diff --git a/bin/hive-deploy.sh b/bin/hive-deploy.sh index f63707fd0..e045cb7c9 100755 --- a/bin/hive-deploy.sh +++ b/bin/hive-deploy.sh @@ -103,11 +103,12 @@ if [ -f "$BASELINE_HELPER_SRC" ] && ! cmp -s "$BASELINE_HELPER_SRC" "$BASELINE_H SYNCED="$SYNCED hive-baseline-check.sh" fi -# Same bootstrap problem as the baseline helper above: hive-discord.service's -# ExecStartPre references /usr/local/bin/hive-checkout-guard.sh (#5435), and +# Same bootstrap problem as the baseline helper above: the ExecStartPre of both +# hive-discord.service (#5435) and hive-snapshot.service (#5483) references +# /usr/local/bin/hive-checkout-guard.sh, and # neither sync loop above can create it — both skip any file that is not # already installed. Without this block an upgraded host would pull a unit that -# calls a script it does not have, and systemd would refuse to start the bot. +# calls a script it does not have, and systemd would refuse to start the unit. # Install it BEFORE the unit files are reinstalled by kick-agents.sh. CHECKOUT_GUARD_SRC="$HIVE_REPO/bin/hive-checkout-guard.sh" CHECKOUT_GUARD_DST="$INSTALL_DIR/hive-checkout-guard.sh" diff --git a/src/deploy/test_hive_snapshot_unit_contract.sh b/src/deploy/test_hive_snapshot_unit_contract.sh old mode 100644 new mode 100755 index e2ce561ca..62c8cc20c --- a/src/deploy/test_hive_snapshot_unit_contract.sh +++ b/src/deploy/test_hive_snapshot_unit_contract.sh @@ -5,28 +5,32 @@ # # WHAT THE BUG WAS. The unit ran: # +# Type=oneshot # User=dev # ExecStart=/tmp/hive/dashboard/publish-snapshot.sh # -# so the executed script resolved out of a world-writable parent that is -# CLEARED ON REBOOT. /tmp's sticky bit only stops a user from deleting or -# renaming entries owned by someone else; it does not stop them creating +# so the executed script resolved out of a world-writable parent that is CLEARED +# ON REBOOT. /tmp's sticky bit only stops a user from deleting or renaming +# entries owned by someone else; it does not stop them creating # /tmp/hive/dashboard/publish-snapshot.sh in the window between the boot that -# wipes /tmp and hive-deploy repopulating the checkout. When -# hive-snapshot.timer next fired, the winner of that race got code execution -# as `dev` — and the real script reads the GitHub App token from -# /var/run/hive-metrics/gh-app-token.cache, so the planted code runs with a -# path to that credential. +# wipes /tmp and hive-deploy repopulating the checkout. The winner of that race +# gets code execution as `dev` the next time hive-snapshot.timer fires. # -# WHY THIS TEST RUNS THE GUARD INSTEAD OF ONLY GREPPING THE UNIT. A grep for -# "ExecStartPre" would pass against a guard that never rejects anything — -# exactly the no-op-guard failure #5398 found in another contract test. So -# besides pinning the unit's directives, this EXECUTES bin/hive-checkout-guard.sh -# with the unit's actual arguments against real directory trees and asserts on -# its EXIT STATUS. The exhaustive per-attack-shape matrix already lives in -# src/deploy/test_hive_discord_unit_contract.sh; this test re-runs the shapes -# specific to THIS unit: the entrypoint is the executed script itself (shipped -# mode 755), so the writable-file check — not the exec-bit — is the live one. +# Severity is lower than #5435 — oneshot on a 15-minute timer rather than +# Restart=always, and no EnvironmentFile hands it a token — but not zero: the +# script reads a GitHub App token from /var/run/hive-metrics/gh-app-token.cache +# and pushes to kubestellar/docs. +# +# WHY THIS TEST RUNS THE GUARD INSTEAD OF GREPPING THE UNIT. A grep for +# "ExecStartPre" would pass against a guard that never rejects anything — and +# that is not hypothetical: the obvious inline form, +# +# ExecStartPre=/usr/bin/find -maxdepth 0 -user dev ! -perm /go=w -print -quit +# +# exits 0 whether or not it matched, so the unsafe case starts the unit anyway. +# So the assertions below EXECUTE bin/hive-checkout-guard.sh against real +# directory trees and assert on its EXIT STATUS, once per attack shape, +# including the pre-fix arrangement which must be rejected. set -uo pipefail PASS=0 @@ -37,10 +41,11 @@ fail() { echo " FAIL: $1"; [ $# -gt 1 ] && echo " $2"; FAIL=$((FAIL + 1) ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" UNIT="${ROOT}/systemd/hive-snapshot.service" GUARD="${ROOT}/bin/hive-checkout-guard.sh" +SCRIPT="${ROOT}/dashboard/publish-snapshot.sh" echo "=== hive-snapshot.service does not execute planted code (#5483) ===" -for f in "$UNIT" "$GUARD"; do +for f in "$UNIT" "$GUARD" "$SCRIPT"; do if [ ! -f "$f" ]; then fail "locate $f" "the layout moved — this test cannot verify anything" echo "" @@ -56,6 +61,8 @@ trap 'rm -rf "$WORK"' EXIT echo "" echo "--- the unit file ---" +# Read the directive out of the unit rather than restating it, so a unit that +# drops the guard fails here instead of silently passing the behaviour tests. GUARD_LINE="$(grep -E '^ExecStartPre=' "$UNIT" || true)" if [ -z "$GUARD_LINE" ]; then fail "hive-snapshot.service has an ExecStartPre guard" \ @@ -67,28 +74,37 @@ else else fail "the guard is hive-checkout-guard.sh" "got: ${GUARD_LINE}" fi - # The guard must be told the exact file ExecStart runs. ExecStart here is an - # absolute path; the guard takes , so both derived parts must - # appear in the guard invocation or it validates something else entirely. - EXEC_PATH="$(grep -E '^ExecStart=' "$UNIT" | head -1 | cut -d= -f2-)" + + # The guard must be told which file ExecStart will run. Checking the directory + # alone would let a planted publish-snapshot.sh through. Unlike the discord + # unit, ExecStart here is an ABSOLUTE path (there is no WorkingDirectory), so + # the directory and basename are derived from it and both must appear in the + # guard line — otherwise a future edit could point ExecStart at one path while + # the guard keeps validating another. + EXEC_PATH="$(grep -E '^ExecStart=' "$UNIT" | head -1 | sed 's/^ExecStart=//')" EXEC_DIR="$(dirname "$EXEC_PATH")" EXEC_FILE="$(basename "$EXEC_PATH")" - if printf '%s' "$GUARD_LINE" | grep -qF -- "$EXEC_DIR" && \ - printf '%s' "$GUARD_LINE" | grep -qF -- "$EXEC_FILE"; then - pass "the guard checks the directory and file ExecStart actually runs (${EXEC_PATH})" + if printf '%s' "$GUARD_LINE" | grep -qF -- "$EXEC_FILE"; then + pass "the guard checks the file ExecStart actually runs (${EXEC_FILE})" + else + fail "the guard checks the file ExecStart runs (${EXEC_FILE})" "got: ${GUARD_LINE}" + fi + if printf '%s' "$GUARD_LINE" | grep -qF -- "$EXEC_DIR"; then + pass "the guard checks the directory ExecStart runs from (${EXEC_DIR})" else - fail "the guard checks the directory and file ExecStart runs (${EXEC_PATH})" "got: ${GUARD_LINE}" + fail "the guard checks the directory ExecStart runs from (${EXEC_DIR})" "got: ${GUARD_LINE}" fi fi -# PrivateTmp would give the unit a private /tmp, hiding the very checkout -# ExecStart points at — the unit could never start. Asserting its ABSENCE keeps -# a future "more hardening is better" edit from silently breaking snapshots. +# PrivateTmp would give the unit a private /tmp, hiding BOTH the checkout +# ExecStart runs from and DOCS_REPO_DIR=/tmp/kubestellar-docs-snapshot, which +# the script clones into. Asserting its ABSENCE keeps a future "more hardening +# is better" edit from breaking the snapshot job. if grep -qE '^PrivateTmp=(yes|true|1)' "$UNIT"; then fail "PrivateTmp is not set on this unit" \ - "ExecStart is under /tmp; a private /tmp hides the checkout and the unit cannot start" + "ExecStart and DOCS_REPO_DIR are both under /tmp; a private /tmp hides them and the unit cannot work" else - pass "PrivateTmp is not set (it would hide the /tmp checkout this unit runs from)" + pass "PrivateTmp is not set (it would hide the /tmp checkout and the /tmp docs clone)" fi for d in NoNewPrivileges ProtectSystem; do @@ -99,22 +115,35 @@ for d in NoNewPrivileges ProtectSystem; do fi done -# ProtectSystem=strict would make /tmp read-only for the service, and the -# script builds the snapshot under /tmp. Same trap as PrivateTmp. +# ProtectSystem=strict would make /tmp read-only for the service. Same trap. if grep -qE '^ProtectSystem=strict' "$UNIT"; then - fail "ProtectSystem is not 'strict'" "strict mounts /tmp read-only; this unit builds under /tmp" + fail "ProtectSystem is not 'strict'" "strict mounts /tmp read-only; this unit runs out of /tmp and writes a git clone there" else pass "ProtectSystem is not 'strict' (which would make the /tmp checkout read-only)" fi -# --- the guard, run with this unit's argument shape --------------------------- +# ProtectHome=read-only is correct for hive-discord.service but WRONG here, and +# this assertion records why so the "make the units consistent" follow-up does +# not quietly break the snapshot push. publish-snapshot.sh drives git and gh, +# both of which write under $HOME. +if grep -qE '^ProtectHome=(read-only|yes|true)' "$UNIT"; then + fail "ProtectHome does not make \$HOME read-only for this unit" \ + "publish-snapshot.sh runs git and gh, which write to \$HOME (gh config/state, git lock files)" +else + pass "ProtectHome is not read-only (git and gh in publish-snapshot.sh write to \$HOME)" +fi + +# --- the guard actually rejects things --------------------------------------- echo "" echo "--- the guard, run against real trees ---" # Build a tree shaped like the real one: a sticky world-writable ancestor -# standing in for /tmp, with the checkout underneath it and the entrypoint -# shipped exactly as git ships it — an executable 755 script. +# standing in for /tmp, with the checkout underneath it. # $WORK/tmp/hive/dashboard/publish-snapshot.sh +# +# Note the mode: publish-snapshot.sh ships 755 in git, unlike bot.js which is +# 644. The guard's group/other-WRITABLE check is what matters here — the exec +# bit is irrelevant to it — so 755 must be accepted. mk_tree() { rm -rf "${WORK}/tmp" mkdir -p "${WORK}/tmp/hive/dashboard" @@ -124,6 +153,7 @@ mk_tree() { chmod 755 "${WORK}/tmp/hive/dashboard/publish-snapshot.sh" } +# expect