🔒 fix(security): stop hive-snapshot.service executing code planted under /tmp (#5483)

.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

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

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"

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 <dir> -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 <dir> <basename>, 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 <rc> <label> -- runs the guard exactly as the unit does.
 expect() {
   local want="$1" label="$2"
   local out rc
@@ -140,45 +170,113 @@ expect() {
   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.
+# The good state. If this ever fails the guard is too strict and would stop the
+# snapshot timer on a healthy host — a worse outcome than the bug being fixed.
+# This also pins that a 755 entrypoint passes: the file ships executable, so a
+# guard that rejected the exec bit would break every run.
 mk_tree
-expect 0 "healthy checkout under a sticky /tmp (script mode 755)"
+expect 0 "healthy checkout under a sticky /tmp, entrypoint 755 as shipped"
 
-# THE VULNERABILITY, reproduced: /tmp wiped by a reboot, checkout not yet
-# restored. Before this fix, the next timer fire ran whatever was there.
+# THE VULNERABILITY, reproduced. This is the pre-fix arrangement: /tmp wiped by
+# a reboot, the checkout not yet restored, so the directory the unit executes
+# from is one any local user can create and fill. Before this fix systemd ran
+# whatever it found when the timer fired; the guard must refuse.
 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.
+rm -f "${WORK}/tmp/hive/dashboard/publish-snapshot.sh"
+expect 1 "post-reboot window, publish-snapshot.sh absent (the #5483 race)"
+
+# The attacker's own tree: they got there first and it is theirs to rewrite.
 mk_tree
-chmod 775 "${WORK}/tmp/hive/dashboard/publish-snapshot.sh"
-expect 1 "publish-snapshot.sh is group-writable"
+chmod 777 "${WORK}/tmp/hive/dashboard"
+expect 1 "checkout directory is world-writable"
+
+# Sticky on the LEAF must not buy an exemption. Sticky protects existing entries
+# from replacement, but new files are exactly the attack.
+mk_tree
+chmod 1777 "${WORK}/tmp/hive/dashboard"
+expect 1 "checkout directory is world-writable even with the sticky bit"
+
+# An intermediate the attacker controls lets them swap the whole subtree, so a
+# leaf-only check is not enough.
+mk_tree
+chmod 777 "${WORK}/tmp/hive"
+expect 1 "an ancestor is world-writable without the sticky bit"
+
+mk_tree
+chmod 775 "${WORK}/tmp/hive"
+expect 1 "an ancestor is group-writable"
 
+# A writable entrypoint can be rewritten in place before the next timer firing,
+# so it is rejected even inside an otherwise-safe directory. 757/775 rather than
+# 666/664 because this file legitimately carries the exec bit — the guard must
+# key on the WRITE bits, not on mode equality.
 mk_tree
-chmod 777 "${WORK}/tmp/hive/dashboard/publish-snapshot.sh"
-expect 1 "publish-snapshot.sh is world-writable"
+chmod 757 "${WORK}/tmp/hive/dashboard/publish-snapshot.sh"
+expect 1 "publish-snapshot.sh is world-writable (and executable, as it ships)"
 
-# A symlinked entrypoint redirects an otherwise-clean path at attacker code.
+mk_tree
+chmod 775 "${WORK}/tmp/hive/dashboard/publish-snapshot.sh"
+expect 1 "publish-snapshot.sh is group-writable (and executable, as it ships)"
+
+# Symlinks are how an attacker redirects an otherwise-clean path at their own
+# code, so neither the directory nor the entrypoint may be one.
 mk_tree
 rm -f "${WORK}/tmp/hive/dashboard/publish-snapshot.sh"
-: > "${WORK}/evil.sh"
+printf '#!/bin/sh\n' > "${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
+mv "${WORK}/tmp/hive/dashboard" "${WORK}/elsewhere"
+ln -s "${WORK}/elsewhere" "${WORK}/tmp/hive/dashboard"
+expect 1 "the checkout directory is a symlink"
+
+# The guard must not be fooled into passing when it cannot tell — a missing
+# directory is a refusal, not a skip.
 mk_tree
 rm -rf "${WORK}/tmp/hive/dashboard"
 expect 1 "the checkout directory does not exist"
 
+# --- the guard is installed before anything references it --------------------
+echo ""
+echo "--- deploy bootstraps the guard ---"
+
+# hive-deploy.sh's sync loops both skip files that are not already installed, so
+# a NEW helper is never bootstrapped by them. #5481 added the explicit install
+# for hive-discord.service; this unit's ExecStartPre depends on exactly the same
+# file, so the assertion is repeated here rather than assumed — if that block is
+# ever removed, BOTH units stop starting and both tests should say so.
+DEPLOY="${ROOT}/bin/hive-deploy.sh"
+if [ ! -f "$DEPLOY" ]; then
+  fail "locate bin/hive-deploy.sh" "cannot verify the guard is installed"
+elif grep -q 'hive-checkout-guard.sh' "$DEPLOY"; then
+  pass "hive-deploy.sh installs hive-checkout-guard.sh explicitly"
+  if grep -A5 'CHECKOUT_GUARD_SRC=' "$DEPLOY" | grep -q 'install -m 0755'; then
+    pass "and installs it executable (0755)"
+  else
+    fail "the guard is installed executable" "ExecStartPre needs the exec bit"
+  fi
+else
+  fail "hive-deploy.sh installs hive-checkout-guard.sh" \
+       "the drift loops skip files that are not already installed, so an upgraded host would never receive it and hive-snapshot.service would fail to start"
+fi
+
+if [ -x "$GUARD" ]; then
+  pass "bin/hive-checkout-guard.sh is executable in git"
+else
+  fail "bin/hive-checkout-guard.sh is executable in git" "mode is $(ls -l "$GUARD" | cut -d' ' -f1)"
+fi
+
+# The real entrypoint must itself be executable in git, since ExecStart invokes
+# it directly rather than through an interpreter.
+if [ -x "$SCRIPT" ]; then
+  pass "dashboard/publish-snapshot.sh is executable in git"
+else
+  fail "dashboard/publish-snapshot.sh is executable in git" \
+       "ExecStart runs it directly, so a non-executable file fails at start"
+fi
+
 echo ""
 echo "=== Results: $PASS passed, $FAIL failed ==="
 [ "$FAIL" -eq 0 ]