fix(security): stop hive-discord.service executing code planted under /tmp (#5435)

[clubanderson]

Security Fix

Files claimed: systemd/hive-discord.service, bin/hive-checkout-guard.sh (new), bin/hive-deploy.sh, src/deploy/test_hive_discord_unit_contract.sh (new), .github/workflows/v2-ci.yml, CHANGELOG.md.

hive-discord.service runs node bot.js with WorkingDirectory=/tmp/hive/discord, so the executed file resolves out of a world-writable parent that is cleared on every reboot. /tmp's sticky bit stops a user deleting or renaming entries owned by someone else — it does not stop them creating /tmp/hive/discord/bot.js in the window after a reboot wipes /tmp and before hive-deploy repopulates the checkout. The winner of that race gets code execution as dev with the Discord bot token from /etc/hive/discord.env. Requires=hive.service orders startup and validates nothing about who owns the code.

The fix

ExecStartPre=/usr/local/bin/hive-checkout-guard.sh /tmp/hive/discord bot.js — 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, because the leaf is where a new bot.js would appear and sticky only protects entries that already exist.
• bot.js exists, is a regular file (not a symlink), and is not group- or world-writable.

Plus NoNewPrivileges, ProtectSystem=full, ProtectHome=read-only, PrivateDevices, RestrictSUIDSGID.

Why a script and not an inline directive

The obvious one-liner is a no-op guard:

ExecStartPre=/usr/bin/find <dir> -maxdepth 0 -user dev ! -perm /go=w -print -quit

find exits 0 after a successful traversal whether or not anything matched, so the unsafe case exits 0 too and systemd starts the unit anyway. It would grep green. That is the same shape of dead assertion #5398 found in another contract test, so the guard's exit status is the assertion and the test executes it rather than grepping for it.

Why the path did not move

The issue's first recommendation was relocating the checkout. I did not, and the checkout stays at /tmp/hive. HIVE_REPO_DIR defaults there, but the literal /tmp/hive is also hardcoded in bin/hive.sh:27, throughout bin/kick-agents.sh (including the loop that reinstalls unit files from /tmp/hive/systemd/*), and in the ExecStart of hive-snapshot.service. Changing the default alone would leave those pointing at a path nothing pulls into. That is a deploy-layout decision across all of them — which is why the issue was filed as issue-only — not a fix to this unit. The guard closes the code-execution path without moving anything.

Upgrade behaviour: existing hosts keep their checkout exactly where it is, so nothing migrates and nothing breaks. The one real upgrade hazard is that hive-deploy.sh's two sync loops both skip files that are not already installed ([ -f "$dst" ] || continue), so a new helper is never bootstrapped — the same problem that needed an explicit block for hive-baseline-check.sh. Without handling it, an upgraded host would pull a unit whose ExecStartPre points at a script it does not have and the bot would fail to start. hive-deploy.sh therefore installs the guard explicitly, before kick-agents.sh reinstalls the units.

Two directives deliberately NOT set

PrivateTmp=true would give the unit a private /tmp namespace, hiding the very checkout WorkingDirectory points at — the service would fail to start. ProtectSystem=strict would make /tmp read-only for the same tree. Both are the intuitive "more hardening" choice here and both break the bot, so the contract test pins them as absent to stop a later hardening pass introducing them.

Testing

src/deploy/test_hive_discord_unit_contract.sh builds real directory trees shaped like the deployment (sticky world-writable ancestor standing in for /tmp) and runs the guard against each attack shape, asserting exit status: healthy checkout (must allow), post-reboot window with bot.js absent, world-writable leaf, sticky world-writable leaf, world- and group-writable ancestors, world- and group-writable bot.js, symlinked bot.js, symlinked checkout directory, missing directory.

• Against the unfixed unit: 3 failures (no ExecStartPre, no NoNewPrivileges, no ProtectSystem).
• Against the fixed unit: 21 passed, 0 failed.

Wired into v2-ci.yml next to the other deploy contract tests — src/deploy/test_*.sh are registered individually there, so an unregistered test would never run.

One portability bug was caught while writing this and is worth noting for anyone reusing the guard: BSD/macOS stat -f '%Lp' prints only the low three permission bits and drops the sticky bit, while GNU stat -c '%a' includes it. Inferring sticky from the digit count therefore reports every sticky directory as non-sticky and would reject /tmp itself. The guard reads sticky via test -k instead.

Overlap with #5436

#5436 (for #5434) touches the sibling unit systemd/ttyd-hive.service — no file overlap. It set no hardening-directive precedent to follow: it changed only a bind address (-i 127.0.0.1) and added no Protect*/NoNewPrivileges lines, and no unit in systemd/ currently carries any hardening directives. I followed its documentation posture instead — an inline comment explaining why the safe default is the default and what to do to change it — and confined the new directives to this unit rather than sweeping all eight, keeping the change reviewable and disjoint. A consistent hardening pass across the remaining units is worth a follow-up but is deliberately not in this PR.

Not verified

• Not exercised on a live host: no reboot-race reproduction against a real systemd, and the ExecStartPre path was not observed refusing a real start. The guard's logic is verified directly, but its integration with systemd is verified only by reading the unit.
• The guard's stat fallbacks were exercised on BSD stat locally; the GNU branch is exercised by CI on Linux, which is the platform that actually matters for these units.
• ProtectHome=read-only is safe as far as the bot's source shows (no writes to $HOME anywhere in discord/), but a runtime that writes an npm/cache path under home would notice it.

Fixes #5435

[clubanderson]

While scoping this I checked the other seven units in systemd/ for the same pattern. hive-snapshot.service is the only other one whose ExecStart resolves out of /tmp/hive (ExecStart=/tmp/hive/dashboard/publish-snapshot.sh, also User=dev), so it carries the same class of exposure.

I left it out of this PR deliberately to keep the change disjoint and reviewable, and filed it as #5483 instead. It is lower severity — Type=oneshot on a timer rather than Restart=always, and no EnvironmentFile, so the payoff is dev execution without an accompanying token. bin/hive-checkout-guard.sh as added here generalizes to it with no modification.

[kubestellar-prow]

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[github-actions]

Thank you for your contribution! Your PR has been merged.

Check out what's new:

• KubeStellar Console — Live multi-cluster dashboard
• Marketplace — Community extensions
• Knowledge Base — Troubleshooting and how-tos

Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey