From f63efc6d888e616959272a8c99e825925196ce38 Mon Sep 17 00:00:00 2001 From: fen Date: Thu, 17 Sep 2026 20:29:40 -0500 Subject: [PATCH] Fix rate limiter bypass via client-controlled X-Forwarded-For (#280) clientIP() keyed rate-limit buckets on the rightmost X-Forwarded-For entry, assuming traefik appends the real client IP. The deployed ingress does not rewrite XFF, so rotating the header gave a fresh bucket per request (pentest H1: 8 creates with rotating XFF -> 6x201). Now the bucket keys on the actual peer address (RemoteAddr) by default; every client-supplied IP header is ignored. Deployments whose ingress overwrites a client-IP header can opt in via PALETTE_TRUSTED_IP_HEADER (e.g. CF-Connecting-IP behind Cloudflare) to restore per-client limits. Adds tests: rotating XFF no longer resets the bucket; the trusted header is honored only when explicitly configured. --- README.md | 1 + internal/api/clientip.go | 57 ++++++++++++++++ internal/api/ratelimit.go | 30 --------- internal/api/ratelimit_xff_test.go | 104 +++++++++++------------------ internal/api/server.go | 5 ++ 5 files changed, 101 insertions(+), 96 deletions(-) create mode 100644 internal/api/clientip.go diff --git a/README.md b/README.md index 342092c..43f12d7 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,7 @@ go build -o palette ./cmd/palette | `PALETTE_ADMIN_KEY` | generated | Admin key; if unset a 32-char hex key is generated and persisted to `/admin-key` (0600) | | `PALETTE_DEFAULT_DARK` | dark on | Default dark mode for new visitors. Set `false`, `0`, or `off` to default to light mode. Visitors who toggle dark mode keep their choice in their browser. | | `PALETTE_UNLOCK_SECRET` | random per start | HMAC secret for password-unlock cookies. Set a fixed value to keep unlock sessions across restarts or across replicas. | +| `PALETTE_TRUSTED_IP_HEADER` | unset | Name of a proxy-controlled client-IP header to key API rate limits on (e.g. `CF-Connecting-IP` when Cloudflare is the ingress; Cloudflare strips any client-supplied value). Unset: rate limits key on the peer address only, and all client-supplied IP headers (X-Forwarded-For, X-Real-Ip) are ignored. (#280) | An `/admin` page exists for runtime settings, protected by a key set at install (`PALETTE_ADMIN_KEY` env var) and resettable locally. See diff --git a/internal/api/clientip.go b/internal/api/clientip.go new file mode 100644 index 0000000..82ae13b --- /dev/null +++ b/internal/api/clientip.go @@ -0,0 +1,57 @@ +// clientIP extracts the client IP for rate-limit keying. +// +// Trust boundary (issue #280): the bucket key MUST NOT come from any header a +// client can influence. The previous rightmost-X-Forwarded-For scheme (#85) +// assumed Traefik appends the real client IP, but the deployed ingress does +// not rewrite XFF, so a client rotating its own XFF value got a fresh bucket +// per request and the limit was unenforceable (pentest H1: 6x201 across 8 +// rotating-XFF creates). +// +// Default: key on the actual peer address (RemoteAddr) only. Behind any +// reverse proxy this is the proxy's address, so all clients share one bucket +// per endpoint — coarse, but safe. +// +// Proxy-honoring mode: a deployment in front of a proxy that OVERWRITES (not +// appends to) a client-IP header can set PALETTE_TRUSTED_IP_HEADER (e.g. +// CF-Connecting-IP when Cloudflare is the ingress; Cloudflare strips any +// client-supplied value). The header is honored ONLY when explicitly +// configured at startup, and X-Forwarded-For / X-Real-Ip are never trusted. +package api + +import ( + "net" + "net/http" + "sync" +) + +var ( + trustedIPMu sync.RWMutex + trustedIPHeader string // empty = never trust any client-IP header +) + +// SetTrustedIPHeader configures the single proxy-controlled header whose +// value may key rate-limit buckets. Called at startup; tests may reset it. +func SetTrustedIPHeader(name string) { + trustedIPMu.Lock() + defer trustedIPMu.Unlock() + trustedIPHeader = name +} + +func getTrustedIPHeader() string { + trustedIPMu.RLock() + defer trustedIPMu.RUnlock() + return trustedIPHeader +} + +func clientIP(r *http.Request) string { + if name := getTrustedIPHeader(); name != "" { + if v := r.Header.Get(name); v != "" { + return v + } + } + host := r.RemoteAddr + if h, _, err := net.SplitHostPort(r.RemoteAddr); err == nil { + host = h + } + return host +} diff --git a/internal/api/ratelimit.go b/internal/api/ratelimit.go index 7c44bd6..b01f9a2 100644 --- a/internal/api/ratelimit.go +++ b/internal/api/ratelimit.go @@ -3,7 +3,6 @@ package api import ( "net/http" "strconv" - "strings" "sync" "time" ) @@ -48,35 +47,6 @@ func (l *limiter) allow(key string, rate, burst float64) bool { return true } -// clientIP extracts the client IP for rate-limit keying (#85). -// -// Trust boundary: palette runs behind exactly ONE trusted reverse proxy -// (Traefik in the k3s pod network). Traefik APPENDS the real client IP to -// X-Forwarded-For, so the RIGHTMOST entry is the last value the trusted -// proxy observed and is unspoofable by the client (a client-supplied fake -// entry only lands on the LEFT and is ignored). This matches chi's -// middleware.RealIP semantics for a single trusted proxy hop. -// -// Direct connections (no XFF header) fall back to RemoteAddr. Directly -// reachable deployments must NOT expose the app to untrusted networks -// without a proxy in front, or attackers could forge the rightmost entry. -func clientIP(r *http.Request) string { - if xff := r.Header.Get("X-Forwarded-For"); xff != "" { - if i := strings.LastIndex(xff, ","); i >= 0 { - return strings.TrimSpace(xff[i+1:]) - } - return strings.TrimSpace(xff) - } - if xr := r.Header.Get("X-Real-Ip"); xr != "" { - return strings.TrimSpace(xr) - } - host := r.RemoteAddr - if i := strings.LastIndex(host, ":"); i > 0 { - host = host[:i] - } - return host -} - var globalLimiter = newLimiter() // globalSettingsFn is set at startup; tests can point it at fixed settings. diff --git a/internal/api/ratelimit_xff_test.go b/internal/api/ratelimit_xff_test.go index 8674636..680d5d0 100644 --- a/internal/api/ratelimit_xff_test.go +++ b/internal/api/ratelimit_xff_test.go @@ -1,85 +1,57 @@ package api -// Issue #85: the rate limit key must use the rightmost X-Forwarded-For entry -// (appended by the trusted Traefik proxy), never the raw/leftmost header -// value a client can forge. A spoofed FIRST XFF entry must not bypass the -// limit or rotate buckets. - import ( - "bytes" + "fmt" "net/http/httptest" "testing" ) -func TestClientIPTakesRightmostXFF(t *testing.T) { - r := httptest.NewRequest("POST", "/", nil) - r.RemoteAddr = "10.42.0.7:51000" // trusted Traefik pod +func TestClientIPUsesRemoteAddrNotXFF(t *testing.T) { + SetTrustedIPHeader("") + defer SetTrustedIPHeader("") + r := httptest.NewRequest("POST", "/api/pastes", nil) + r.RemoteAddr = "203.0.113.7:4432" r.Header.Set("X-Forwarded-For", "1.2.3.4, 1.2.3.5, 203.0.113.9") - if got := clientIP(r); got != "203.0.113.9" { - t.Fatalf("clientIP = %q, want rightmost 203.0.113.9", got) - } -} - -func TestClientIPXRealIPFallback(t *testing.T) { - r := httptest.NewRequest("POST", "/", nil) - r.RemoteAddr = "10.42.0.7:51000" r.Header.Set("X-Real-Ip", "203.0.113.10") - if got := clientIP(r); got != "203.0.113.10" { - t.Fatalf("clientIP = %q, want 203.0.113.10", got) + if got := clientIP(r); got != "203.0.113.7" { + t.Fatalf("clientIP = %q, want peer 203.0.113.7", got) } } -func TestClientIPDirectFallback(t *testing.T) { - r := httptest.NewRequest("POST", "/", nil) - r.RemoteAddr = "198.51.100.5:51000" - if got := clientIP(r); got != "198.51.100.5" { - t.Fatalf("clientIP = %q, want 198.51.100.5", got) +func TestClientIPTrustedHeaderOnlyWhenConfigured(t *testing.T) { + SetTrustedIPHeader("") + defer SetTrustedIPHeader("") + r := httptest.NewRequest("POST", "/api/pastes", nil) + r.RemoteAddr = "10.0.1.47:9999" + r.Header.Set("CF-Connecting-IP", "198.51.100.9") + if got := clientIP(r); got != "10.0.1.47" { + t.Fatalf("unconfigured: clientIP = %q, want peer 10.0.1.47", got) + } + SetTrustedIPHeader("CF-Connecting-IP") + if got := clientIP(r); got != "198.51.100.9" { + t.Fatalf("configured: clientIP = %q, want CF-Connecting-IP value", got) } } -// TestRateLimitSpoofedFirstXFFDoesNotBypass: an attacker rotating a fake -// leftmost XFF entry stays limited on their real (rightmost) IP. -func TestRateLimitSpoofedFirstXFFDoesNotBypass(t *testing.T) { - srv := newTestServer(t) - h := srv.routes() - for i := 0; i < 5; i++ { - req := httptest.NewRequest("POST", "/api/pastes", bytes.NewReader([]byte(`{"content":"hi"}`))) - req.RemoteAddr = "10.42.0.7:51000" - // each request spoofs a DIFFERENT leftmost entry - req.Header.Set("X-Forwarded-For", spoofN(i)+", 203.0.113.9") - rr := httptest.NewRecorder() - h.ServeHTTP(rr, req) - if rr.Code != 201 { - t.Fatalf("req %d: want 201, got %d", i, rr.Code) +// Issue #280: rotating X-Forwarded-For must NOT reset the bucket. Pentest +// repro was 8 creates with rotating XFF -> 6x201. +func TestRotatingXFFDoesNotResetBucket(t *testing.T) { + globalLimiter = newLimiter() + defer SetTrustedIPHeader("") + SetTrustedIPHeader("") + s := defaultSettings(Config{}) // burst/limit defaults; any header values are ignored anyway + var allowed, limited int + for i := 0; i < 8; i++ { + r := httptest.NewRequest("POST", "/api/pastes", nil) + r.RemoteAddr = "198.51.100.1:5000" + r.Header.Set("X-Forwarded-For", fmt.Sprintf("9.9.9.%d", i)) + if rateLimitCreate(r, s) { + allowed++ + } else { + limited++ } } - // 6th request, still the same real IP, new spoofed prefix: must 429 - req := httptest.NewRequest("POST", "/api/pastes", bytes.NewReader([]byte(`{"content":"hi"}`))) - req.RemoteAddr = "10.42.0.7:51000" - req.Header.Set("X-Forwarded-For", "9.9.9.9, 203.0.113.9") - rr := httptest.NewRecorder() - h.ServeHTTP(rr, req) - if rr.Code != 429 { - t.Fatalf("spoofed 6th req: want 429, got %d", rr.Code) - } -} - -func spoofN(i int) string { - return "1.2.3." + string(rune('0'+i)) -} - -// Distinct real IPs must still get distinct buckets (no over-limiting). -func TestRateLimitDistinctRightmostIPsIndependent(t *testing.T) { - srv := newTestServer(t) - h := srv.routes() - for _, ip := range []string{"203.0.113.20", "203.0.113.21"} { - req := httptest.NewRequest("POST", "/api/pastes", bytes.NewReader([]byte(`{"content":"hi"}`))) - req.RemoteAddr = "10.42.0.7:51000" - req.Header.Set("X-Forwarded-For", "6.6.6.6, "+ip) - rr := httptest.NewRecorder() - h.ServeHTTP(rr, req) - if rr.Code != 201 { - t.Fatalf("ip %s: want 201, got %d", ip, rr.Code) - } + if float64(allowed) != s.RateLimitBurst || limited != 8-int(s.RateLimitBurst) { + t.Fatalf("rotating XFF: allowed=%d limited=%d, want allowed=%v (burst), limited=%d", allowed, limited, s.RateLimitBurst, 8-int(s.RateLimitBurst)) } } diff --git a/internal/api/server.go b/internal/api/server.go index b36a714..6a61094 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -28,6 +28,10 @@ type Config struct { DBPath string MaxTextBytes int64 MaxItemBytes int64 + // TrustedIPHeader optionally names a proxy-controlled client-IP header + // (e.g. CF-Connecting-IP behind Cloudflare) to key rate limits on. Empty + // (default) keys on the peer address only. See clientip.go (#280). + TrustedIPHeader string } type apiServer struct { @@ -39,6 +43,7 @@ type apiServer struct { } func NewServer(st *store.Store, cfg Config, ui *web.UI, ss *settingsStore, adminKey string) *apiServer { + SetTrustedIPHeader(cfg.TrustedIPHeader) // #280 return &apiServer{store: st, cfg: cfg, ui: ui, settings: ss, adminKey: adminKey} } -- 2.54.0