diff --git a/AGENTS.md b/AGENTS.md index 0293297..36f2df2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -149,6 +149,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU - **WebSocket message-size cap** (`cmd/odek/serve.go`) — `odek serve` sets `MaxPayloadBytes` on every WebSocket connection so a local client cannot OOM the server with a huge frame. - **WebSocket approval relay non-blocking** (`cmd/odek/wsapprover.go`) — `HandleResponse` uses a non-blocking send on the pending response channel, so duplicate or late approval responses are dropped instead of blocking the WebSocket read goroutine and exhausting the global connection semaphore. - **Session file size cap** (`internal/session/session.go`) — session files larger than 32 MiB are rejected by `Load()` to prevent OOM from tampered or corrupted transcripts. +- **Session ID write-path validation** (`internal/session/session.go`) — `saveLocked` validates `sess.ID` with `ValidateSessionID` before computing the destination path, and `Load` checks that the embedded ID matches the filename it was loaded from. A planted session file with an attacker-controlled `"id"` field can no longer redirect `Append`/`Save` to write outside the session directory. - **Skill file size cap** (`internal/skills/loader.go`) — `SKILL.md` files larger than 1 MiB are skipped so a malicious project cannot OOM the process at startup or bloat the system prompt. - **Serve sandbox default-on** — `odek serve` enables `--sandbox` automatically unless `--no-sandbox` is passed. - **Sandbox volume confinement** (`internal/sandbox/sandbox.go`) — extra `--sandbox-volume` host paths must resolve to a location under the working directory, cannot contain `..` or symlink escapes, and cannot match sensitive prefixes such as `/etc`, `/proc`, `/sys`, `/dev`, `/root`, `/home`, `/var`, `/run`, or `/var/run/docker.sock`. diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 2221cd7..342001d 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -655,6 +655,12 @@ Now: - `recordTurnAudit` scans `user` messages for untrusted wrappers as well as `tool` messages. - The divergence check receives the original, pre-enrichment user prompt, so injected resources are treated as novel when the agent acts on them. +### 39l. Session store write-path validates the embedded session ID + +`internal/session/session.go` built the destination path from `sess.ID` without validating it, while `Load` only validated the filename requested by the caller. A planted session file (for example, dropped by a malicious local process or extracted from an archive) whose JSON contained `"id": "../config"` would cause the next `Append` or `Save` to write outside the session directory, such as overwriting `~/.odek/config.json`. + +`saveLocked` now calls `ValidateSessionID` before computing the filesystem path, and `Load` checks that the ID inside the file matches the filename it was loaded from. Any mismatch aborts the operation instead of following the attacker-controlled embedded ID. + ### 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. diff --git a/internal/session/session.go b/internal/session/session.go index 5736710..b5ef697 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -311,6 +311,14 @@ func (s *Store) Save(sess *Session) error { // read and write gets replaced with a regular file. // Also atomically updates the session index with the session's metadata. func (s *Store) saveLocked(sess *Session) error { + // Reject malformed or traversal-bearing session IDs before the ID is used + // to build a filesystem path. A planted session file with an embedded + // "id":"../config" must not cause a subsequent Save/Append to overwrite + // files outside the session directory. + if err := ValidateSessionID(sess.ID); err != nil { + return fmt.Errorf("session: refusing unsafe save: %w", err) + } + // Redact secrets from all messages and the task label before writing to // disk. This is defense-in-depth: the loop engine already redacts tool // outputs, but this catches any secrets that slipped through @@ -369,6 +377,12 @@ func (s *Store) Load(id string) (*Session, error) { if err := json.Unmarshal(data, &sess); err != nil { return nil, fmt.Errorf("session: parse %q: %w", id, err) } + // The on-disk ID must match the filename it was loaded from. This prevents + // a planted session file from redirecting a later Save/Append to a path + // derived from an attacker-controlled embedded ID. + if sess.ID != id { + return nil, fmt.Errorf("session: load %q: ID mismatch (file contains %q)", id, sess.ID) + } return &sess, nil } diff --git a/internal/session/session_test.go b/internal/session/session_test.go index 226e2e4..77ad262 100644 --- a/internal/session/session_test.go +++ b/internal/session/session_test.go @@ -774,6 +774,64 @@ func TestStore_Delete_PathTraversalRejected(t *testing.T) { } } +// TestStore_Save_RejectMalformedID verifies that Save refuses to write a +// session whose embedded ID contains path traversal. +func TestStore_Save_RejectMalformedID(t *testing.T) { + store := newTestStore(t) + msgs := []llm.Message{{Role: "user", Content: "test"}} + sess, _ := store.Create(msgs, "m", "test") + + sess.ID = "../config" + err := store.Save(sess) + if err == nil { + t.Fatal("Save() with traversal ID should return error") + } + if !strings.Contains(err.Error(), "invalid ID") { + t.Errorf("error should mention 'invalid ID', got: %v", err) + } +} + +// TestStore_Load_RejectEmbeddedIDMismatch verifies that Load detects a +// session file whose embedded ID does not match its filename. +func TestStore_Load_RejectEmbeddedIDMismatch(t *testing.T) { + store := newTestStore(t) + + // Plant a session file whose filename is benign but whose JSON ID is not. + plantedID := "20260718-plant001" + malicious := `{"id":"../config","created_at":"2026-07-18T00:00:00Z","updated_at":"2026-07-18T00:00:00Z","model":"m","turns":1,"task":"x","messages":[]}` + if err := os.WriteFile(store.Path(plantedID), []byte(malicious), 0600); err != nil { + t.Fatal(err) + } + + _, err := store.Load(plantedID) + if err == nil { + t.Fatal("Load() of mismatched embedded ID should return error") + } + if !strings.Contains(err.Error(), "ID mismatch") { + t.Errorf("error should mention 'ID mismatch', got: %v", err) + } +} + +// TestStore_Append_RejectEmbeddedIDMismatch verifies that Append refuses to +// rewrite a planted session file whose embedded ID differs from the filename. +func TestStore_Append_RejectEmbeddedIDMismatch(t *testing.T) { + store := newTestStore(t) + + plantedID := "20260718-plant002" + malicious := `{"id":"../config","created_at":"2026-07-18T00:00:00Z","updated_at":"2026-07-18T00:00:00Z","model":"m","turns":1,"task":"x","messages":[]}` + if err := os.WriteFile(store.Path(plantedID), []byte(malicious), 0600); err != nil { + t.Fatal(err) + } + + err := store.Append(plantedID, []llm.Message{{Role: "user", Content: "more"}}) + if err == nil { + t.Fatal("Append() to planted mismatched file should return error") + } + if !strings.Contains(err.Error(), "ID mismatch") { + t.Errorf("error should mention 'ID mismatch', got: %v", err) + } +} + // ── Additional edge-case coverage ────────────────────────────────────── func TestValidateSessionID_NullByte(t *testing.T) {