**Tooling:** go1.26.0 windows/amd64; staticcheck (latest); go vet.
**Prior art:** controller `BUGHUNT.md` (2026-02-25, v0.30.3) read in full. NOT re-reported unless regressed. NOTE: BUGHUNT predates slice 8C which deleted ~12.3k LOC (internal/storage/*, restic, restore_drives, monitor/watchdog) — many BUGHUNT items (C1, C3, H9, M11-M20 partial, L8-L21 partial) reference now-deleted code and are MOOT. Agent has no BUGHUNT.
## Progress log
- 17:20 — Resumed: prior session left only a Phase-0 skeleton stale at v0.51.0 + a duplicate product commit. Reset audit branch onto current main (v0.58.0). Re-baselined.
- 17:50 — **Verified by hand**: CTRL-001 (import path traversal via manifest.AppName) — read restore.go:320-409 + manifest.go:36-42, confirmed no validator. AGENT-001 (inline wipe TOCTOU) — read disks.go:420-479, confirmed Format targets mutable req.Device.
- 17:55 — Wrote + ran failing evidence test `internal/appexport/traversal_audit_test.go` (CTRL-001). FAILS as expected. Committed Tier-1 findings (this checkpoint).
Both codebases are in good shape; the fail-safe/fail-destructive postures that matter most **hold**. No Critical found. The destructive-storage and operator-signature surfaces of the agent are unusually well-built (locked verify pipeline, hash-only token store, fsync-durable nonce store, narrow privileged fence, TLS pinning, fail-destructive ambiguity defaults — all confirmed). The controller's at-rest crypto (AES-256-GCM), restore data-key fail-closed gate, restic single-flight mutex, and (for the password-set path) full CSRF+auth coverage all hold.
The **one High** is a real path-traversal write primitive: the app-**import** path joins the attacker-controlled `manifest.AppName` from inside a `.fab` bundle straight into `filepath.Join`+`os.MkdirAll` with no validation (the archive-entry zip-slip guard exists, but the *stack-name* segment is unguarded). Verified by a failing evidence test. The remaining findings are Medium edge-cases (a TOCTOU between device-inspect and mkfs on the inline customer-confirmed wipe; decrypt-before-MAC writing transient unauthenticated plaintext) and a tail of Low/Info hardening items.
This session covered **Tier 1 in full** for both repos plus the localapi↔agentapi contract. Tier 2/3 (stacks deploy invariants, reconcile crash-safety, report↔hub contract, templates) are largely **not yet covered** — see §"What was NOT covered".
## Top-10 action list
| # | ID | Sev | Repo | Title | Effort |
|---|---|---|---|---|---|
| 1 | CTRL-001 | **High** | controller | App-import path traversal via unvalidated `manifest.AppName` | S |
Mechanism: `manifest.AppName` is read from the untrusted `manifest.json` inside an imported `.fab`. `UnmarshalManifest` only validates JSON, no segment check; no `IsValidStackName` exists in the package. `filepath.Join` cleans `..`, so `../../etc/cron.d/x` resolves outside `stacksDir`, then `os.MkdirAll` creates it and `restoreConfig`/`SaveEncryptedAppConfig` write `app.yaml`/`docker-compose.yml` there. The archive-entry zip-slip guard (restore.go:1047) does NOT cover this — it guards tar member names, not the stack-name segment.
Trigger: Import of a crafted `.fab`. The handler `/api/export/import` is behind RequireAuth+CSRF when a dashboard password is set, but on the demo (no password) BOTH are skipped → fully open. The `.fab` must sit under a registered `exports/` dir (`isValidExportPath` validates the file *location*, not its *contents*) — reachable via FileBrowser or a shared/malicious export.
Impact: Arbitrary-directory write as the controller process (config, restored app data, re-encrypted `app.yaml`) outside the stacks namespace — corruption/escape, potential clobber of controller config or writes onto mounted drives.
Fix sketch: In `UnmarshalManifest` (or immediately after) reject `AppName` unless it matches a strict single-segment allowlist (`^[a-z0-9][a-z0-9-]*$`, no `/ \ . ..`). Apply the same to `ConfigFiles`/`VolumeNames`/`HDDSubdirs` entries used in joins.
Verify: `cd controller && go test ./internal/appexport/ -run Traversal -v` → fails at this commit (parent-escape / deep-escape). Manual: craft a `.fab` with `"app_name":"../evil"`, import, observe `stackDir` outside `GetStacksBaseDir()`.
ifdec.Allowed{s.disks.Format(r.Context(),req.Device,req.FSType)}// FORMAT the same mutable path
```
Mechanism: The confirmed path inspects + derives + gate-binds the durable id, then formats `req.Device` (the raw `/dev` node) — never re-resolving durable→path immediately before mkfs. The durable id is *checked* but never *used as the wipe target*. A USB re-enumeration between derive and Format makes mkfs hit a different physical disk than the one confirmed. The signed-jobs `WipeExecutor` (signedjobs/wipe.go:74-99) deliberately does resolve→re-derive→re-inspect just before Format — proving the intended pattern this inline path violates.
Trigger: Customer-confirmed user-data wipe where `/dev/sdbN` is reassigned (hot-unplug/replug, udev churn, multi-USB hub) in the sub-second window.
Impact: mkfs on an unintended physical disk → data loss on the wrong drive, defeating "wipe X wipes exactly X" for the inline tier.
Fix sketch: After `AuthorizeWipe` Allowed, `storage.ResolveDurableDevice(deviceDurable)` → re-derive+match → re-inspect `DataBearing()` → Format the *resolved* path, not `req.Device` (mirror `WipeExecutor.Execute`).
Verify: localapi test that swaps the device behind `req.Device` between InspectDevice and Format; assert refuse-or-re-resolve.
### [AGENT-002] Blank-device format runs mkfs un-gated with a probe→mkfs TOCTOU
Mechanism: When the agent's own probe reads the device blank, it formats with no gate, no durable binding, on the mutable `req.Device`. "blank → benign" is correct in isolation, but a device blank at probe time can be replaced at the same node before mkfs. Same root cause as AGENT-001 (formatting a mutable path). `InspectDevice` itself is fail-safe (unprobed → data-bearing), so classification is sound; the residual risk is the path-vs-physical gap.
Trigger: `/dev/sdbN` reassigned between blank probe and mkfs.
Impact: mkfs on a now-data-bearing device that was never gated. Lower likelihood than AGENT-001 (no durable binding involved).
Fix sketch: Bind even the blank path to a durable id: derive at probe, resolve back immediately before Format, refuse on mismatch.
Verify: Test a device swapped blank→data-bearing between probe and format is not silently mkfs'd.
### [AGENT-003] `InspectDevice` swallows the `blkid` error; `lsblk` is the sole "probed" authority
iflerr==nil{probe.Probed=true;...}// lsblk is the ONLY thing that sets Probed
```
Mechanism: `blkid`'s exit/stderr is discarded; `Probed` is set solely from `lsblk` succeeding. blkid and lsblk cover different signature classes. If blkid errors transiently (busy/partial read) and returns empty while `lsblk -J` parses the device as having no fstype/pttype/children/mount, the probe is `Probed=true` with no positive evidence → `DataBearing()` false → reachable to the un-gated AGENT-002 format. This is exactly the "error in inspection → safe-to-wipe" pattern, narrowed by lsblk's real coverage of fstype/pttype/partitions/mount (so Medium, not Critical).
Trigger: blkid errors/empties while lsblk succeeds on a device whose data signature is blkid-only (some RAID/crypto members reported via blkid USAGE that lsblk's 4 columns miss).
Impact: A device with real data classified blank and formatted without a gate.
Fix sketch: Treat a blkid hard error (non-empty stderr / non-2 exit) as a probe failure; require both reads to complete before `Probed=true`, or fold blkid success into the Probed decision.
Verify: Unit test: blkid runner errors+empty, lsblk returns a clean blank-looking device → assert `Probed==false` (data-bearing).
### [CTRL-002] FAB decrypt streams plaintext to disk *before* verifying the HMAC tag
stream.XORKeyStream(decrypted,buf[:n]);out.Write(decrypted)// :203-204 plaintext to disk as it goes
...
if!hmac.Equal(mac.Sum(nil),storedMAC){os.Remove(outputPath);...}// :222-224 MAC checked only at the end
```
Mechanism: Construction is sound (AES-256-CTR + HMAC-SHA256 Encrypt-then-MAC, distinct scrypt-derived keys, constant-time `hmac.Equal`, random salt+IV per file). But decrypt writes full plaintext to `outputPath` and verifies the tag only afterward; cleanup is a best-effort `os.Remove` whose error is ignored. A same-key bit-flipped ciphertext yields controlled plaintext deltas that briefly land on disk before rejection; a crash between write and verify leaves unauthenticated attacker-influenced data behind. Violates "no unauthenticated plaintext is ever produced."
Trigger: Importing a tampered encrypted `.fab` (decrypted tgz written to a temp file before the tag check rejects).
Impact: Transient unauthenticated plaintext on disk; ignored `os.Remove` error can leave a partial decrypted file. Not a key/confidentiality break.
Fix sketch: Decrypt to temp → fsync → verify MAC → only then rename to `outputPath`; check the `os.Remove` error.
Verify: Flip one ciphertext byte, call `DecryptFile`; instrument to confirm bytes existed at the output path mid-call.
### [CTRL-005] Decompression-bomb: import extraction has no total-size / entry-count cap
Mechanism: Each gzip-inflated entry copied with unbounded `io.Copy`, no aggregate size or file-count limit, extracted to `os.MkdirTemp` on the ~8GB guest rootfs. Zip-slip guard is present (:1047), so availability-only.
Trigger: Importing an oversized/bomb `.fab`.
Impact: Fills guest rootfs during extraction → breaks controller + other apps until `defer os.RemoveAll(tmpDir)` runs. DoS.
Fix sketch: Track cumulative bytes vs a cap and vs `system.GetDiskUsage(tmpDir)` free; abort on exceed; cap entry count.
Mechanism: controller.yaml is correctly 0600, but settings.json — holding the dashboard bcrypt hash and the **plaintext** Hub retrieval password — is 0644. Any non-root UID or mounted-in app with read access to the data dir can read it.
Trigger: Any local read access to the data dir.
Impact: Disclosure of the retrieval password (re-pull customer config from Hub) and bcrypt hash (offline cracking). Defense-in-depth; de-privileged single-tenant container lowers exposure.
Fix sketch: Write settings.json 0600; optionally encrypt RetrievalPassword at rest with the existing AES key.
Verify: `stat -c %a settings.json` → 644.
### [CTRL-007] Pre-auth setup CSRF is a forgeable double-submit cookie
returncookie.Value==formToken// cookie==form, both attacker-controllable; no server secret/HMAC
```
Mechanism: Pure double-submit with no server-stored secret/HMAC and `HttpOnly:false`. An attacker who can set `felhom_csrf` on the victim (cookie injection over plain-HTTP `:8081`, or a same-site subdomain) can post a matching token to the pre-auth setup wizard.
Trigger: Controller in setup mode (`NeedsSetup`) + attacker can plant/predict the cookie.
Impact: Pre-auth CSRF on `/setup/manual`,`/setup/fresh` → set dashboard password, domain, git repo, Cloudflare tokens. Bounded to the one-time setup window.
Fix sketch: HMAC the token with a server secret, verify Origin/Referer on setup POSTs.
Verify: POST `/setup/manual` with matching cookie+form but no prior GET → accepted.
ifp.DurableID==""{return..."refusing an unbound decommission"}
d.intent.SetDecommissioned(p.DurableID)// any non-empty string accepted; no scheme check
```
Mechanism: Wipe requires the `byid:`/`byuuid:` scheme via `ResolveDurableDevice`; decommission accepts any non-empty string and writes it into the intent map. The watchdog keys on the *storage* scheme (`uuid:…`) at watchdog.go:178. A wrong-scheme id records an intent that never matches → decommission silently no-ops while reporting success.
Trigger: Signed decommission whose `durable_id` uses the device scheme (`byid:`) instead of the storage scheme.
Impact: Operator believes a drive is permanently decommissioned; watchdog still auto-mounts it. Requires operator id-format error; signature is valid.
Fix sketch: Validate `p.DurableID` carries the storage-scheme prefix before recording; reject unknown schemes (fail loud, not no-op).
Verify: Decommission with `byid:…` → should refuse.
### [CTRL-009] Login rate-limiter keys on spoofable `X-Forwarded-For`
Mechanism: Limit key derived from client-supplied XFF with no trusted-proxy allowlist; rotating XFF defeats the 5/min lock. bcrypt cost-10 is the real backstop.
Trigger: Brute-force `/login` with varying XFF.
Impact: Online password brute force (hardening gap, not an immediate break).
Fix sketch: Honor XFF only from a configured trusted-proxy CIDR; else key on `r.RemoteAddr`; add a global failed-login cap.
Verify: 5 failed logins with distinct XFF each reach bcrypt (no lock).
### [CTRL-011] Open redirect via protocol-relative `?next=//host`
gofunc(){if_,err:=r.RunOnce(context.Background());err!=nil{...}}()// no deadline → mkfs/pct can hang forever
```
Mechanism: ctx flows to `exec.CommandContext`; a wedged mkfs/pct never gets cancelled, and the single-flight `running` flag then short-circuits every later pass → the signed-jobs consumer stalls until restart.
Trigger: A destructive mkfs hangs (IO stall, DM hang).
Impact: Runner goroutine wedged; no further signed ops until restart.
Fix sketch: `context.WithTimeout` per pass in `OnEnvelope`, and/or per-op deadline around Format.
Verify: Inject a blocking Format; assert RunOnce returns within timeout.
### [AGENT-012] FileNonceStore loads expired nonces; `MemoryNonceStore` never evicts
fn(w,r,vmid)// handler ALWAYS receives the token's vmid
```
Mechanism: `withGuest` rejects a mismatching *query* vmid; a mismatching *body* vmid is caught separately by per-handler `scopedFromBody`. Self-scoping holds today on all 14 routes (handler always uses the token's vmid), but the invariant relies on per-handler discipline, not one chokepoint — a future route that reads an id from body/path and forgets `scopedFromBody` would regress it.
Trigger: New handler omitting the body-vmid check.
Impact: None today; latent cross-guest exposure on future routes.
Fix sketch: Centralize the body-vmid check, or assert handlers never read an id other than the wrapper's `vmid`.
Verify: Table test POSTing `{"vmid":<other>}` to every mutating route → expect 403.
### [AGENT-006] `wholeDiskOf` symlink-resolution failure falls back to raw string
...return"",false// unrecognized → caller treats as system (fail-safe)
```
Mechanism: Direction is fail-safe (unrecognized → system/protected). Drift: `SystemDisks` only adds a disk when `wholeDiskOf` succeeds; an EvalSymlinks failure on a system mount's device omits that OS disk from `sysDisks`, so a sibling user-data disk could flap. No unsafe verdict, but classification of user-data disks can be unstable when topology resolution is flaky.
Trigger: Transient `/dev` churn failing EvalSymlinks on a system mount during classification.
Impact: No unsafe wipe; possible user-data classification flapping.
Fix sketch: On EvalSymlinks error for a system mount, treat the whole `SystemDisks` result as `ok=false` (all candidates → system).
Verify: `SystemDisks` test with a `/boot` device that fails to resolve → assert `ok==false`.
---
## Findings — Info
### [CTRL-005b] DB-dump per-DB summary built then discarded (SA4010 ×4)
Severity: Info • Category: dead-code • Location: appexport/export.go:337 (eea235b)
`stepStart = time.Now()` reassigned but never read; encrypt step uses its own `encStart`. Cosmetic timing-log gap. Fix: remove the dead assignment or add the missing `time.Since(stepStart)` log.
### [CTRL-010] Setup writes config to a hardcoded path, ignoring resolved `Paths`
Severity: Info • Category: correctness • Location: setup/handlers.go:372 (eea235b)
`configPath := "/opt/docker/felhom-controller/controller.yaml"` hardcoded. On a deployment whose runtime config path differs, setup could write where the runtime never reads → `NeedsSetup` stays true, re-exposing the pre-auth wizard persistently. Fix: pass the resolved config path into `setup.NewServer`.
### [CTRL-012] `os.MkdirAll`/`os.Remove`/`Sync` errors swallowed in export staging
Severity: Info • Category: error-handling • Location: appexport/export.go:280,298,319,571,605 (eea235b)
Staging dir/remove errors ignored → less precise later failures. Critical bundle paths (tar, EncryptFile, final rename) do check errors. Fix: `failJob` on staging MkdirAll errors.
### [AGENT-005] `bindsToAction` is self-referential for one-shot jobs (defense-in-depth note)
Severity: Info • Category: invariant-drift • Location: signedjobs/runner.go:125-148; reconcile/gate.go:265-276 (d17b5ab)
For hub-queued jobs the gate compares the verified blob's params against an intent copied from the same blob → `bindsToAction` is tautologically true. By design (the executor's durable resolve+re-inspect is the real binding), but the gate adds no re-binding to the agent-surfaced `PendingOp`. The executor's data-bearing re-check is the sole backstop. Document explicitly or add a sanity assert of blob durable-id vs the host's current view.
### [AGENT-010] localapi `TokenStore.Lookup` constant-time compare is a self-comparison
Severity: Info • Category: security • Location: localapi/tokenstore.go:138-154 (d17b5ab)
The secret-bearing step is the `byHash[want]` map lookup (variable-time); the subsequent `subtle.ConstantTimeCompare(want, byVMID[vmid])` compares a value to itself (always 1) — no timing benefit. Not exploitable (SHA-256 of a 256-bit random token). Risk is documentary: don't claim a timing guarantee the code doesn't provide. Hash-only invariant still HOLDS.
### [AGENT-011] Host-wide metrics/disk topology served to any guest token
Severity: Info • Category: security • Location: localapi/host_metrics.go:28-49, disks.go:109-147 (d17b5ab)
`/host/metrics` and `/disks` return host-wide CPU/temp/SMART + storage targets to any authenticated guest, by design (one-customer-per-host model, documented inline). Revisit only if multi-tenancy is ever introduced.
Severity: Info • Category: security • Location: proxmox/mutate.go:105-115 (d17b5ab)
`DestroyLXC` always passes `force=1&purge=1&destroy-unreferenced-disks=1` with no internal vmid/scratch guard; safety depends entirely on the upstream `reconcile.Gate`. The only destructive consumer (signed-jobs runner) routes through the gate (verified). Add a routing test asserting no non-gated caller reaches it.
### [AGENT-008] (positive) Fail-destructive defaults are correctly wired end-to-end
Confirmed compensating control: unprobed device → data-bearing; `NoopHostOps` → every device data-bearing; unknown op class → Destructive; nil verifier → refuse `pending_signature`; role default → system. The dangerous direction (ambiguity→allow-wipe) is the default nowhere. This downgrades AGENT-002/003 from Critical to Medium.
- Privileged ops narrow + TLS pinned — **HOLDS** @ proxmox/privileged.go:11-178 (3 fenced exceptions, no shell), tls.go:33-75 (CAFile | leaf-SHA256 pin; InsecureSkipVerify off by default). Blanket primitive AGENT-013 gated upstream.
- Signed-job sig verified before destructive op + nonce-replay prevented — **HOLDS** @ verifier.go:99-181 (verify over raw bytes, allow-list by key material), noncestore.go:114-135 (fsync-durable, fail-safe). Growth note AGENT-012.
- No `pct exec` in provision back-half / bootstrap.json chown 100000:100000 — **NOT verified this session** (Tier 2, provision pkg).
- No secrets in logs — **HOLDS** (token "Never logged"; escrow logs only short FPs; pty discards passphrase echo).
- **Durable-resolve-before-mutate helper (agent):** AGENT-001/002 + AGENT-007 all stem from formatting/recording a device by a *path/string* instead of re-resolving the *durable id* at the moment of action. A single `resolveAndReinspect(durableID) (path, probe, error)` used by both the inline localapi format path and `WipeExecutor` would make "act on exactly the confirmed device" structural.
- **Single path-segment validator (controller):** CTRL-001 shows `manifest.AppName` and friends reach `filepath.Join` unvalidated while API stack routes have `extractName`. Extract one `ValidateStackName` and apply at every untrusted boundary (import manifest, archive subdir names, export filenames).
- **Decrypt-then-rename pattern (controller):** CTRL-002 — the export side already does tmp→rename; the decrypt side should too. Centralize an `atomicDecrypt(verify, then rename)`.
- (Carried from BUGHUNT, still relevant where code survived 8C) atomic-write helper unification; injected-logger consistency (`crypto.DecryptMap` global log).
## Test-gap analysis
- **appexport import path** — ZERO tests in the package (no `_test.go` existed before this audit's evidence test). The highest-severity finding (CTRL-001) lived in untested code. Import/restore/crypto-decrypt all lack unit coverage.
- **agent inline format/wipe path** (localapi/disks.go format handler) — needs a swap-device-between-inspect-and-format test (would catch AGENT-001/002).
- **storage classify under inspection error** — no test for blkid-error/lsblk-success (AGENT-003).
- **Assumption:** "one customer per host" deployment model holds (makes AGENT-011 Info not Medium). Documented in arch doc 03.
- **Assumption:** AGENT-001/002 trigger (USB `/dev` re-enumeration in the inspect→mkfs window) is rare on these N100 boxes → rated Medium not High; raise to High if USB churn proves common.
- gofmt noise = CRLF/autocrlf artifact, not a finding.
- Two controller Tier-1 auditors independently numbered findings CTRL-001..; merged + renumbered here into one namespace.
- **Full mutating-route × CSRF/auth coverage table** (password-set path) was produced and shows no uncovered mutating runtime route; available on request — every `/api/*` + `/` subtree wrapped in RequireAuth+CsrfProtect (main.go:703-728); only `/api/health` (GET) and `/api/host-metrics` (GET) are CSRF-exempt by nature.
## What was NOT covered (defines next session)
Tier 1 is complete for both repos + the localapi route walk. NOT yet done:
t.Fatalf("CTRL-001: UnmarshalManifest returned a traversal AppName %q with no rejection; "+
"a sanitizer (single safe path segment, allowlist) is missing",m.AppName)
}
}
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
admin