diff --git a/AGENTS.md b/AGENTS.md index 1c9d461..baec75b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -163,7 +163,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU - **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`. - **Sandbox read-only enforcement** (`cmd/odek/sandbox_file.go` + `cmd/odek/file_tool.go` + `cmd/odek/perf_tools.go`) — when a sandbox container is active, `write_file`, `patch`, and `batch_patch` translate host paths to `/workspace/...` and copy data into the container with `docker cp`, so a read-only workspace mount (`--sandbox-readonly`) is enforced for the agent's own file tools. - **Project config sensitive-field rejection** (`internal/config/loader.go`) — `./odek.json` is untrusted, so `base_url`, `api_key`, `system`, the `dangerous` section, `embedding`, `memory`, `sessions`, `skills.dirs`/`skills.embedding`, `telegram`, and `web_search` set there are ignored (with stderr warnings). Project-level sandbox knobs (`sandbox_env`, `sandbox_image`, `sandbox_network`, `sandbox_volumes`) are not silently ignored either; they are gated by the project-level sandbox approval flow before application. These can only be configured from operator-controlled sources: `~/.odek/config.json`, `ODEK_*` env vars, or CLI flags. -- **MCP subprocess environment sanitisation** (`internal/mcpclient/client.go`) — MCP server children receive only a minimal allowlist of safe environment variables plus explicit `env` overrides. Keys matching secret patterns (`*_API_KEY`, `*_TOKEN`, `*_SECRET`, `*_PASSWORD`, etc.) are stripped, preventing a compromised or malicious MCP server from reading parent secrets. +- **MCP subprocess environment sanitisation** (`internal/mcpclient/client.go`) — MCP server children receive only a minimal allowlist of safe environment variables plus explicit `env` overrides. Keys matching secret patterns (`*_API_KEY`, `*_TOKEN`, `*_SECRET`, `*_PASSWORD`, etc.) are stripped, preventing a compromised or malicious MCP server from reading parent secrets. Names are normalised (uppercased, `-`/`_` stripped) before matching, so non-underscore spellings such as `API-KEY` or `APIKEY` cannot slip a secret through the override path. - **MCP `tools/list` metadata hardening** (`internal/mcpclient/client.go`, `cmd/odek/mcp_approval.go`, `cmd/odek/main.go`) — tool names from MCP servers are validated (ASCII letters/digits/`-`/`_`, ≤ 64 chars) and descriptions are scanned for injection patterns. Tools whose raw names collide with odek built-ins are rejected. Each tool from a project-level server requires per-tool approval (interactive, `ODEK_APPROVE_MCP=1`, or persisted in `~/.odek/mcp_tool_approvals.json`); global servers from `~/.odek/config.json` are operator-trusted. Approval keys hash the server's `env` map (sorted key/value pairs), and the interactive prompt prints each env variable with its value, so a later `env` change cannot silently reuse an old approval. - **Read-only perf-tool file-size cap** (`cmd/odek/perf_tools.go`) — `count_lines`, `checksum`, `head_tail`, and `word_count` reject files larger than 10 MiB before scanning/hashing, consistent with other perf tools. - **Inline content size cap** (`cmd/odek/perf_tools.go`) — `base64` and `tr` reject inline `string`/`content` arguments larger than 10 MiB, preventing prompt-injected multi-hundred-megabyte payloads from OOMing the process. diff --git a/docs/SECURITY.md b/docs/SECURITY.md index e6b8ab9..4d35926 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -423,7 +423,7 @@ Go's `net.IP.IsPrivate()` covers RFC1918 and RFC4193 private ranges, but it does ### 19. MCP server environment sanitisation -MCP server subprocesses no longer inherit the full odek process environment. They receive only a minimal allowlist of safe variables (e.g. `PATH`, `HOME`, `LANG`, `TMPDIR`) plus any explicit `env` overrides from the server config. Keys matching secret patterns — `*_API_KEY`, `*_TOKEN`, `*_SECRET`, `*_PASSWORD`, `*_CREDENTIAL`, `*_PRIVATE_KEY`, etc. — are stripped even when listed in `env`. This prevents a compromised or malicious MCP server from reading secrets loaded from `~/.odek/secrets.env` or other provider keys that were present in the parent environment. +MCP server subprocesses no longer inherit the full odek process environment. They receive only a minimal allowlist of safe variables (e.g. `PATH`, `HOME`, `LANG`, `TMPDIR`) plus any explicit `env` overrides from the server config. Keys matching secret patterns — `*_API_KEY`, `*_TOKEN`, `*_SECRET`, `*_PASSWORD`, `*_CREDENTIAL`, `*_PRIVATE_KEY`, etc. — are stripped even when listed in `env`. Matching normalises each name first (uppercased, `-` and `_` removed), so non-underscore spellings like `API-KEY` or `APIKEY` — both legal in environment variable names — cannot bypass the filter through the override path. This prevents a compromised or malicious MCP server from reading secrets loaded from `~/.odek/secrets.env` or other provider keys that were present in the parent environment. ### 20. Schedule file atomic-write hardening diff --git a/internal/mcpclient/client.go b/internal/mcpclient/client.go index fa0ab80..dab0d63 100644 --- a/internal/mcpclient/client.go +++ b/internal/mcpclient/client.go @@ -308,13 +308,18 @@ var allowedEnvVars = map[string]bool{ // isSensitiveEnvVar reports whether a key looks like a secret. These patterns // are blocked from being forwarded to MCP children even if they are present in // the parent environment or explicitly supplied as overrides. +// +// The name is normalised (uppercased, "-" and "_" stripped) before matching so +// non-underscore spellings — API-KEY, APIKEY, Private-Key — cannot slip a +// secret past the filter; environment variable names legally contain hyphens +// or no separator at all. func isSensitiveEnvVar(key string) bool { - upper := strings.ToUpper(key) + norm := strings.NewReplacer("-", "", "_", "").Replace(strings.ToUpper(key)) for _, pat := range []string{ - "API_KEY", "TOKEN", "SECRET", "PASSWORD", "CREDENTIAL", "CREDS", - "PRIVATE_KEY", "ACCESS_KEY", + "APIKEY", "TOKEN", "SECRET", "PASSWORD", "CREDENTIAL", "CREDS", + "PRIVATEKEY", "ACCESSKEY", } { - if strings.Contains(upper, pat) { + if strings.Contains(norm, pat) { return true } } diff --git a/internal/mcpclient/client_test.go b/internal/mcpclient/client_test.go index 915cf7e..5688af5 100644 --- a/internal/mcpclient/client_test.go +++ b/internal/mcpclient/client_test.go @@ -417,22 +417,50 @@ func TestBuildEnv_OverridesCannotInjectSecrets(t *testing.T) { defer func() { osEnviron = orig }() result := buildEnv(map[string]string{ - "LEGIT_VAR": "ok", + "LEGIT_VAR": "ok", "EVIL_API_KEY": "sk-stolen", "BOT_TOKEN": "tok-stolen", + // Non-underscore spellings: env var names legally contain hyphens or + // no separator, and must be caught by the normalised matching. + "API-KEY": "sk-stolen", + "APIKEY": "sk-stolen", + "PRIVATE-KEY": "pem-stolen", + "SECRET-KEY": "shh", }) m := envToMap(result) if m["LEGIT_VAR"] != "ok" { t.Errorf("LEGIT_VAR = %q, want ok", m["LEGIT_VAR"]) } - for _, k := range []string{"EVIL_API_KEY", "BOT_TOKEN"} { + for _, k := range []string{"EVIL_API_KEY", "BOT_TOKEN", "API-KEY", "APIKEY", "PRIVATE-KEY", "SECRET-KEY"} { if _, ok := m[k]; ok { t.Errorf("sensitive override %q should be dropped", k) } } } +func TestIsSensitiveEnvVar_NormalizesSeparators(t *testing.T) { + blocked := []string{ + "MY_API_KEY", "API-KEY", "APIKEY", "Api-Key", + "PRIVATE-KEY", "PRIVATEKEY", "ACCESS-KEY", "ACCESSKEY", + "SECRET-KEY", "SECRETKEY", "AUTH-TOKEN", "AUTHTOKEN", + "DB-PASSWORD", "PASSWORD", "AWS-CREDS", "CREDENTIALS", + } + for _, k := range blocked { + if !isSensitiveEnvVar(k) { + t.Errorf("isSensitiveEnvVar(%q) = false, want true", k) + } + } + allowed := []string{ + "PATH", "HOME", "EDITOR", "NODE_ENV", "MCP_SERVER_NAME", "KEYSTONE", + } + for _, k := range allowed { + if isSensitiveEnvVar(k) { + t.Errorf("isSensitiveEnvVar(%q) = true, want false", k) + } + } +} + func envToMap(env []string) map[string]string { m := make(map[string]string, len(env)) for _, e := range env {