diff --git a/AGENTS.md b/AGENTS.md index 2d3a414..bba564e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -181,6 +181,9 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU - **Web UI prompt/model validation** (`cmd/odek/serve.go`) — server-side cap on WebSocket prompt size (1 MiB) and validation of model ID length/characters, preventing oversized or unusual payloads from reaching the LLM. - **Sub-agent runaway limits** (`cmd/odek/subagent.go`) — `--timeout` is capped at 3600s and `--max-iter` at 100, so a single `odek subagent` invocation cannot run indefinitely. - **Secrets.env permission gate** (`internal/config/loader.go`) — refuses to load `~/.odek/secrets.env` when it is group/world-readable, preventing local users from reading API keys injected into the environment. +- **git config code execution** (`internal/danger/classifier.go`) — `git -c alias.*=!`, `git -c core.pager=...`, `git -c core.fsmonitor=...`, `git -c credential.helper=...`, and the `git config` subcommand are classified as `code_execution` because they can inject arbitrary shell commands. +- **find / rsync destructive flags** (`internal/danger/classifier.go`) — `find -delete` and `rsync --delete` / `--del` / `--remove-source-files` are classified as `destructive`; `find -fprint` / `-fprintf` are classified as `local_write`. +- **Shell operand path classification** (`internal/danger/classifier.go`) — file operands and redirect targets of shell commands are checked with `ClassifyPath`, so writes to `~/.bashrc`, `~/.zshrc`, `~/.profile`, `~/.ssh`, `~/.odek`, etc. are escalated to `system_write` instead of auto-allowed `local_write`. - **Secret redaction** (`internal/redact/redact.go`) — 20+ patterns: OpenAI, Anthropic, GitHub PAT, AWS, PEM, JWT, Vault, Google OAuth, SendGrid, Discord, DB URLs, etc. ### Security findings (`sec_findings.md`) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 339f2b1..7f15926 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -95,6 +95,9 @@ The classifier is hardened against common evasion tricks (see the package doc in - `wipefs`, `blkdiscard`, `sgdisk`/`gdisk`/`cfdisk`/`sfdisk`, `mkswap`, `badblocks`, `cryptsetup`, and the `mkfs.*` family are `destructive`; `shred` is target-aware (local file → `local_write`, raw device / wipe target → `destructive`). - `shutdown`, `reboot`, `halt`, `poweroff`, `init 0`/`init 6` — machine power-control commands are `destructive` (deny-by-default) with an accurate label instead of falling through to `unknown`. - `env` and `printenv` — a full process-environment dump is classified as `system_write` because it can leak secrets that the redaction scanner does not recognise. `env FOO=bar ` still classifies the real `` normally. +- `git -c alias.x='!id' x`, `git -c core.pager='sh -c id' --paginate log`, `git config --global alias.pwn '!cmd'` — `git -c` / `--config-env` overrides and the `git config` subcommand are `code_execution` because they can define arbitrary shell commands. +- `find . -delete`, `rsync -a --delete /empty/ ~`, `rsync --remove-source-files` — bulk-deletion flags are `destructive`; `find -fprint` / `-fprintf` are `local_write` because they write match lists to arbitrary files. +- `echo x >> ~/.bashrc`, `cp evil ~/.profile`, `dd if=evil of=~/.bashrc` — shell file operands and redirect targets are run through `ClassifyPath`, so writes to shell rc files, `~/.ssh`, `~/.odek` trust anchors, and other home-sensitive paths are `system_write` instead of auto-allowed `local_write`. Regression suites (`internal/danger/classifier_bypass_test.go` and `hardening_test.go`) pin these as known-closed evasions. If you find a new bypass, those test files are the place to add it. diff --git a/internal/danger/classifier.go b/internal/danger/classifier.go index 8945741..2c025b3 100644 --- a/internal/danger/classifier.go +++ b/internal/danger/classifier.go @@ -1572,6 +1572,86 @@ func isSensitiveOdekPath(tok string) bool { return isOdekTrustAnchor(home, abs) } +// classifyShellTokenPath expands ~ and common environment-variable shorthands +// in a shell token, strips prefixes like "of=", and returns the ClassifyPath +// result for the filesystem path it names. It lets shell commands be checked +// against the same home-sensitive-dir and rc-file lists used by the file tools, +// closing the gap where `echo x >> ~/.bashrc` was auto-allowed as local_write. +func classifyShellTokenPath(tok string) RiskClass { + path := tok + + // Strip common key=value prefixes used by dd and similar tools. + for _, prefix := range []string{"of=", "if="} { + if strings.HasPrefix(strings.ToLower(path), prefix) { + path = path[len(prefix):] + break + } + } + if path == "" { + return Safe + } + + // Expand ~ and simple $HOME/${HOME} forms that appear in shell commands. + home, _ := os.UserHomeDir() + if home != "" { + if strings.HasPrefix(path, "~") { + path = home + path[1:] + } else if path == "$HOME" || strings.HasPrefix(path, "$HOME/") { + path = home + path[len("$HOME"):] + } else if path == "${HOME}" || strings.HasPrefix(path, "${HOME}/") { + path = home + path[len("${HOME}"):] + } + } + + return ClassifyPath(path) +} + +// shellPathIsSensitive reports whether a shell token names a path that should +// be treated as system_write or worse. It is used for both write operands and +// general path arguments so reads of rc files and trust anchors are gated. +func shellPathIsSensitive(tok string) bool { + cls := classifyShellTokenPath(tok) + return Rank(cls) >= Rank(SystemWrite) +} + +// shellPathIsHomeSensitive reports whether a shell token names a path under +// the user's home directory that ClassifyPath considers system_write or worse +// (e.g. ~/.bashrc, ~/.ssh/id_rsa, ~/.odek/config.json). It is narrower than +// shellPathIsSensitive so that touching /etc itself does not break the existing +// classification of commands like `cd /etc`. +func shellPathIsHomeSensitive(tok string) bool { + home, err := os.UserHomeDir() + if err != nil || home == "" { + return false + } + path := tok + for _, prefix := range []string{"of=", "if="} { + if strings.HasPrefix(strings.ToLower(path), prefix) { + path = path[len(prefix):] + break + } + } + if path == "" { + return false + } + if strings.HasPrefix(path, "~") { + path = home + path[1:] + } else if path == "$HOME" || strings.HasPrefix(path, "$HOME/") { + path = home + path[len("$HOME"):] + } else if path == "${HOME}" || strings.HasPrefix(path, "${HOME}/") { + path = home + path[len("${HOME}"):] + } + abs, err := filepath.Abs(path) + if err != nil { + return false + } + abs = filepath.Clean(abs) + if abs != home && !strings.HasPrefix(abs, home+"/") { + return false + } + return Rank(ClassifyPath(abs)) >= Rank(SystemWrite) +} + // ── Small token helpers ──────────────────────────────────────────────── // commandName returns the program name from a token, taking the basename of @@ -1629,6 +1709,21 @@ func hasAny(tokens []string, names ...string) bool { return false } +// rsyncDeleteFlags are flags that cause rsync to delete files on the +// destination or remove them from the source — unrecoverable bulk deletion. +func hasAnyRsyncDelete(tokens []string) bool { + for _, t := range tokens { + if strings.HasPrefix(t, "--delete") { + return true + } + switch t { + case "--del", "--remove-source-files": + return true + } + } + return false +} + // isAssignment reports whether a token is a NAME=VALUE shell assignment // (used to skip `env FOO=bar … cmd`). A leading-slash token like // /a=b is a path, not an assignment. @@ -1854,6 +1949,16 @@ func isDestructive(first string, tokens []string) bool { return false } + // find -delete removes every matched file; rsync --delete / --remove-source-files + // can wipe a directory tree. Both are unrecoverable bulk deletion, equivalent to + // rm -rf, so they classify as destructive. + if first == "find" && hasAny(tokens, "-delete") { + return true + } + if first == "rsync" && hasAnyRsyncDelete(tokens) { + return true + } + if !destructivePrefixes[first] || len(tokens) < 2 { return false } @@ -1905,17 +2010,17 @@ func isSystemWrite(first string, tokens []string) bool { // write check. Escalate them here so they prompt instead. if writePrefixes[first] || first == "ln" || first == "install" { for _, tok := range tokens[1:] { - if isSystemPath(tok) { + if shellPathIsSensitive(tok) { return true } } } - // Check redirect targets for system paths + // Check redirect targets for sensitive paths for _, tok := range tokens { if tok == ">" || tok == ">>" { continue } - if isSystemPath(tok) { + if shellPathIsSensitive(tok) { // Check if it's a redirect target (token follows > or >>) for i, t := range tokens { if (t == ">" || t == ">>") && i+1 < len(tokens) && tokens[i+1] == tok { @@ -1924,6 +2029,15 @@ func isSystemWrite(first string, tokens []string) bool { } } } + // dd of=... writes its output to an arbitrary path; catch writes to + // rc files and trust anchors that would otherwise slip through as safe. + if first == "dd" { + for _, tok := range tokens { + if strings.HasPrefix(strings.ToLower(tok), "of=") && shellPathIsSensitive(tok) { + return true + } + } + } return false } @@ -1994,6 +2108,10 @@ func isLocalWrite(first string, tokens []string) bool { if writePrefixes[first] { return true } + // find -fprint/-fprintf write match lists to a file (arbitrary path) + if first == "find" && hasAny(tokens, "-fprint", "-fprintf") { + return true + } // Any command with > or >> is a write for _, tok := range tokens { if tok == ">" || tok == ">>" { @@ -2067,7 +2185,72 @@ func isNetworkEgress(first string, tokens []string) bool { return true } +// gitCodeExecConfigKeys are git config keys whose values cause arbitrary +// command execution when set via -c/--config-env or git config. +var gitCodeExecConfigKeys = map[string]bool{ + "core.pager": true, + "core.fsmonitor": true, + "credential.helper": true, +} + +// isGitCodeExecution reports whether a git invocation carries a config override +// or subcommand that can execute arbitrary shell code. It deliberately errs on +// the side of classification rather than parsing the full git config grammar: +// aliases whose value starts with "!" run shell commands, and a small set of +// core/credential keys spawn pagers or helpers. +func isGitCodeExecution(tokens []string) bool { + for i := 0; i < len(tokens); i++ { + tok := tokens[i] + if tok == "git" { + continue + } + + var val string + consumed := false + switch { + case tok == "-c" || tok == "--config-env": + if i+1 < len(tokens) { + val = tokens[i+1] + consumed = true + } + case strings.HasPrefix(tok, "-c"): + val = tok[2:] + case strings.HasPrefix(tok, "--config-env="): + val = tok[len("--config-env="):] + case !strings.HasPrefix(tok, "-"): + // Once we hit the subcommand, global options are done. + if tok == "config" { + return true + } + } + + if consumed { + i++ + } + if val == "" { + continue + } + + key, value, _ := strings.Cut(val, "=") + key = strings.ToLower(key) + if strings.HasPrefix(key, "alias.") && strings.HasPrefix(value, "!") { + return true + } + if gitCodeExecConfigKeys[key] { + return true + } + } + return false +} + func isCodeExecution(first string, tokens []string) bool { + // git -c/--config-env can inject arbitrary shell commands via aliases, + // core.pager, core.fsmonitor, credential.helper, etc.; git config writes + // can persist the same payloads. + if first == "git" && isGitCodeExecution(tokens) { + return true + } + // Pipe to shell interpreter for i, tok := range tokens { if tok == "|" && i+1 < len(tokens) && pipedShells[commandName(tokens[i+1])] { @@ -2317,16 +2500,18 @@ func hasArgAfter(tokens []string, after, target string) bool { return false } -// touchesSystemPath reports whether any token names a system path (an +// touchesSystemPath reports whether any token names a sensitive path (an // argument or a redirect target alike). It is intentionally broader than the // redirect-only scan in isSystemWrite — it catches reads/args such as -// `cat /etc/foo` or an unknown tool pointed at /usr — so both checks exist. +// `cat /etc/foo`, `cat ~/.bashrc`, or an unknown tool pointed at /usr — so both +// checks exist. It keeps the legacy trailing-slash system-prefix check and +// additionally uses ClassifyPath for home rc files and trust anchors. func touchesSystemPath(tokens []string) bool { for _, tok := range tokens { if tok == ">" || tok == ">>" { continue } - if isSystemPath(tok) { + if isSystemPath(tok) || shellPathIsHomeSensitive(tok) { return true } } diff --git a/internal/danger/classifier_test.go b/internal/danger/classifier_test.go index cd92bf8..6e34953 100644 --- a/internal/danger/classifier_test.go +++ b/internal/danger/classifier_test.go @@ -715,6 +715,97 @@ func TestClassify_RsyncRemote(t *testing.T) { } } +// TestClassify_GitConfigCodeExecution verifies H-1: git -c/--config-env and +// git config can inject arbitrary shell commands through aliases, pager, and +// credential helpers, so they must classify as code_execution. +func TestClassify_GitConfigCodeExecution(t *testing.T) { + tests := []struct { + cmd string + want RiskClass + }{ + {`git -c alias.x='!id' x`, CodeExecution}, + {`git -c alias.x=!id x`, CodeExecution}, + {`git -calias.x=!id x`, CodeExecution}, + {`git -c core.pager='sh -c id' --paginate log`, CodeExecution}, + {`git -c core.fsmonitor='!pwn' status`, CodeExecution}, + {`git -c credential.helper='!curl evil' clone`, CodeExecution}, + {`git config --global alias.pwn '!cmd'`, CodeExecution}, + {`git config user.email x`, CodeExecution}, + // Benign config overrides stay in their normal class. + {`git -c http.proxy=http://evil fetch origin`, NetworkEgress}, + {`git -C /repo status`, Safe}, + {`git status`, Safe}, + } + for _, tt := range tests { + t.Run(tt.cmd, func(t *testing.T) { + got := Classify(tt.cmd) + if got != tt.want { + t.Errorf("Classify(%q) = %s, want %s", tt.cmd, got, tt.want) + } + }) + } +} + +// TestClassify_FindRsyncDestructive verifies H-2: find -delete and rsync +// --delete / --remove-source-files can wipe directory trees, so they are +// destructive; find -fprint/-fprintf are local writes. +func TestClassify_FindRsyncDestructive(t *testing.T) { + tests := []struct { + cmd string + want RiskClass + }{ + {"find . -delete", Destructive}, + {"find ~ -delete", Destructive}, + {"find . -fprint ~/.bashrc", LocalWrite}, + {"find . -fprintf /tmp/list '%p\\n'", LocalWrite}, + {"rsync -a --delete /tmp/empty/ ~", Destructive}, + {"rsync -av --remove-source-files /a /b", Destructive}, + {"rsync -av --del /a /b", Destructive}, + {"rsync -av /src/ /dst/", Safe}, + {"rsync -av /src/ user@host:/dst/", NetworkEgress}, + } + for _, tt := range tests { + t.Run(tt.cmd, func(t *testing.T) { + got := Classify(tt.cmd) + if got != tt.want { + t.Errorf("Classify(%q) = %s, want %s", tt.cmd, got, tt.want) + } + }) + } +} + +// TestClassify_ShellRCTargets verifies H-3: shell writes to shell rc files +// and other home-sensitive paths are escalated to system_write using the same +// ClassifyPath lists that gate the file tools. +func TestClassify_ShellRCTargets(t *testing.T) { + tests := []struct { + cmd string + want RiskClass + }{ + {"echo x >> ~/.bashrc", SystemWrite}, + {"echo x >> ~/.zshrc", SystemWrite}, + {"echo x >> ~/.profile", SystemWrite}, + {"echo x >> $HOME/.bashrc", SystemWrite}, + {"cp evil ~/.profile", SystemWrite}, + {"tee -a ~/.zshrc", SystemWrite}, + {"dd if=evil of=~/.bashrc", SystemWrite}, + {"cat ~/.bashrc", SystemWrite}, + {"cat ~/.ssh/id_rsa", SystemWrite}, + {"cat ~/.odek/config.json", SystemWrite}, + // Non-rc home paths stay local_write. + {"mv evil ~/.local/bin/git", LocalWrite}, + {"echo x > ~/notes.txt", LocalWrite}, + } + for _, tt := range tests { + t.Run(tt.cmd, func(t *testing.T) { + got := Classify(tt.cmd) + if got != tt.want { + t.Errorf("Classify(%q) = %s, want %s", tt.cmd, got, tt.want) + } + }) + } +} + func TestClassify_PythonDashC(t *testing.T) { got := Classify("python -c 'print(1)'") if got != CodeExecution {