fix(security): Telegram low findings #62, #75, #76

- #62: cap Telegram plan files at 1 MiB for ReadPlan/MostRecentPlan; list
  previews read only first 8 KiB
- #75: verify outbound media final component with atomic O_NOFOLLOW open +
  fstat on Unix to close symlink TOCTOU race
- #76: create Telegram log files with 0600 and chmod existing files
- added regression tests and updated SECURITY.md / AGENTS.md

Author: jkyberneees

AGENTS.md

@@ -152,7 +152,9 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
 - **Telegram singleton flock lock** (`cmd/odek/telegram.go` + `internal/flock`) — the Telegram bot now uses an advisory `flock` on `~/.odek/telegram.lock` instead of a PID file probed with signals. This removes the non-Linux path where a planted PID could cause odek to kill an arbitrary process.
 - **Telegram photo caption wrapping** (`cmd/odek/telegram.go`) — photo captions cross the Telegram trust boundary, so they are wrapped as untrusted content both when passed to the local vision model and when injected into the main agent's user message.
 - **`send_message` callback prefix restriction** (`internal/tool/send_message.go` + `cmd/odek/telegram.go`) — the `send_message` tool rejects any button whose `callback_data` starts with a reserved internal prefix (`apr:`, `den:`, `trs:`, `clarify:`, `skill_save:`, `skill_skip:`); only user-facing `cb:` callbacks are allowed. The Telegram sender closure validates again as defense-in-depth, preventing a forged approval or skill button.
-- **Telegram outbound media path allowlist** (`internal/telegram/media_path.go` + `internal/telegram/handler.go` + `internal/tool/send_message.go` + `cmd/odek/telegram.go`) — paths supplied to `MEDIA:...` prefixes or `send_message(file=...)` are resolved to an absolute path and verified against an allowlist (cwd, `~/.odek/media/`, system temp dir). `os.Lstat` rejects symlink final components and `filepath.EvalSymlinks` ensures the resolved path does not escape the allowlist, preventing prompt-injection-driven exfiltration of arbitrary files.
+- **Telegram outbound media path allowlist** (`internal/telegram/media_path.go` + `internal/telegram/handler.go` + `internal/tool/send_message.go` + `cmd/odek/telegram.go`) — paths supplied to `MEDIA:...` prefixes or `send_message(file=...)` are resolved to an absolute path and verified against an allowlist (cwd, `~/.odek/media/`, system temp dir). On Unix the final component is opened with `O_NOFOLLOW` and `fstat`'d to avoid a symlink TOCTOU race; `filepath.EvalSymlinks` ensures the resolved path does not escape the allowlist, preventing prompt-injection-driven exfiltration of arbitrary files.
+- **Telegram plan file size cap** (`internal/telegram/plan.go`) — plan files larger than 1 MiB are rejected by `ReadPlan` and `MostRecentPlan`, and `ListPlans` only reads the first 8 KiB for preview. This prevents a prompt-injected agent from causing an OOM via a multi-hundred-megabyte plan file.
+- **Telegram log file permissions** (`internal/telegram/log.go`) — Telegram log files are created with `0600`, and `os.Chmod` hardens existing files. Chat IDs and task snippets are no longer world-readable by default.
 - **Session ID entropy + session-scoped auth tokens** (`internal/session/session.go`, `cmd/odek/serve.go`) — session IDs now carry 128 bits of randomness (16 bytes / 32 hex chars); each session stores a 256-bit `AuthToken` required by `GET/DELETE/POST /api/sessions/<id>`, `POST /api/cancel`, and WebSocket session-resume messages via `X-Session-Token` header, `session_token` cookie, or `auth_token` WS field. Per-IP rate limiting (60/min) on session lookups adds a brute-force backstop.
 - **Skill/episode untrusted wrapper** (`internal/loop/loop.go` + `odek.go`) — skill context and retrieved session-episode context are passed through the caller-provided untrusted wrapper (the same nonce'd `<untrusted_content_*>` boundary used for tool output) before being injected into the model's system context. This prevents a compromised or tainted skill/episode from being treated as trusted system instructions.
 - **`env` / `printenv` environment-dump gate** (`internal/danger/classifier.go`) — bare `env` and `printenv` invocations are classified as `system_write` because they can leak process-environment secrets that the redaction scanner does not recognise. `env VAR=value <cmd>` still classifies `<cmd>` normally.

docs/SECURITY.md

@@ -353,7 +353,7 @@ When the agent emits `MEDIA:photo:/path`, `MEDIA:voice:/path`, `MEDIA:document:/
 - `~/.odek/media/`, and
 - the system temporary directory.
 
-The path is resolved to an absolute, cleaned form with `filepath.Abs`, symlinks are resolved with `filepath.EvalSymlinks`, and the final component is checked with `os.Lstat`. If the final component is a symlink, or if the resolved path escapes the allowlist, the upload is rejected. This closes the arbitrary-file-read/exfiltration vector where a prompt-injected agent asks the bot to send files such as `/home/user/.ssh/id_rsa`.
+The path is resolved to an absolute, cleaned form with `filepath.Abs`, symlinks are resolved with `filepath.EvalSymlinks`, and the final component is verified with an atomic `O_NOFOLLOW` open + `fstat` (Unix). If the final component is a symlink, or if the resolved path escapes the allowlist, the upload is rejected. This closes the arbitrary-file-read/exfiltration vector where a prompt-injected agent asks the bot to send files such as `/home/user/.ssh/id_rsa`.
 
 ### 24. Session ID entropy + session-scoped auth tokens
 
@@ -448,6 +448,14 @@ Telegram's message and caption limits are defined in UTF-16 code units, but `int
 
 The `send_message` tool lets the agent send arbitrary text messages to Telegram using `ParseModeMarkdownV2`. Because the LLM may echo or reformat attacker-controllable content, the text is now escaped with `telegram.EscapeMarkdown` before sending. This prevents a prompt-injected payload from using Telegram's Markdown syntax to hide malicious links, fake buttons, or instruction-like formatting inside an otherwise ordinary-looking message.
 
+### 39a. Telegram plan file size cap
+
+Plan files live in `~/.odek/plans/` and are loaded by `/plan_view` and injected into context by `/plan_resume`. A prompt-injected agent could write a multi-hundred-megabyte plan, causing the next plan operation to OOM. `ReadPlan` and `MostRecentPlan` now reject files larger than 1 MiB, and `ListPlans` reads only the first 8 KiB for preview.
+
+### 39b. Telegram log file permissions
+
+Telegram log files were created with world-readable `0644` permissions, exposing chat IDs and task snippets to other local users. `NewFileLogger` now creates log files with `0600` and `os.Chmod`'s existing files to the same mode.
+
 ### 40. `/api/resources` result limit cap
 
 The `/api/resources?q=...&limit=N` autocomplete endpoint previously accepted any positive `limit` value. It is now capped to 100 results both in the HTTP handler and in `Registry.Search`. This prevents a prompt-injected or attacker-forged request from forcing an unbounded directory walk and returning a multi-megabyte JSON response.

internal/telegram/log.go

@@ -56,12 +56,15 @@ type fileLogger struct {
 func NewFileLogger(level LogLevel, path string) Logger {
 	fl := &fileLogger{level: level, mu: &sync.Mutex{}}
 	if path != "" {
-		f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
+		f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600)
 		if err != nil {
 			// Fall back to stderr if we can't open the file.
 			fmt.Fprintf(os.Stderr, "telegram: failed to open log file %s: %v\n", path, err)
 			fl.file = os.Stderr
 		} else {
+			// Harden existing files that were created with 0644 in earlier
+			// versions; ignore chmod errors (e.g. read-only FS).
+			_ = os.Chmod(path, 0600)
 			fl.file = f
 		}
 	} else {

internal/telegram/log_test.go

@@ -50,6 +50,15 @@ func TestNewFileLogger_filePath(t *testing.T) {
 	if _, err := os.Stat(path); os.IsNotExist(err) {
 		t.Error("log file was not created")
 	}
+
+	// Verify permissions are not group/other-readable.
+	info, err := os.Stat(path)
+	if err != nil {
+		t.Fatalf("stat log file: %v", err)
+	}
+	if info.Mode().Perm()&0077 != 0 {
+		t.Errorf("log file permissions = %04o, want no group/other bits", info.Mode().Perm())
+	}
 }
 
 func TestNewFileLogger_invalidPath(t *testing.T) {

internal/telegram/media_path.go

@@ -42,19 +42,16 @@ func ResolveMediaPath(path string) (string, error) {
 	abs = filepath.Clean(abs)
 
 	// The final component must not be a symlink and must be a regular file.
-	info, err := os.Lstat(abs)
-	if err != nil {
-		return "", fmt.Errorf("media path: lstat: %w", err)
-	}
-	if info.Mode()&os.ModeSymlink != 0 {
-		return "", fmt.Errorf("media path: symlinks are not allowed: %s", abs)
-	}
-	if !info.Mode().IsRegular() {
-		return "", fmt.Errorf("media path: not a regular file: %s", abs)
+	// On Unix this is done with an atomic O_NOFOLLOW open + fstat to prevent
+	// a TOCTOU race where a directory is swapped for a symlink between lstat
+	// and the subsequent read.
+	if err := verifyRegularFile(abs); err != nil {
+		return "", err
 	}
 
-	// Resolve all symlinks in the path. Any symlink that escapes the allowlist
-	// is caught by the containment check below.
+	// Resolve any symlinks in the path (but not the final component, which we
+	// already verified is a regular file). Any symlink that escapes the
+	// allowlist is caught by the containment check below.
 	resolved, err := filepath.EvalSymlinks(abs)
 	if err != nil {
 		return "", fmt.Errorf("media path: resolve symlinks: %w", err)

internal/telegram/media_path_check_other.go

@@ -0,0 +1,25 @@
+//go:build !unix
+
+package telegram
+
+import (
+	"fmt"
+	"os"
+)
+
+// verifyRegularFile is the non-Unix fallback. It uses Lstat instead of an
+// atomic O_NOFOLLOW open; the Unix implementation provides stronger TOCTOU
+// protection.
+func verifyRegularFile(path string) error {
+	info, err := os.Lstat(path)
+	if err != nil {
+		return fmt.Errorf("media path: lstat: %w", err)
+	}
+	if info.Mode()&os.ModeSymlink != 0 {
+		return fmt.Errorf("media path: symlinks are not allowed: %s", path)
+	}
+	if !info.Mode().IsRegular() {
+		return fmt.Errorf("media path: not a regular file: %s", path)
+	}
+	return nil
+}

internal/telegram/media_path_check_unix.go

@@ -0,0 +1,33 @@
+//go:build unix
+
+package telegram
+
+import (
+	"fmt"
+
+	"golang.org/x/sys/unix"
+)
+
+// verifyRegularFile verifies that path points to a regular file and is not a
+// symlink. It opens the path with O_NOFOLLOW and fstat's the resulting fd so
+// the check is atomic: an attacker cannot swap a regular file for a symlink
+// between an lstat and a later open (TOCTOU).
+func verifyRegularFile(path string) error {
+	fd, err := unix.Open(path, unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_NONBLOCK, 0)
+	if err != nil {
+		if err == unix.ELOOP {
+			return fmt.Errorf("media path: symlinks are not allowed: %s", path)
+		}
+		return fmt.Errorf("media path: open: %w", err)
+	}
+	defer unix.Close(fd)
+
+	var st unix.Stat_t
+	if err := unix.Fstat(fd, &st); err != nil {
+		return fmt.Errorf("media path: fstat: %w", err)
+	}
+	if st.Mode&unix.S_IFMT != unix.S_IFREG {
+		return fmt.Errorf("media path: not a regular file: %s", path)
+	}
+	return nil
+}

internal/telegram/plan.go

@@ -2,6 +2,7 @@ package telegram
 
 import (
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"sort"
@@ -10,6 +11,12 @@ import (
 	"unicode"
 )
 
+// maxPlanBytes caps the size of a plan file that odek will read into memory
+// or inject into a session context. A prompt-injected agent could otherwise
+// write a multi-hundred-megabyte plan and OOM the next /plan_view or
+// /plan_resume.
+const maxPlanBytes = 1 * 1024 * 1024 // 1 MiB
+
 // ── Plan Manager ───────────────────────────────────────────────────────
 //
 // Plans are stored as .md files in ~/.odek/plans/. Each plan is a
@@ -53,6 +60,38 @@ func ensurePlansDir() (string, error) {
 	return dir, nil
 }
 
+// readPlanFile reads a plan file after verifying it is within the size cap.
+func readPlanFile(path string) ([]byte, error) {
+	info, err := os.Stat(path)
+	if err != nil {
+		return nil, err
+	}
+	if info.Size() > maxPlanBytes {
+		return nil, fmt.Errorf("plan file %q is too large (%d bytes, max %d)", filepath.Base(path), info.Size(), maxPlanBytes)
+	}
+	return os.ReadFile(path)
+}
+
+// readPlanPreview reads the first few kilobytes of a plan file for the plan
+// list preview. It does not load arbitrarily large files just to extract the
+// first line.
+func readPlanPreview(path string, maxLen int) (string, error) {
+	f, err := os.Open(path)
+	if err != nil {
+		return "", err
+	}
+	defer f.Close()
+
+	// 8 KiB is plenty for a first-line preview.
+	const previewBufBytes = 8 * 1024
+	buf := make([]byte, previewBufBytes)
+	n, err := io.ReadFull(f, buf)
+	if err != nil && err != io.EOF && err != io.ErrUnexpectedEOF {
+		return "", err
+	}
+	return firstLine(string(buf[:n]), maxLen), nil
+}
+
 // Slugify converts a description into a filesystem-safe slug.
 // Rules: lowercase, max 60 chars, only [a-z0-9] and hyphens,
 // multiple hyphens collapsed, no leading/trailing hyphens.
@@ -131,8 +170,8 @@ func ListPlans(limit int) ([]PlanInfo, error) {
 		slug := strings.TrimSuffix(e.Name(), ".md")
 		path := filepath.Join(dir, e.Name())
 		preview := ""
-		if data, err := os.ReadFile(path); err == nil {
-			preview = firstLine(string(data), 80)
+		if p, err := readPlanPreview(path, 80); err == nil {
+			preview = p
 		}
 		infos = append(infos, PlanInfo{
 			Slug:    slug,
@@ -204,7 +243,7 @@ func ReadPlan(slugPrefix string) (string, string, error) {
 		return "", "", fmt.Errorf("no plan matching %q found — use /plans to list", slugPrefix)
 	}
 
-	data, err := os.ReadFile(filepath.Join(dir, match+".md"))
+	data, err := readPlanFile(filepath.Join(dir, match+".md"))
 	if err != nil {
 		return "", "", fmt.Errorf("read plan %q: %w", match, err)
 	}
@@ -276,7 +315,7 @@ func MostRecentPlan() (string, string, error) {
 		return "", "", fmt.Errorf("no plans found — create one with /plan <description>")
 	}
 
-	data, err := os.ReadFile(infos[0].Path)
+	data, err := readPlanFile(infos[0].Path)
 	if err != nil {
 		return "", "", fmt.Errorf("read plan: %w", err)
 	}

internal/telegram/plan_test.go

@@ -181,6 +181,24 @@ func TestReadPlan_NoMatch(t *testing.T) {
 	}
 }
 
+func TestReadPlan_OversizeRejected(t *testing.T) {
+	tmp := t.TempDir()
+	t.Setenv("HOME", tmp)
+
+	dir := filepath.Join(tmp, ".odek", "plans")
+	os.MkdirAll(dir, 0755)
+	path := filepath.Join(dir, "huge-plan.md")
+	os.WriteFile(path, []byte(strings.Repeat("x", maxPlanBytes+1)), 0644)
+
+	_, _, err := ReadPlan("huge-plan")
+	if err == nil {
+		t.Fatal("expected error for oversized plan")
+	}
+	if !strings.Contains(err.Error(), "too large") {
+		t.Errorf("error = %q, want 'too large'", err)
+	}
+}
+
 func TestDeletePlan(t *testing.T) {
 	tmp := t.TempDir()
 	t.Setenv("HOME", tmp)