From cc0c0cf03e3ebdd1377aaa0b8ad6c0b39e880955 Mon Sep 17 00:00:00 2001 From: Nebez Briefkani Date: Sat, 13 Jan 2024 12:46:37 -0800 Subject: [PATCH] caddyhttp: Security enhancements for client IP parsing (#5805) Co-authored-by: Francis Lavoie --- caddyconfig/httpcaddyfile/serveroptions.go | 8 + modules/caddyhttp/server.go | 57 +++- modules/caddyhttp/server_test.go | 292 +++++++++++++++++++++ 3 files changed, 352 insertions(+), 5 deletions(-) diff --git a/caddyconfig/httpcaddyfile/serveroptions.go b/caddyconfig/httpcaddyfile/serveroptions.go index 6d7c6787..c131a641 100644 --- a/caddyconfig/httpcaddyfile/serveroptions.go +++ b/caddyconfig/httpcaddyfile/serveroptions.go @@ -46,6 +46,7 @@ type serverOptions struct { Protocols []string StrictSNIHost *bool TrustedProxiesRaw json.RawMessage + TrustedProxiesStrict int ClientIPHeaders []string ShouldLogCredentials bool Metrics *caddyhttp.Metrics @@ -217,6 +218,12 @@ func unmarshalCaddyfileServerOptions(d *caddyfile.Dispenser) (any, error) { ) serverOpts.TrustedProxiesRaw = jsonSource + case "trusted_proxies_strict": + if d.NextArg() { + return nil, d.ArgErr() + } + serverOpts.TrustedProxiesStrict = 1 + case "client_ip_headers": headers := d.RemainingArgs() for _, header := range headers { @@ -340,6 +347,7 @@ func applyServerOptions( server.StrictSNIHost = opts.StrictSNIHost server.TrustedProxiesRaw = opts.TrustedProxiesRaw server.ClientIPHeaders = opts.ClientIPHeaders + server.TrustedProxiesStrict = opts.TrustedProxiesStrict server.Metrics = opts.Metrics if opts.ShouldLogCredentials { if server.Logs == nil { diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go index d060738f..bece8747 100644 --- a/modules/caddyhttp/server.go +++ b/modules/caddyhttp/server.go @@ -173,6 +173,19 @@ type Server struct { // remote IP address. ClientIPHeaders []string `json:"client_ip_headers,omitempty"` + // If greater than zero, enables strict ClientIPHeaders + // (default X-Forwarded-For) parsing. If enabled, the + // ClientIPHeaders will be parsed from right to left, and + // the first value that is both valid and doesn't match the + // trusted proxy list will be used as client IP. If zero, + // the ClientIPHeaders will be parsed from left to right, + // and the first value that is a valid IP address will be + // used as client IP. + // + // This depends on `trusted_proxies` being configured. + // This option is disabled by default. + TrustedProxiesStrict int `json:"trusted_proxies_strict,omitempty"` + // Enables access logging and configures how access logs are handled // in this server. To minimally enable access logs, simply set this // to a non-null, empty struct. @@ -839,17 +852,28 @@ func determineTrustedProxy(r *http.Request, s *Server) (bool, string) { if s.trustedProxies == nil { return false, ipAddr.String() } - for _, ipRange := range s.trustedProxies.GetIPRanges(r) { - if ipRange.Contains(ipAddr) { - // We trust the proxy, so let's try to - // determine the real client IP - return true, trustedRealClientIP(r, s.ClientIPHeaders, ipAddr.String()) + + if isTrustedClientIP(ipAddr, s.trustedProxies.GetIPRanges(r)) { + if s.TrustedProxiesStrict > 0 { + return true, strictUntrustedClientIp(r, s.ClientIPHeaders, s.trustedProxies.GetIPRanges(r), ipAddr.String()) } + return true, trustedRealClientIP(r, s.ClientIPHeaders, ipAddr.String()) } return false, ipAddr.String() } +// isTrustedClientIP returns true if the given IP address is +// in the list of trusted IP ranges. +func isTrustedClientIP(ipAddr netip.Addr, trusted []netip.Prefix) bool { + for _, ipRange := range trusted { + if ipRange.Contains(ipAddr) { + return true + } + } + return false +} + // trustedRealClientIP finds the client IP from the request assuming it is // from a trusted client. If there is no client IP headers, then the // direct remote address is returned. If there are client IP headers, @@ -884,6 +908,29 @@ func trustedRealClientIP(r *http.Request, headers []string, clientIP string) str return clientIP } +// strictUntrustedClientIp iterates through the list of client IP headers, +// parses them from right-to-left, and returns the first valid IP address +// that is untrusted. If no valid IP address is found, then the direct +// remote address is returned. +func strictUntrustedClientIp(r *http.Request, headers []string, trusted []netip.Prefix, clientIP string) string { + for _, headerName := range headers { + ips := strings.Split(strings.Join(r.Header.Values(headerName), ","), ",") + + for i := len(ips) - 1; i >= 0; i-- { + ip, _, _ := strings.Cut(strings.TrimSpace(ips[i]), "%") + ipAddr, err := netip.ParseAddr(ip) + if err != nil { + continue + } + if !isTrustedClientIP(ipAddr, trusted) { + return ipAddr.String() + } + } + } + + return clientIP +} + // cloneURL makes a copy of r.URL and returns a // new value that doesn't reference the original. func cloneURL(from, to *url.URL) { diff --git a/modules/caddyhttp/server_test.go b/modules/caddyhttp/server_test.go index 96a241ba..fd0e1e34 100644 --- a/modules/caddyhttp/server_test.go +++ b/modules/caddyhttp/server_test.go @@ -6,6 +6,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/netip" "testing" "time" @@ -144,3 +145,294 @@ func BenchmarkServer_LogRequest_WithTraceID(b *testing.B) { s.logRequest(accLog, req, wrec, &duration, repl, bodyReader, false) } } +func TestServer_TrustedRealClientIP_NoTrustedHeaders(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "192.0.2.1:12345" + ip := trustedRealClientIP(req, []string{}, "192.0.2.1") + + assert.Equal(t, ip, "192.0.2.1") +} + +func TestServer_TrustedRealClientIP_OneTrustedHeaderEmpty(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "192.0.2.1:12345" + ip := trustedRealClientIP(req, []string{"X-Forwarded-For"}, "192.0.2.1") + + assert.Equal(t, ip, "192.0.2.1") +} + +func TestServer_TrustedRealClientIP_OneTrustedHeaderInvalid(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "192.0.2.1:12345" + req.Header.Set("X-Forwarded-For", "not, an, ip") + ip := trustedRealClientIP(req, []string{"X-Forwarded-For"}, "192.0.2.1") + + assert.Equal(t, ip, "192.0.2.1") +} + +func TestServer_TrustedRealClientIP_OneTrustedHeaderValid(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "192.0.2.1:12345" + req.Header.Set("X-Forwarded-For", "10.0.0.1") + ip := trustedRealClientIP(req, []string{"X-Forwarded-For"}, "192.0.2.1") + + assert.Equal(t, ip, "10.0.0.1") +} + +func TestServer_TrustedRealClientIP_OneTrustedHeaderValidArray(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "192.0.2.1:12345" + req.Header.Set("X-Forwarded-For", "1.1.1.1, 2.2.2.2, 3.3.3.3") + ip := trustedRealClientIP(req, []string{"X-Forwarded-For"}, "192.0.2.1") + + assert.Equal(t, ip, "1.1.1.1") +} + +func TestServer_TrustedRealClientIP_SkipsInvalidIps(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "192.0.2.1:12345" + req.Header.Set("X-Forwarded-For", "not an ip, bad bad, 10.0.0.1") + ip := trustedRealClientIP(req, []string{"X-Forwarded-For"}, "192.0.2.1") + + assert.Equal(t, ip, "10.0.0.1") +} + +func TestServer_TrustedRealClientIP_MultipleTrustedHeaderValidArray(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "192.0.2.1:12345" + req.Header.Set("Real-Client-IP", "1.1.1.1, 2.2.2.2, 3.3.3.3") + req.Header.Set("X-Forwarded-For", "3.3.3.3, 4.4.4.4") + ip1 := trustedRealClientIP(req, []string{"X-Forwarded-For", "Real-Client-IP"}, "192.0.2.1") + ip2 := trustedRealClientIP(req, []string{"Real-Client-IP", "X-Forwarded-For"}, "192.0.2.1") + ip3 := trustedRealClientIP(req, []string{"Missing-Header-IP", "Real-Client-IP", "X-Forwarded-For"}, "192.0.2.1") + + assert.Equal(t, ip1, "3.3.3.3") + assert.Equal(t, ip2, "1.1.1.1") + assert.Equal(t, ip3, "1.1.1.1") +} + +func TestServer_DetermineTrustedProxy_NoConfig(t *testing.T) { + server := &Server{} + + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "192.0.2.1:12345" + + trusted, clientIP := determineTrustedProxy(req, server) + + assert.False(t, trusted) + assert.Equal(t, clientIP, "192.0.2.1") +} + +func TestServer_DetermineTrustedProxy_NoConfigIpv6(t *testing.T) { + server := &Server{} + + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "[::1]:12345" + + trusted, clientIP := determineTrustedProxy(req, server) + + assert.False(t, trusted) + assert.Equal(t, clientIP, "::1") +} + +func TestServer_DetermineTrustedProxy_NoConfigIpv6Zones(t *testing.T) { + server := &Server{} + + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "[::1%eth2]:12345" + + trusted, clientIP := determineTrustedProxy(req, server) + + assert.False(t, trusted) + assert.Equal(t, clientIP, "::1") +} + +func TestServer_DetermineTrustedProxy_TrustedLoopback(t *testing.T) { + loopbackPrefix, _ := netip.ParsePrefix("127.0.0.1/8") + + server := &Server{ + trustedProxies: &StaticIPRange{ + ranges: []netip.Prefix{loopbackPrefix}, + }, + ClientIPHeaders: []string{"X-Forwarded-For"}, + } + + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "127.0.0.1:12345" + req.Header.Set("X-Forwarded-For", "31.40.0.10") + + trusted, clientIP := determineTrustedProxy(req, server) + + assert.True(t, trusted) + assert.Equal(t, clientIP, "31.40.0.10") +} + +func TestServer_DetermineTrustedProxy_UntrustedPrefix(t *testing.T) { + loopbackPrefix, _ := netip.ParsePrefix("127.0.0.1/8") + + server := &Server{ + trustedProxies: &StaticIPRange{ + ranges: []netip.Prefix{loopbackPrefix}, + }, + ClientIPHeaders: []string{"X-Forwarded-For"}, + } + + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "10.0.0.1:12345" + req.Header.Set("X-Forwarded-For", "31.40.0.10") + + trusted, clientIP := determineTrustedProxy(req, server) + + assert.False(t, trusted) + assert.Equal(t, clientIP, "10.0.0.1") +} + +func TestServer_DetermineTrustedProxy_MultipleTrustedPrefixes(t *testing.T) { + loopbackPrefix, _ := netip.ParsePrefix("127.0.0.1/8") + localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8") + + server := &Server{ + trustedProxies: &StaticIPRange{ + ranges: []netip.Prefix{loopbackPrefix, localPrivatePrefix}, + }, + ClientIPHeaders: []string{"X-Forwarded-For"}, + } + + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "10.0.0.1:12345" + req.Header.Set("X-Forwarded-For", "31.40.0.10") + + trusted, clientIP := determineTrustedProxy(req, server) + + assert.True(t, trusted) + assert.Equal(t, clientIP, "31.40.0.10") +} + +func TestServer_DetermineTrustedProxy_MultipleTrustedClientHeaders(t *testing.T) { + loopbackPrefix, _ := netip.ParsePrefix("127.0.0.1/8") + localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8") + + server := &Server{ + trustedProxies: &StaticIPRange{ + ranges: []netip.Prefix{loopbackPrefix, localPrivatePrefix}, + }, + ClientIPHeaders: []string{"CF-Connecting-IP", "X-Forwarded-For"}, + } + + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "10.0.0.1:12345" + req.Header.Set("CF-Connecting-IP", "1.1.1.1, 2.2.2.2") + req.Header.Set("X-Forwarded-For", "3.3.3.3, 4.4.4.4") + + trusted, clientIP := determineTrustedProxy(req, server) + + assert.True(t, trusted) + assert.Equal(t, clientIP, "1.1.1.1") +} + +func TestServer_DetermineTrustedProxy_MatchLeftMostValidIp(t *testing.T) { + localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8") + + server := &Server{ + trustedProxies: &StaticIPRange{ + ranges: []netip.Prefix{localPrivatePrefix}, + }, + ClientIPHeaders: []string{"X-Forwarded-For"}, + } + + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "10.0.0.1:12345" + req.Header.Set("X-Forwarded-For", "30.30.30.30, 45.54.45.54, 10.0.0.1") + + trusted, clientIP := determineTrustedProxy(req, server) + + assert.True(t, trusted) + assert.Equal(t, clientIP, "30.30.30.30") +} + +func TestServer_DetermineTrustedProxy_MatchRightMostUntrusted(t *testing.T) { + localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8") + + server := &Server{ + trustedProxies: &StaticIPRange{ + ranges: []netip.Prefix{localPrivatePrefix}, + }, + ClientIPHeaders: []string{"X-Forwarded-For"}, + TrustedProxiesStrict: 1, + } + + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "10.0.0.1:12345" + req.Header.Set("X-Forwarded-For", "30.30.30.30, 45.54.45.54, 10.0.0.1") + + trusted, clientIP := determineTrustedProxy(req, server) + + assert.True(t, trusted) + assert.Equal(t, clientIP, "45.54.45.54") +} + +func TestServer_DetermineTrustedProxy_MatchRightMostUntrustedSkippingEmpty(t *testing.T) { + localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8") + + server := &Server{ + trustedProxies: &StaticIPRange{ + ranges: []netip.Prefix{localPrivatePrefix}, + }, + ClientIPHeaders: []string{"Missing-Header", "CF-Connecting-IP", "X-Forwarded-For"}, + TrustedProxiesStrict: 1, + } + + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "10.0.0.1:12345" + req.Header.Set("CF-Connecting-IP", "not a real IP") + req.Header.Set("X-Forwarded-For", "30.30.30.30, bad, 45.54.45.54, not real") + + trusted, clientIP := determineTrustedProxy(req, server) + + assert.True(t, trusted) + assert.Equal(t, clientIP, "45.54.45.54") +} + +func TestServer_DetermineTrustedProxy_MatchRightMostUntrustedSkippingTrusted(t *testing.T) { + localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8") + + server := &Server{ + trustedProxies: &StaticIPRange{ + ranges: []netip.Prefix{localPrivatePrefix}, + }, + ClientIPHeaders: []string{"CF-Connecting-IP", "X-Forwarded-For"}, + TrustedProxiesStrict: 1, + } + + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "10.0.0.1:12345" + req.Header.Set("CF-Connecting-IP", "10.0.0.1, 10.0.0.2, 10.0.0.3") + req.Header.Set("X-Forwarded-For", "30.30.30.30, 45.54.45.54, 10.0.0.4") + + trusted, clientIP := determineTrustedProxy(req, server) + + assert.True(t, trusted) + assert.Equal(t, clientIP, "45.54.45.54") +} + +func TestServer_DetermineTrustedProxy_MatchRightMostUntrustedFirst(t *testing.T) { + localPrivatePrefix, _ := netip.ParsePrefix("10.0.0.0/8") + + server := &Server{ + trustedProxies: &StaticIPRange{ + ranges: []netip.Prefix{localPrivatePrefix}, + }, + ClientIPHeaders: []string{"CF-Connecting-IP", "X-Forwarded-For"}, + TrustedProxiesStrict: 1, + } + + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "10.0.0.1:12345" + req.Header.Set("CF-Connecting-IP", "10.0.0.1, 90.100.110.120, 10.0.0.2, 10.0.0.3") + req.Header.Set("X-Forwarded-For", "30.30.30.30, 45.54.45.54, 10.0.0.4") + + trusted, clientIP := determineTrustedProxy(req, server) + + assert.True(t, trusted) + assert.Equal(t, clientIP, "90.100.110.120") +}