|
| 1 | +# FIX_PLAN.md |
| 2 | + |
| 3 | +Remediation plan for bugs and security vulnerabilities identified in the |
| 4 | +security/correctness audit (2026-06-10). Findings are ordered by priority. |
| 5 | +Each entry lists the location, the flaw, the concrete fix, and verification |
| 6 | +steps. Check items off as they land. |
| 7 | + |
| 8 | +## Priority order (fix these first) |
| 9 | + |
| 10 | +1. **#1** — Telegram document path traversal (remote arbitrary-write/RCE). ✅ Fixed |
| 11 | +2. **#3** — Fail-open Telegram authorization (amplifies #1). ✅ Fixed |
| 12 | +3. **#2** — `trustAll` never resets within a run (one click unlocks the turn). ✅ Fixed |
| 13 | +4. **#4 / #5** — WS auth + serve-mode approval deadlock. |
| 14 | + |
| 15 | +> Status: #1, #2, and #3 are implemented and tested in this PR. The remaining |
| 16 | +> items are open. #4 and #6 involve a behavior/default change (WS token) and are |
| 17 | +> left for maintainer direction. |
| 18 | +
|
| 19 | +--- |
| 20 | + |
| 21 | +## 1. Path traversal → arbitrary file write via Telegram document filename |
| 22 | +- **Status:** ✅ Fixed — `sanitizeDocName` strips directory components; tests in |
| 23 | + `download_test.go` (`TestSanitizeDocName`, `TestDownloadDocument_NoTraversal`). |
| 24 | +- **Severity:** Critical |
| 25 | +- **Location:** `internal/telegram/download.go:156-164` |
| 26 | +- **Flaw:** `DownloadDocument` uses the sender-controlled `Document.FileName` |
| 27 | + directly: `safeName := fileName; localPath := filepath.Join(dir, safeName)`. |
| 28 | + `filepath.Join` collapses `../`, so a document named |
| 29 | + `../../../home/<user>/.odek/config.json` (or `../../.ssh/authorized_keys`) |
| 30 | + writes outside `~/.odek/media/`. Both path and contents are attacker- |
| 31 | + controlled → RCE/persistence. Photo/voice paths are safe (hashed names). |
| 32 | +- **Fix:** |
| 33 | + - `safeName = filepath.Base(filepath.Clean(fileName))`. |
| 34 | + - Reject if the result is empty, `.`, `..`, or still contains a path |
| 35 | + separator; fall back to the generated `doc_<fileID>` name. |
| 36 | + - Confirm the final `localPath` is still within `dir` (defense in depth): |
| 37 | + resolve both with `filepath.Abs` and check `strings.HasPrefix`. |
| 38 | +- **Verify:** unit test in `internal/telegram` sending a `FileName` of |
| 39 | + `../../evil.txt` and asserting the write stays under `~/.odek/media/`. |
| 40 | + |
| 41 | +## 2. Approval bypass: `trustAll` never resets within a run |
| 42 | +- **Status:** ✅ Fixed — grant is now reset at the end of each iteration's tool |
| 43 | + phase (not via `defer`). Regression test: |
| 44 | + `TestBatchApprovalTrustAllNotLeakedAcrossIterations` in `loop_test.go`. |
| 45 | +- **Severity:** High |
| 46 | +- **Location:** `internal/loop/loop.go:885-891` |
| 47 | +- **Flaw:** `defer ta.SetTrustAll(false)` sits inside the |
| 48 | + `for i := 0; i < e.maxIter` loop, so it fires at function return, not at |
| 49 | + iteration end. After one approved batch, all later dangerous ops auto-approve |
| 50 | + for the rest of the turn (incl. destructive/blocked classes). |
| 51 | +- **Fix:** scope the reset to the iteration — set `trustAll` true before tool |
| 52 | + execution and `false` immediately after (e.g. wrap the phase in a closure, or |
| 53 | + add an explicit `ta.SetTrustAll(false)` at the end of the iteration body). |
| 54 | + Do not rely on `defer` inside the loop. |
| 55 | +- **Verify:** test that a second dangerous tool call in a later iteration of the |
| 56 | + same run still triggers `PromptCommand`. |
| 57 | + |
| 58 | +## 3. Fail-open Telegram authorization |
| 59 | +- **Status:** ✅ Fixed — `ValidateConfig` now refuses to start with no allowlist |
| 60 | + unless `ODEK_TELEGRAM_ALLOW_ALL=true` is set; `isAllowed` is fail-closed |
| 61 | + (empty lists + no opt-in → deny); startup logs a loud warning when running |
| 62 | + open. **Also closed a callback-query authorization bypass** found while |
| 63 | + challenging the fix: `handleCallback` (inline-button presses) did not call |
| 64 | + `isAllowed` — it now does, so callbacks are gated like messages. Tests: |
| 65 | + `TestValidateConfig_noAllowlistFailsClosed`, `TestValidateConfig_allowAllOptIn`, |
| 66 | + `TestConfigFromEnv_allowAll`, `TestIsAllowed_EmptyAllowlist` (now asserts deny), |
| 67 | + `TestIsAllowed_EmptyAllowlistWithAllowAll`, `TestHandleCallback_RespectsAllowlist`, |
| 68 | + `TestHandleUpdate_CallbackQueryNotAllowed` (now asserts deny). |
| 69 | +- **Note (not fixed — pre-existing, lower priority):** in a *group* chat where |
| 70 | + only `AllowedChats` is set, any group member can press another user's |
| 71 | + approval/clarify buttons (callback passes the chat-level allowlist). Tightening |
| 72 | + this requires binding an approval to the specific user who triggered the run — |
| 73 | + out of scope for this PR. |
| 74 | +- **Severity:** High |
| 75 | +- **Location:** `internal/telegram/handler.go:505-533`; defaults in |
| 76 | + `internal/telegram/config.go` |
| 77 | +- **Flaw:** `isAllowed` returns `true` when both `AllowedChats` and |
| 78 | + `AllowedUsers` are empty, and the default config leaves both empty with no |
| 79 | + warning. Anyone who finds the bot gets full (possibly godmode) access. |
| 80 | +- **Fix:** make the bot fail-closed — if no allow-list is configured, either |
| 81 | + refuse to start (log a fatal/clear error) or run with tools disabled. |
| 82 | + Emit a loud startup warning when the allow-list is empty. |
| 83 | +- **Verify:** start the bot with no `ODEK_TELEGRAM_ALLOWED_*` env vars and |
| 84 | + confirm it refuses / restricts rather than serving all users. |
| 85 | + |
| 86 | +## 4. WebSocket `/ws` (and `/api/*`) has no authentication |
| 87 | +- **Severity:** High |
| 88 | +- **Location:** `cmd/odek/serve.go:148,874-888` (and the `/api/*` handlers |
| 89 | + ~904-1057) |
| 90 | +- **Flaw:** the only gate is an Origin check that returns `nil` for empty |
| 91 | + Origin, so any non-browser client (no Origin header) gets full agent control. |
| 92 | + `/api/*` handlers have no Origin check at all. Safety depends solely on the |
| 93 | + loopback bind; `--addr 0.0.0.0` removes it. |
| 94 | +- **Fix:** |
| 95 | + - Add a per-process bearer/session token required on the WS handshake and all |
| 96 | + `/api/*` requests (generate on startup, print to the operator). |
| 97 | + - Reject non-loopback `RemoteAddr` unless a token is explicitly configured. |
| 98 | + - Apply the same origin/token gate to every `/api/*` handler. |
| 99 | +- **Verify:** connect with `websocat` (no Origin) and confirm rejection without |
| 100 | + a token; confirm `--addr 0.0.0.0` still requires the token. |
| 101 | + |
| 102 | +## 5. Web UI approvals deadlock and always time out |
| 103 | +- **Severity:** High |
| 104 | +- **Location:** `cmd/odek/serve.go:494-590`; `cmd/odek/wsapprover.go:153-184` |
| 105 | +- **Flaw:** `handleWS` is the only goroutine reading the socket and calls |
| 106 | + `RunWithMessages` synchronously. The approver blocks on `HandleResponse`, |
| 107 | + which is only called from that same (now-blocked) receive loop, so |
| 108 | + `approval_response` is never read → 60s timeout → denial. Approval UI in serve |
| 109 | + mode is dead. |
| 110 | +- **Fix:** read the WebSocket on a dedicated goroutine (channel-feed the receive |
| 111 | + loop), or run the prompt on a separate goroutine from the socket reader so |
| 112 | + responses can be processed while a prompt is in flight. |
| 113 | +- **Verify:** in serve mode, trigger a dangerous tool and confirm the browser |
| 114 | + approval is received and honored without timing out. |
| 115 | + |
| 116 | +## 6. `write_file`/`patch` `~/.odek/` carve-out enables privilege escalation |
| 117 | +- **Severity:** High |
| 118 | +- **Location:** `cmd/odek/file_tool.go:760-768`; |
| 119 | + `internal/danger/classifier.go:177-186`; `patch` wiring in `cmd/odek/main.go` |
| 120 | +- **Flaw:** `confineToCWD` allows any path under `~/.odek/`, and |
| 121 | + `~/.odek/config.json` classifies as auto-allowed `LocalWrite`. A confined/ |
| 122 | + untrusted sub-agent can rewrite its own config to disable the sandbox / enable |
| 123 | + YOLO on the next run, or drop an auto-loaded `SKILL.md`. Shell rc files |
| 124 | + (`~/.bashrc`, `~/.zshrc`, `~/.profile`) also fall through to `LocalWrite`. |
| 125 | + `patch` is created without `restrictToCWD`. |
| 126 | +- **Fix:** |
| 127 | + - Classify `~/.odek/config.json`, `~/.odek/skills/`, and the shell rc/profile |
| 128 | + files (and crontab paths) as `SystemWrite` (prompt/deny). |
| 129 | + - Exclude those paths from the `confineToCWD` carve-out. |
| 130 | + - Give `patch` the same `restrictToCWD` confinement as `write_file`. |
| 131 | +- **Verify:** confined agent attempt to write `~/.odek/config.json` and |
| 132 | + `~/.bashrc` must hit an approval/deny path, not auto-allow. |
| 133 | + |
| 134 | +## 7. Data race in `wsApprover` can fatally crash serve |
| 135 | +- **Severity:** Medium |
| 136 | +- **Location:** `cmd/odek/wsapprover.go:117,175` |
| 137 | +- **Flaw:** `a.approveAll[cls] || a.trustAll` read unlocked at :117 while |
| 138 | + `a.approveAll[cls] = true` writes unlocked at :175. Parallel tool goroutines |
| 139 | + share one approver → concurrent map read/write is a fatal Go runtime error. |
| 140 | +- **Fix:** guard all `approveAll`/`trustAll` access with the existing mutex |
| 141 | + (mirror `internal/telegram/approver.go`, which locks correctly). |
| 142 | +- **Verify:** run the serve approval path under `go test -race` with parallel |
| 143 | + tool calls. |
| 144 | + |
| 145 | +## 8. SSRF: URL gate is string-only, no dial-time IP check |
| 146 | +- **Severity:** Medium |
| 147 | +- **Location:** `internal/danger/classifier.go:195-244`; |
| 148 | + `cmd/odek/browser_tool.go`; `cmd/odek/perf_tools.go` |
| 149 | +- **Flaw:** `ClassifyURL` inspects only the literal hostname. A domain whose A |
| 150 | + record resolves to `169.254.169.254` / `10.x` / `192.168.x` classifies as |
| 151 | + plain `NetworkEgress`, and the HTTP clients use a default transport with no |
| 152 | + post-resolution IP guard → SSRF / DNS-rebinding to cloud metadata & internal |
| 153 | + services when egress is allowed. |
| 154 | +- **Fix:** install a custom `DialContext` on the browser/http_batch clients that |
| 155 | + re-checks the resolved `net.IP` against loopback/private/link-local/ULA ranges |
| 156 | + and refuses them. This also closes the redirect-hop and rebinding window. |
| 157 | +- **Verify:** request a host that resolves to `169.254.169.254` and confirm the |
| 158 | + dial is refused. |
| 159 | + |
| 160 | +## 9. Sub-agent results lost on >64KB NDJSON line, then full-timeout hang |
| 161 | +- **Severity:** Medium |
| 162 | +- **Location:** `cmd/odek/subagent_tool.go:247-285` |
| 163 | +- **Flaw:** `runTask` reads child stdout with a default `bufio.Scanner` (64KB |
| 164 | + token cap). Streamed `tool_call` events embedding full tool args (e.g. a large |
| 165 | + `write_file`) exceed 64KB → `ErrTooLong` → reader stops → child blocks on a |
| 166 | + full pipe → `cmd.Wait` hangs until the 120s timeout kills a task that actually |
| 167 | + succeeded. |
| 168 | +- **Fix:** call `scanner.Buffer(make([]byte, 0, 64*1024), maxCap)` with a large |
| 169 | + cap, or switch to a `bufio.Reader` with `ReadString('\n')`. |
| 170 | +- **Verify:** sub-agent task whose streamed event line exceeds 64KB returns its |
| 171 | + result instead of timing out. |
| 172 | + |
| 173 | +## 10. `/new` deletes the per-chat mutex while a run holds it |
| 174 | +- **Severity:** High (correctness/data-integrity) |
| 175 | +- **Location:** `cmd/odek/telegram.go:263` |
| 176 | +- **Flaw:** `/new` runs `chatMu.Delete(chatID)` on the update loop, |
| 177 | + unsynchronized with in-flight `handleChatMessage` goroutines. The next message |
| 178 | + `LoadOrStore`s a fresh mutex and `TryLock`s it → two concurrent runs for the |
| 179 | + same chat corrupt interleaved `sessionManager.Save` writes and clobber each |
| 180 | + other's approver. |
| 181 | +- **Fix:** do not delete the mutex on `/new` (a small per-chat leak is |
| 182 | + acceptable), or only delete it while holding the lock and with no active run. |
| 183 | +- **Verify:** send `/new` during an active run, then a message; confirm the two |
| 184 | + runs do not execute concurrently. |
| 185 | + |
| 186 | +--- |
| 187 | + |
| 188 | +## Secondary findings (track, lower priority) |
| 189 | + |
| 190 | +- **Telegram bot token leaked into logs** — `internal/telegram/bot.go:117-120`. |
| 191 | + `*url.Error` includes the token-bearing URL. Strip credentials from logged |
| 192 | + URLs (log method only). |
| 193 | +- **`currentPromptCancel` single global slot** — `cmd/odek/serve.go:37,581`. |
| 194 | + `/api/cancel` cancels the wrong prompt with multiple tabs; A finishing clobbers |
| 195 | + B's cancel. Track per-connection / per-prompt-ID. |
| 196 | +- **`/stop` cannot interrupt a pending `clarify`** — `cmd/odek/telegram.go:1353`. |
| 197 | + Circular wait until the 10-minute timeout. Make clarify observe `agentCtx`. |
| 198 | +- **Sub-agent / panic edge cases in loop** — |
| 199 | + `internal/loop/loop.go:927-945`: a panicked tool returns an empty result |
| 200 | + (early `return` before `results[idx]` is set); the model gets no error. |
| 201 | +- **UTF-8 byte-slicing in Telegram previews** — |
| 202 | + `cmd/odek/telegram.go:305-327`. `taskPreview[:50]`/`[:80]` slice by byte and |
| 203 | + can emit invalid UTF-8 (Telegram 400). Also `Messages[0]` can index an empty |
| 204 | + slice and previews the system prompt. Use rune-aware truncation (see |
| 205 | + `truncate()` in `subagent.go`). |
| 206 | +- **`activeTaskWG.Add` races `gracefulRestart`'s `Wait`** — |
| 207 | + `cmd/odek/telegram.go:1063-1076` vs `:879-961`. Invert to Add-first, |
| 208 | + re-check-flag, Done+return on bail. |
| 209 | +- **`serve --open` never opens a browser** — |
| 210 | + `cmd/odek/serve.go:1104-1113`. `os.StartProcess` with a bare name does no PATH |
| 211 | + lookup. Use `exec.LookPath` / `exec.Command`. |
| 212 | +- **SearXNG `secret_key` weak sentinel** — |
| 213 | + `docker/searxng/settings.yml` ships `ultrasecretkey` with a matching compose |
| 214 | + default. Mitigated (no host port) but should hard-fail on the sentinel. |
| 215 | +- **Sessions dir created `0755`** — `internal/session/session.go:71`. Tighten to |
| 216 | + `0700` (files are already `0600`). |
| 217 | +- **`ffprobe` leading-dash arg injection** — `cmd/odek/vision_tool.go:101-106`. |
| 218 | + Low impact; add a `--` separator before positional paths. |
| 219 | + |
| 220 | +## Operational follow-up |
| 221 | + |
| 222 | +- **Rotate the credentials** in the working-tree `docker/.env` |
| 223 | + (`ODEK_API_KEY`, `ODEK_TELEGRAM_BOT_TOKEN`) if this tree was ever shared, |
| 224 | + imaged, or backed up. (`docker/.env` is gitignored and was never committed — |
| 225 | + only `.env.example` is tracked.) |
| 226 | + |
| 227 | +## Confirmed NOT vulnerable (do not re-investigate) |
| 228 | + |
| 229 | +- `shell.go` — argv-form `exec.Command` + token-based fail-closed danger |
| 230 | + classifier. |
| 231 | +- File tools — `O_NOFOLLOW` + atomic temp-write/rename (symlink TOCTOU closed). |
| 232 | +- `subagent_key.go` — API key handed off via an unlinked FD, not env/args. |
| 233 | +- `mcp.go` — MCP servers come from local resolved config, not remote-supplied |
| 234 | + definitions; tool descriptions are injection-scanned and outputs wrapped. |
| 235 | +- `docker/.env` — gitignored, never committed (see operational follow-up for the |
| 236 | + on-disk copy). |
0 commit comments