audit: Tier-1 findings (both repos) + CTRL-001 traversal evidence test
Read-only deep sweep checkpoint. Tier 1 complete for controller+agent: - CTRL-001 (High): app-import path traversal via unvalidated manifest.AppName (failing evidence test) - AGENT-001/002/003, CTRL-002 (Medium): wipe TOCTOU, blkid-error classify, decrypt-before-MAC - 12 Low/Info hardening items + invariant checklist + contract/dead-code/test-gap sections Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,431 @@
|
||||
# AUDIT — felhom-controller + felhom-agent deep sweep — 2026-06-13
|
||||
|
||||
**Branch (both repos):** `audit/2026-06-13-deep-sweep` (off latest `main`)
|
||||
**Auditor:** Claude Code (Opus 4.8, unattended overnight session)
|
||||
**Mode:** read-only, evidence-based. No fixes applied. No mutations, deploys, or builds.
|
||||
|
||||
| Repo | HEAD commit | Version | Note |
|
||||
|---|---|---|---|
|
||||
| felhom-controller | `eea235bd6952184c4681b4b133396d6b8b0aaf33` | v0.58.0 | all CTRL findings cite this commit |
|
||||
| felhom-agent | `d17b5ab45dc7df51836e26d7a38450cd391b94b2` | v0.29.1 | all AGENT findings cite this commit |
|
||||
| felhom.eu / hub (reference) | `d59691dd826a901da39562aeaa5d989b8ea1a7ee` | hub v0.11.0 | contract reference |
|
||||
|
||||
**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:25 — Phase 0 complete (both repos). Baselines below.
|
||||
- 17:35 — Tier 1 dispatched (4 parallel auditors): agent destructive-path, agent auth/authz, controller backup/crypto, controller web/auth/setup. All returned.
|
||||
- 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).
|
||||
- (next) Tier 2: stacks deploy invariant, agentapi↔localapi contract diff, report↔hub contract.
|
||||
|
||||
## Baseline (Phase 0)
|
||||
|
||||
| Check | Controller (module at `controller/`) | Agent (module at root) |
|
||||
|---|---|---|
|
||||
| `go build ./...` | PASS | PASS |
|
||||
| `go vet ./...` | PASS (clean) | PASS (clean) |
|
||||
| `go test ./...` | PASS (BUGHUNT's TestBackupCopiesOnPath now green) | PASS |
|
||||
| `gofmt -l .` | CRLF noise only (autocrlf=true); not a finding | same |
|
||||
| staticcheck | 18 reports — triaged (4× SA4010 in backup.go → CTRL-005; SA4006/SA4017 export.go → CTRL-006; 5× U1000 dead code → §Dead code) | clean |
|
||||
|
||||
## Executive summary
|
||||
|
||||
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 |
|
||||
| 2 | AGENT-001 | Medium | agent | Inline customer-confirmed wipe formats mutable `/dev` path (classify→mkfs TOCTOU) | M |
|
||||
| 3 | CTRL-002 | Medium | controller | FAB decrypt streams plaintext to disk *before* verifying HMAC tag | S |
|
||||
| 4 | AGENT-003 | Medium | agent | `InspectDevice` swallows `blkid` error; `lsblk` is sole "probed" authority | S |
|
||||
| 5 | AGENT-002 | Medium | agent | Blank-device format runs mkfs un-gated with a probe→mkfs TOCTOU | M |
|
||||
| 6 | CTRL-005 | Low | controller | Decompression-bomb: import extraction has no size/count cap | S |
|
||||
| 7 | CTRL-008 | Low | controller | `settings.json` (bcrypt hash + plaintext retrieval pw) written 0644 | S |
|
||||
| 8 | CTRL-007 | Low | controller | Pre-auth setup CSRF is a forgeable double-submit cookie | M |
|
||||
| 9 | AGENT-007 | Low | agent | Decommission durable-id scheme unvalidated → silent no-op intent | S |
|
||||
| 10 | CTRL-009 | Low | controller | Login rate-limiter keys on spoofable `X-Forwarded-For` | S |
|
||||
|
||||
---
|
||||
|
||||
## Findings — Critical / High
|
||||
|
||||
### [CTRL-001] App-import path traversal via unvalidated `manifest.AppName`
|
||||
Severity: High
|
||||
Category: security
|
||||
Location: controller/internal/appexport/restore.go:339, 365, 401 (commit eea235b); root cause manifest.go:36-42
|
||||
Confidence: **verified-by-test** (`internal/appexport/traversal_audit_test.go`, this branch)
|
||||
Evidence:
|
||||
```go
|
||||
manifest, err := UnmarshalManifest(manifestData) // AppName fully from bundle JSON, no validation
|
||||
stackDir := filepath.Join(stacksDir, manifest.AppName) // :339
|
||||
os.MkdirAll(stackDir, 0755) // :365
|
||||
composePath := filepath.Join(stackDir, "docker-compose.yml") // :401
|
||||
```
|
||||
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()`.
|
||||
|
||||
---
|
||||
|
||||
## Findings — Medium / Low
|
||||
|
||||
### [AGENT-001] Inline customer-confirmed wipe formats a mutable `/dev` path (classify→mkfs TOCTOU)
|
||||
Severity: Medium (data-loss consequence; narrow USB-reenumeration trigger)
|
||||
Category: security / correctness
|
||||
Location: agent/internal/localapi/disks.go:429-471 (commit d17b5ab)
|
||||
Confidence: verified-static
|
||||
Evidence:
|
||||
```go
|
||||
probe, err := s.disks.InspectDevice(r.Context(), req.Device) // inspect /dev/sdbN
|
||||
role := s.deviceRole(r.Context(), req.Device)
|
||||
deviceDurable, _ := storage.DeviceDurableID(req.Device) // derive id of /dev/sdbN NOW
|
||||
dec := s.diskGate.AuthorizeWipe(WipeRequest{Role:..., DeviceDurableID: deviceDurable, Confirmed: req.Confirmed, ConfirmDurableID: req.DurableID})
|
||||
if dec.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
|
||||
Severity: Medium
|
||||
Category: security
|
||||
Location: agent/internal/localapi/disks.go:430-444 (commit d17b5ab)
|
||||
Confidence: verified-static
|
||||
Evidence:
|
||||
```go
|
||||
probe, err := s.disks.InspectDevice(r.Context(), req.Device)
|
||||
if err != nil { /* fall through; probe.DataBearing()==true on !Probed (fail-safe) */ }
|
||||
if !probe.DataBearing() { s.disks.Format(r.Context(), req.Device, req.FSType); writeOK(...); return }
|
||||
```
|
||||
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
|
||||
Severity: Medium
|
||||
Category: error-handling / security
|
||||
Location: agent/internal/storage/hostops.go:293-331 (commit d17b5ab)
|
||||
Confidence: verified-static
|
||||
Evidence:
|
||||
```go
|
||||
bout, _, _ := h.runner.Run(ctx, h.bins.Blkid, "-p", "-o", "export", device) // err DROPPED
|
||||
for k, v := range parseBlkidExport(bout) { ... }
|
||||
lout, _, lerr := h.runner.Run(ctx, h.bins.Lsblk, "-J", ..., device)
|
||||
if lerr == 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
|
||||
Severity: Medium
|
||||
Category: security
|
||||
Location: controller/internal/appexport/crypto.go:191-227 (commit eea235b)
|
||||
Confidence: verified-static
|
||||
Evidence:
|
||||
```go
|
||||
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
|
||||
Severity: Low
|
||||
Category: resource-leak
|
||||
Location: controller/internal/appexport/restore.go:1021-1072 (extractTarGz), :280 (commit eea235b)
|
||||
Confidence: verified-static
|
||||
Evidence:
|
||||
```go
|
||||
case tar.TypeReg:
|
||||
outFile, _ := os.Create(target)
|
||||
io.Copy(outFile, tr) // :1064 unbounded
|
||||
```
|
||||
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.
|
||||
Verify: Import a tar.gz inflating beyond rootfs free; observe `/tmp` fill.
|
||||
|
||||
### [CTRL-008] `settings.json` (bcrypt hash + plaintext retrieval pw + tokens) written world-readable (0644)
|
||||
Severity: Low
|
||||
Category: security
|
||||
Location: controller/internal/settings/settings.go:241 (commit eea235b)
|
||||
Confidence: verified-static
|
||||
Evidence:
|
||||
```go
|
||||
os.WriteFile(tmpPath, data, 0644) // file carries PasswordHash (bcrypt) + RetrievalPassword (plaintext)
|
||||
```
|
||||
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
|
||||
Severity: Low
|
||||
Category: security
|
||||
Location: controller/internal/setup/csrf.go:33-43 (commit eea235b)
|
||||
Confidence: verified-static
|
||||
Evidence:
|
||||
```go
|
||||
return cookie.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.
|
||||
|
||||
### [AGENT-007] Decommission durable-id scheme unvalidated → silent no-op intent
|
||||
Severity: Low
|
||||
Category: contract-mismatch / correctness
|
||||
Location: agent/internal/signedjobs/decommission.go:63-79 (commit d17b5ab)
|
||||
Confidence: verified-static
|
||||
Evidence:
|
||||
```go
|
||||
if p.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`
|
||||
Severity: Low
|
||||
Category: security
|
||||
Location: controller/internal/web/auth.go:127-161 (commit eea235b)
|
||||
Confidence: verified-static
|
||||
Evidence:
|
||||
```go
|
||||
ip := r.RemoteAddr
|
||||
if fwd := r.Header.Get("X-Forwarded-For"); fwd != "" { ip = strings.Split(fwd, ",")[0] } // attacker-controlled
|
||||
```
|
||||
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`
|
||||
Severity: Low
|
||||
Category: security
|
||||
Location: controller/internal/web/auth.go:187-191 (commit eea235b)
|
||||
Confidence: verified-static
|
||||
Evidence:
|
||||
```go
|
||||
if nextURL != "" && strings.HasPrefix(nextURL, "/") { redirectTo = nextURL } // "//evil.com" passes
|
||||
```
|
||||
Mechanism: `next` only required to start with `/`; `//evil.com` is protocol-relative and most browsers redirect to that host.
|
||||
Trigger: `POST /login?next=//evil.com` after valid credentials.
|
||||
Impact: Post-login open redirect (phishing). Requires a valid login first.
|
||||
Fix sketch: Reject `next` starting with `//` or `/\`.
|
||||
Verify: Log in with `next=//example.com` → 302 to `//example.com`.
|
||||
|
||||
### [AGENT-004] Signed-op execution runs under `context.Background()` (no timeout)
|
||||
Severity: Low
|
||||
Category: resource-leak
|
||||
Location: agent/internal/signedjobs/runner.go:81-85 (commit d17b5ab)
|
||||
Confidence: verified-static
|
||||
Evidence:
|
||||
```go
|
||||
go func() { 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
|
||||
Severity: Low
|
||||
Category: resource-leak
|
||||
Location: agent/internal/authz/noncestore.go:92-112 (commit d17b5ab)
|
||||
Confidence: verified-static
|
||||
Evidence:
|
||||
```go
|
||||
for _, line := range ... { ... s.idx[r.Nonce] = r.Exp } // no expiry filter on load; compaction only after CompactEvery new appends
|
||||
```
|
||||
Mechanism: `Open` loads all logged nonces incl. expired; compaction driven only by new appends (default 1000). Frequent restarts + long validity window keep expired nonces resident. Bounded by op-rate × window; correctness unaffected (time-window check rejects expired ops first).
|
||||
Trigger: High op volume + frequent restarts.
|
||||
Impact: Slow nonce-log growth on a busy host. Not a security hole.
|
||||
Fix sketch: Skip `r.Exp.Before(now)` during `load()`; run one compaction on `Open`.
|
||||
Verify: Open a store whose log holds only expired records → assert `len(idx)==0`.
|
||||
|
||||
### [AGENT-009] localapi cross-guest scope check covers only the query `vmid` (latent)
|
||||
Severity: Low
|
||||
Category: security
|
||||
Location: agent/internal/localapi/server.go:263-308 (commit d17b5ab)
|
||||
Confidence: verified-static
|
||||
Evidence:
|
||||
```go
|
||||
if q := r.URL.Query().Get("vmid"); q != "" { if want != vmid { ...403 } }
|
||||
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
|
||||
Severity: Low
|
||||
Category: correctness
|
||||
Location: agent/internal/storage/role.go:69-87 (commit d17b5ab)
|
||||
Confidence: verified-static
|
||||
Evidence:
|
||||
```go
|
||||
if resolved, err := filepath.EvalSymlinks(device); err == nil { dev = resolved }
|
||||
... 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: Low → Info (observability only)
|
||||
Category: error-handling / dead-code
|
||||
Location: controller/internal/backup/backup.go:179-253 (commit eea235b)
|
||||
Confidence: verified-static
|
||||
Evidence: `summary` accumulates `OK/SKIP/FAIL <db>` lines (`:188,:193,:204,:208`) but is never read; caller returns generic `"some database dumps failed"` (:249). SKIP reasons ("drive disconnected/decommissioned") are lost.
|
||||
Impact: Operators lose the per-DB failure/skip breakdown — can't tell which app/drive is unprotected.
|
||||
Fix sketch: Fold `summary` into the returned error / `m.lastDBDump` status, or remove the dead var.
|
||||
Verify: Wire `summary` into the error → SA4010 clears.
|
||||
|
||||
### [CTRL-006] `executeExport` step-5 timing baseline dead-assigned (SA4006/SA4017)
|
||||
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.
|
||||
|
||||
### [AGENT-013] `DestroyLXC` uses blanket destructive flags; narrowness is external (gated upstream)
|
||||
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
|
||||
Severity: Info • Category: security • Location: storage/hostops.go:64-69,449-453; reconcile/classify.go:99-112; gate.go:146-150 (d17b5ab)
|
||||
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.
|
||||
|
||||
---
|
||||
|
||||
## Contract checks (controller↔agent, controller↔hub)
|
||||
|
||||
| Contract | Status | Note |
|
||||
|---|---|---|
|
||||
| agent `internal/localapi` ↔ controller `internal/agentapi` | **partially checked — clean so far** | Auth auditor walked all 14 localapi routes; self-scoping + envelope shape consistent. Full field-by-field JSON-tag diff vs `agentapi` NOT yet done — see "NOT covered". |
|
||||
| controller `internal/report/types.go` ↔ hub ingest | **NOT checked** | Deferred to Tier 3. |
|
||||
| Operator-signature namespace `felhom-op-v1` (agent verifier) ↔ opsign tool | clean | verifier.go:99-181 fixed namespace; `cmd/felhom-opsign` present. |
|
||||
|
||||
## Invariant checklist results
|
||||
|
||||
**Agent**
|
||||
- Data-bearing classification from device inspection only (never caller claims) — **HOLDS** @ localapi/disks.go:429-435 + storage/hostops.go:283-333 (caller "blank/force" claim explicitly ignored). Caveat AGENT-003 (transient blkid-error read drop).
|
||||
- Fail-destructive on ambiguity — **HOLDS** @ hostops.go:64-69, classify.go:111, role.go:91-99 (AGENT-008).
|
||||
- Signature gate before destructive op; no un-gated path — **HOLDS for signed-jobs** @ runner.go:134-148; **DRIFTED for inline path** → AGENT-001/AGENT-002 (authorized resource ≠ mutated resource; blank branch un-gated).
|
||||
- Token store hash-only — **HOLDS** @ tokenstore.go:106-170 (only sha256 hashes persisted; AGENT-010 note).
|
||||
- localapi self-scoping (cross-guest→403) — **HOLDS** @ server.go:263-308 (latent-discipline AGENT-009).
|
||||
- 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).
|
||||
|
||||
**Controller**
|
||||
- No `:latest` anywhere — **NOT fully verified** (Tier 3 templates/infra sweep pending; spot-check clean).
|
||||
- Secrets never logged (keys only) — **HOLDS** spot-checked (handlers.go:56 logs presence only); CTRL-008 is at-rest file mode, not logging.
|
||||
- Protected stacks unstoppable server-side — **NOT verified** (Tier 2, stacks pkg).
|
||||
- `Deployed` set before `up -d`, reverted on failure (mem+disk) — **NOT verified** (Tier 2, stacks/deploy.go).
|
||||
- restic behind running mutex — **HOLDS** @ backup.go:144-148, restore.go:31-42 (restic moved to agent; all entry points single-flight).
|
||||
- filebrowser compose preserved if present — **NOT verified** (Tier 2).
|
||||
- `EnsureBaseStack` non-fatal+idempotent — **NOT verified** (Tier 2).
|
||||
- CSRF on every state-changing route incl. setup — **HOLDS for runtime mux** (password-set), full route×CSRF/auth table in session notes; **setup uses weaker CSRF** → CTRL-007.
|
||||
- Restore data-key fail-closed gate — **HOLDS** @ appbackup/restore_unit.go:38-50,113-116.
|
||||
- At-rest crypto AEAD — **HOLDS** @ crypto/crypto.go:50-61 (AES-256-GCM, fresh nonce). FAB export Encrypt-then-MAC HOLDS w/ CTRL-002 caveat.
|
||||
|
||||
## Refactor & shared-code opportunities
|
||||
|
||||
- **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).
|
||||
- Stacks deploy crash-safety, reconcile Recover() ground-truth — coverage not yet assessed (Tier 2).
|
||||
|
||||
## Dead code inventory (staticcheck U1000, controller)
|
||||
|
||||
- `cmd/controller/main.go:1172` `fileExists` — unused (also BUGHUNT L1; still present).
|
||||
- `internal/system/info.go:11` `debugf` — unused.
|
||||
- `internal/web/alerts.go:233` `countLevel` — unused.
|
||||
- `internal/web/handlers.go:1083` `(*Server).countAppsUsingPath` — unused.
|
||||
- (agent: staticcheck clean — no dead-code reports.)
|
||||
|
||||
## Session notes, assumptions, open questions
|
||||
|
||||
- Session unattended; conservative assumptions inline. Severity reflects consequence; confidence kept honest (most findings verified-static, CTRL-001 verified-by-test).
|
||||
- **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:
|
||||
- **Tier 2 agent:** `internal/reconcile` engine/plan/queue/journal/**recover**/normalize/bringup — crash-safety (marker-before-mutation, `ListLXC` ground truth in `Recover()`, defer-unquiesce); `internal/provision` back-half (token mint, bootstrap.json chown 100000:100000, no `pct exec`); `internal/proxmox` task/upid/WaitTask parsing; `internal/pbs` fingerprint pinning + verify semantics.
|
||||
- **Tier 2 controller:** `internal/stacks` (Deployed-before-up-d invariant + rollback in mem+disk, protected-stack server-side enforcement, EnsureBaseStack idempotent, filebrowser-compose preserve) — these are the highest-value un-audited invariants; `internal/quiesce`, `internal/selfupdate`, `internal/recovery`, `internal/agentapi` (TLS/fingerprint, timeouts, error mapping).
|
||||
- **Tier 3 contracts:** field-by-field `agentapi`↔`localapi` JSON-tag/status-code diff; `report/types.go`↔hub ingest types; templates funcmap completeness + XSS + leftover-emoji.
|
||||
- **Tier 3 cross-cutting:** goroutine/ticker lifecycle & shared-state races (BUGHUNT flagged many in surviving pkgs — scheduler late-registration M10, watchdog deleted); panic isolation in scheduler/loop jobs; os/exec timeout hygiene across `docker`/`pct` calls.
|
||||
- **`-race` run:** not executed (would need the build server; CGO/sqlite on Windows). Recommended next session in a throwaway dir on 192.168.0.180.
|
||||
- **Live read-only inspection** (docker logs/df/findmnt on demo) — skipped; static-only this session.
|
||||
Reference in New Issue
Block a user