From 85367a962ff0b806930b8e57976e45e0300b02a1 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sat, 18 Jul 2026 21:40:57 +0200 Subject: [PATCH] fix(ssrf): refuse outbound requests through HTTP(S)_PROXY The SSRF dial guard validates the address the transport dials. When a proxy is configured, the transport dials the proxy and sends the real target inside the CONNECT/request envelope, so the guard only validated the proxy and internal/rebound targets could slip through. - Wrap tr.Proxy in ssrfGuardedTransport so that any request whose proxy resolution returns a proxy URL is refused with a clear error. - Log a one-time warning when proxy env vars are detected. - Add TestSSRFGuardedTransport_RefusesProxy regression test. - Document the proxy refusal in SECURITY.md and AGENTS.md. Closes L-6 in sec_findings.md. --- AGENTS.md | 1 + cmd/odek/ssrf_guard.go | 43 +++++++++++++++++++++++++++++++++++++ cmd/odek/ssrf_guard_test.go | 27 +++++++++++++++++++++++ docs/SECURITY.md | 2 ++ 4 files changed, 73 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index f83d899..702861d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -116,6 +116,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU - **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. +- **SSRF guard proxy refusal** (`cmd/odek/ssrf_guard.go`) — when `HTTP(S)_PROXY` is set, the transport would dial the proxy instead of the target and the dial guard would only validate the proxy address. `ssrfGuardedTransport` detects an active proxy, logs a warning, and refuses the request so SSRF protection is not silently voided. - **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. Every `/api/*` endpoint additionally requires the per-instance `odek_ws_token` (cookie or `X-Odek-Ws-Token` header) and rejects non-loopback `Host` headers, closing DNS-rebinding reads of `/api/sessions`, `/api/resources`, and `/api/models`. - **Browser history cap** (`cmd/odek/browser_tool.go`) — navigation history is capped at 50 snapshots to prevent memory DoS from repeated `browser_navigate` calls. - **Browser element cap** (`cmd/odek/browser_tool.go`) — the number of interactive elements extracted per page is capped at 500 so a hostile page cannot OOM the agent with thousands of links or buttons. diff --git a/cmd/odek/ssrf_guard.go b/cmd/odek/ssrf_guard.go index 50a9b9a..1ff58c7 100644 --- a/cmd/odek/ssrf_guard.go +++ b/cmd/odek/ssrf_guard.go @@ -5,11 +5,18 @@ import ( "fmt" "net" "net/http" + "net/url" + "os" + "sync" "time" "github.com/BackendStack21/odek/internal/danger" ) +// proxyWarnOnce ensures the proxy-disabled warning is logged only once per +// process even though ssrfGuardedTransport is called for several tools. +var proxyWarnOnce sync.Once + // SSRF / DNS-rebinding dial guard. // // danger.ClassifyURL inspects only the literal hostname of a URL, so a domain @@ -116,6 +123,42 @@ func ssrfGuardedTransport(allowedHosts ...string) *http.Transport { } else { tr = &http.Transport{Proxy: http.ProxyFromEnvironment} } + + // Proxies bypass the dial-layer SSRF guard: the transport dials the proxy, + // and the target address is sent inside the CONNECT/request envelope. An + // attacker-controlled proxy (or a rebinding target reachable through a + // legitimate proxy) would therefore defeat the guard. Fail closed: if a proxy + // is configured for a request, refuse it rather than silently void protection. + if tr.Proxy != nil { + if proxyEnvSet() { + proxyWarnOnce.Do(func() { + fmt.Fprintf(os.Stderr, "warning: HTTP(S)_PROXY is set but odek's SSRF guard cannot validate target addresses through a proxy; proxy routing is disabled for outbound tool traffic\n") + }) + } + originalProxy := tr.Proxy + tr.Proxy = func(req *http.Request) (*url.URL, error) { + proxyURL, err := originalProxy(req) + if err != nil { + return nil, err + } + if proxyURL != nil { + return nil, fmt.Errorf("refusing request through HTTP(S)_PROXY: SSRF guard cannot validate target address via a proxy") + } + return nil, nil + } + } + tr.DialContext = ssrfGuardedDial(base.DialContext, net.DefaultResolver.LookupIPAddr, allowedHosts...) return tr } + +// proxyEnvSet reports whether any of the standard HTTP(S)_PROXY environment +// variables are set, so callers can warn that outbound proxying is disabled. +func proxyEnvSet() bool { + for _, v := range []string{"HTTP_PROXY", "HTTPS_PROXY", "http_proxy", "https_proxy", "NO_PROXY", "no_proxy"} { + if os.Getenv(v) != "" { + return true + } + } + return false +} diff --git a/cmd/odek/ssrf_guard_test.go b/cmd/odek/ssrf_guard_test.go index 2b892ad..0f6e979 100644 --- a/cmd/odek/ssrf_guard_test.go +++ b/cmd/odek/ssrf_guard_test.go @@ -4,6 +4,7 @@ import ( "context" "net" "net/http" + "net/url" "strings" "testing" @@ -225,6 +226,32 @@ func TestWebSearch_SSRF_ResolvesInternal(t *testing.T) { // TestSSRFGuardedTransport_Installed is a guard against regressions that would // silently drop the SSRF protection from the production constructors. +func TestSSRFGuardedTransport_RefusesProxy(t *testing.T) { + // Replace the default transport with one whose Proxy function always returns + // a proxy URL. This avoids depending on the test environment's proxy env + // vars and proves the guard refuses proxy-routed requests regardless of how + // the proxy was configured. + orig := http.DefaultTransport + http.DefaultTransport = &http.Transport{ + Proxy: http.ProxyURL(&url.URL{Scheme: "http", Host: "proxy.example.com:8080"}), + } + defer func() { http.DefaultTransport = orig }() + + tr := ssrfGuardedTransport() + client := &http.Client{Transport: tr} + + resp, err := client.Get("http://example.com/") + if err == nil { + if resp != nil { + resp.Body.Close() + } + t.Fatal("expected request to be refused when a proxy is configured") + } + if !strings.Contains(err.Error(), "refusing request through HTTP(S)_PROXY") { + t.Errorf("error %q should mention proxy refusal", err) + } +} + func TestSSRFGuardedTransport_Installed(t *testing.T) { b := newBrowserTool(danger.DangerousConfig{}) if b.client.Transport == nil { diff --git a/docs/SECURITY.md b/docs/SECURITY.md index a6b8dab..6174506 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -394,6 +394,8 @@ client := &http.Client{ There is no user-facing allowlist config field today; the list is derived from each tool's own operator-controlled `base_url`. If you need a broader or user-editable allowlist, add a `dangerous.ssrf_allowed_hosts` (or `network.allowed_hosts`) array to the config and merge it into the set passed to `ssrfGuardedTransport`. +When `HTTP(S)_PROXY` is set, the transport would dial the proxy address instead of the target, so the dial-time guard would validate only the proxy and the real target could be an internal/rebound address. `ssrfGuardedTransport` detects an active proxy and refuses the request with a clear error rather than silently disabling SSRF protection. Outbound tool traffic therefore requires direct connections. + ### 18b. CGNAT and benchmark IP blocking Go's `net.IP.IsPrivate()` covers RFC1918 and RFC4193 private ranges, but it does not cover RFC 6598 CGNAT (`100.64.0.0/10`) or the RFC 2544 benchmark-testing range (`198.18.0.0/15`). Tailscale and similar overlay networks use `100.64/10` addresses, so an attacker who could steer the agent to `http://100.x.x.x` (or a hostname that rebinding resolves to such an address) could reach unauthenticated internal services that the operator expected to be unreachable from the agent.