diff --git a/src/deploy/test_hive_snapshot_unit_contract.sh b/src/deploy/test_hive_snapshot_unit_contract.sh new file mode 100644 index 000000000..e2ce561ca --- /dev/null +++ b/src/deploy/test_hive_snapshot_unit_contract.sh @@ -0,0 +1,184 @@ +#!/usr/bin/env bash +# hive-snapshot.service must not execute code out of a directory a local user +# could have planted (#5483, same class as #5435). +# Run: bash src/deploy/test_hive_snapshot_unit_contract.sh +# +# WHAT THE BUG WAS. The unit ran: +# +# 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 +# /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. +# +# 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. +set -uo pipefail + +PASS=0 +FAIL=0 +pass() { echo " PASS: $1"; PASS=$((PASS + 1)); } +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" + +echo "=== hive-snapshot.service does not execute planted code (#5483) ===" + +for f in "$UNIT" "$GUARD"; do + if [ ! -f "$f" ]; then + fail "locate $f" "the layout moved — this test cannot verify anything" + echo "" + echo "=== Results: $PASS passed, $FAIL failed ===" + exit 1 + fi +done + +WORK="$(mktemp -d)" +trap 'rm -rf "$WORK"' EXIT + +# --- the unit wires the guard in --------------------------------------------- +echo "" +echo "--- the unit file ---" + +GUARD_LINE="$(grep -E '^ExecStartPre=' "$UNIT" || true)" +if [ -z "$GUARD_LINE" ]; then + fail "hive-snapshot.service has an ExecStartPre guard" \ + "without one, systemd executes whatever publish-snapshot.sh is present when the timer fires" +else + pass "hive-snapshot.service has an ExecStartPre guard" + if printf '%s' "$GUARD_LINE" | grep -q 'hive-checkout-guard.sh'; then + pass "the guard is hive-checkout-guard.sh (exit status is the assertion)" + 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-)" + 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})" + else + fail "the guard checks the directory and file ExecStart runs (${EXEC_PATH})" "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. +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" +else + pass "PrivateTmp is not set (it would hide the /tmp checkout this unit runs from)" +fi + +for d in NoNewPrivileges ProtectSystem; do + if grep -qE "^${d}=" "$UNIT"; then + pass "${d} is set" + else + fail "${d} is set" "unit lost a hardening directive" + fi +done + +# ProtectSystem=strict would make /tmp read-only for the service, and the +# script builds the snapshot under /tmp. Same trap as PrivateTmp. +if grep -qE '^ProtectSystem=strict' "$UNIT"; then + fail "ProtectSystem is not 'strict'" "strict mounts /tmp read-only; this unit builds under /tmp" +else + pass "ProtectSystem is not 'strict' (which would make the /tmp checkout read-only)" +fi + +# --- the guard, run with this unit's argument shape --------------------------- +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. +# $WORK/tmp/hive/dashboard/publish-snapshot.sh +mk_tree() { + rm -rf "${WORK}/tmp" + mkdir -p "${WORK}/tmp/hive/dashboard" + chmod 1777 "${WORK}/tmp" + chmod 755 "${WORK}/tmp/hive" "${WORK}/tmp/hive/dashboard" + : > "${WORK}/tmp/hive/dashboard/publish-snapshot.sh" + chmod 755 "${WORK}/tmp/hive/dashboard/publish-snapshot.sh" +} + +expect() { + local want="$1" label="$2" + local out rc + out="$(bash "$GUARD" "${WORK}/tmp/hive/dashboard" publish-snapshot.sh 2>&1)" + rc=$? + if [ "$rc" -eq "$want" ]; then + if [ "$want" -eq 0 ]; then + pass "${label}: guard allows startup (rc=0)" + else + pass "${label}: guard REFUSES startup (rc=${rc})" + fi + else + fail "${label}: expected rc=${want}, got rc=${rc}" "${out}" + fi +} + +# The good state: a healthy checkout with the script at its shipped 755 mode. +# If this fails the guard is too strict and snapshots stop on a healthy host. +mk_tree +expect 0 "healthy checkout under a sticky /tmp (script mode 755)" + +# THE VULNERABILITY, reproduced: /tmp wiped by a reboot, checkout not yet +# restored. Before this fix, the next timer fire ran whatever was there. +mk_tree +rm -rf "${WORK}/tmp/hive" +mkdir -p "${WORK}/tmp/hive/dashboard" +: > "${WORK}/tmp/hive/dashboard/publish-snapshot.sh" +chmod 777 "${WORK}/tmp/hive" "${WORK}/tmp/hive/dashboard" +chmod 755 "${WORK}/tmp/hive/dashboard/publish-snapshot.sh" +expect 1 "post-reboot window, attacker-shaped world-writable checkout (the #5483 race)" + +# The entrypoint is a 755 SCRIPT, so unlike bot.js the writable-file check is +# the one that matters here: a group/world-writable script can be rewritten in +# place between guard check and the next timer fire. +mk_tree +chmod 775 "${WORK}/tmp/hive/dashboard/publish-snapshot.sh" +expect 1 "publish-snapshot.sh is group-writable" + +mk_tree +chmod 777 "${WORK}/tmp/hive/dashboard/publish-snapshot.sh" +expect 1 "publish-snapshot.sh is world-writable" + +# A symlinked entrypoint redirects an otherwise-clean path at attacker code. +mk_tree +rm -f "${WORK}/tmp/hive/dashboard/publish-snapshot.sh" +: > "${WORK}/evil.sh" +chmod 755 "${WORK}/evil.sh" +ln -s "${WORK}/evil.sh" "${WORK}/tmp/hive/dashboard/publish-snapshot.sh" +expect 1 "publish-snapshot.sh is a symlink" + +# Missing checkout is a refusal, not a skip. +mk_tree +rm -rf "${WORK}/tmp/hive/dashboard" +expect 1 "the checkout directory does not exist" + +echo "" +echo "=== Results: $PASS passed, $FAIL failed ===" +[ "$FAIL" -eq 0 ] diff --git a/systemd/hive-snapshot.service b/systemd/hive-snapshot.service index 7f12774bb..fc2145972 100644 --- a/systemd/hive-snapshot.service +++ b/systemd/hive-snapshot.service @@ -9,7 +9,40 @@ User=dev Environment=PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin Environment=HIVE_DASHBOARD_URL=http://localhost:3001 Environment=DOCS_REPO_DIR=/tmp/kubestellar-docs-snapshot +# Refuse to start unless the checkout we execute from is owned by this service +# user and is not group/other-writable (#5483, same class as #5435). +# +# WHY THIS GUARD EXISTS. ExecStart resolves out of /tmp/hive, a predictable +# path under 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 after a reboot wipes /tmp and before hive-deploy repopulates +# the checkout. When hive-snapshot.timer next fires, systemd would run that +# file as `dev`, and the script it replaces 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. +# +# The path stays under /tmp deliberately: the literal /tmp/hive checkout +# location is shared with bin/hive.sh, bin/kick-agents.sh and hive-deploy; +# relocating it is a deploy-layout change, not a fix to this unit. The guard +# closes the code-execution path without moving anything. See +# bin/hive-checkout-guard.sh for why this is a script whose EXIT STATUS is the +# assertion rather than an inline `find` (which exits 0 even on no match). +# +# Test with: bash src/deploy/test_hive_snapshot_unit_contract.sh +ExecStartPre=/usr/local/bin/hive-checkout-guard.sh /tmp/hive/dashboard publish-snapshot.sh ExecStart=/tmp/hive/dashboard/publish-snapshot.sh TimeoutStartSec=120 +# Hardening (mirrors hive-discord.service, #5481). PrivateTmp is deliberately +# NOT set: it would give this unit a private /tmp namespace, hiding the very +# checkout ExecStart points at, and the unit could not start. Likewise +# ProtectSystem must not be 'strict' — strict mounts /tmp read-only and the +# script builds the snapshot under /tmp. ProtectHome=read-only still lets +# git/gh read ~/.gitconfig while blocking writes outside /tmp and /var. +NoNewPrivileges=yes +ProtectSystem=full +ProtectHome=read-only +PrivateDevices=yes +RestrictSUIDSGID=yes StandardOutput=journal StandardError=journal