diff --git a/CHANGELOG.md b/CHANGELOG.md index 788e82c2..199e7355 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ ## Changes since v7.15.3 +- security: harden `X-Forwarded-For` (and other `--real-client-ip-header` values) parsing so that, once `--trusted-proxy-ip` is configured, the client IP is resolved by walking the hop chain from the newest (rightmost) entry and skipping known trusted proxies, instead of trusting the client-supplied leftmost value. This closes a spoofing gap where a client could set the header to an IP on the `--trusted-ip` allowlist and have it trusted even though a legitimate reverse proxy only appended (rather than replaced) the header. + # V7.15.3 ## Release Highlights diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index 965953fa..9673d354 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -195,6 +195,8 @@ Provider specific options can be found on their respective subpages. :::warning When `--reverse-proxy` is enabled, configure `--trusted-proxy-ip` to the IPs or CIDR ranges of the reverse proxies that are allowed to send `X-Forwarded-*` headers. If you leave it unset, OAuth2 Proxy currently trusts all source IPs for backwards compatibility, which means a client that can reach OAuth2 Proxy directly may be able to spoof forwarded headers. + +Once `--trusted-proxy-ip` is configured, the value used for `--real-client-ip-header` (e.g. `X-Forwarded-For`) is parsed by walking the comma-separated hop list from the newest (rightmost) entry inward, skipping over entries that are themselves trusted proxies. The first entry that isn't a trusted proxy is used as the client IP. This prevents a client from bypassing the restriction by setting the header to an arbitrary value themselves: since a trusted proxy only ever *appends* to the header rather than replacing it, a client-supplied leftmost value can no longer be trusted, only the value the nearest trusted proxy actually observed. If the direct connection isn't itself a trusted proxy, the header is ignored entirely. ::: | Flag / Config Field | Type | Description | Default | diff --git a/oauthproxy.go b/oauthproxy.go index f8dc5471..7c0f8b99 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -630,7 +630,7 @@ func (p *OAuthProxy) isTrustedIP(req *http.Request) bool { return false } - remoteAddr, err := ip.GetClientIP(p.realClientIPParser, req) + remoteAddr, err := ip.GetClientIP(p.realClientIPParser, req, requestTrustedProxies(req)) if err != nil { logger.Errorf("Error obtaining real IP for trusted IP list: %v", err) // Possibly spoofed X-Real-IP header @@ -644,6 +644,15 @@ func (p *OAuthProxy) isTrustedIP(req *http.Request) bool { return p.trustedIPs.Has(remoteAddr) } +// requestTrustedProxies returns the set of proxies trusted to supply forwarded headers for +// req, as computed for the request's RequestScope, or nil if no scope/restriction is set. +func requestTrustedProxies(req *http.Request) ipapi.TrustedProxies { + if scope := middlewareapi.GetRequestScope(req); scope != nil { + return scope.TrustedProxies + } + return nil +} + // SignInPage writes the sign in template to the response func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code int) { prepareNoCache(rw) @@ -883,7 +892,7 @@ func (p *OAuthProxy) doOAuthStart(rw http.ResponseWriter, req *http.Request, ove // OAuthCallback is the OAuth2 authentication flow callback that finishes the // OAuth2 authentication flow func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { - remoteAddr := ip.GetClientString(p.realClientIPParser, req, true) + remoteAddr := ip.GetClientString(p.realClientIPParser, req, true, requestTrustedProxies(req)) // finish the oauth cycle // #nosec G120 -- The default max size in Go is already capped at 10MB so this would be the absolute max and is diff --git a/oauthproxy_test.go b/oauthproxy_test.go index b3271e5b..9b2e45ac 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -2177,6 +2177,7 @@ func TestTrustedIPs(t *testing.T) { tests := []struct { name string trustedIPs []string + trustedProxyIPs []string reverseProxy bool realClientIPHeader string req *http.Request @@ -2337,6 +2338,25 @@ func TestTrustedIPs(t *testing.T) { }(), expectTrusted: false, }, + // Reported vulnerability: a client sends X-Forwarded-For set to an IP on the + // --trusted-ip allowlist, hoping to bypass auth. The trusted reverse proxy appends the + // client's real (untrusted) IP rather than replacing the header. With --trusted-proxy-ip + // scoped to just the proxy, oauth2-proxy must walk to the rightmost hop (the attacker's + // real IP) instead of trusting the client-supplied leftmost value. + { + name: "SpoofedLeftmostHopBehindTrustedProxyIsNotTrusted", + trustedIPs: []string{"9.9.9.9"}, + trustedProxyIPs: []string{"10.0.0.5/32"}, + reverseProxy: true, + realClientIPHeader: "X-Forwarded-For", + req: func() *http.Request { + req, _ := http.NewRequest(http.MethodGet, "/", nil) + req.Header.Add("X-Forwarded-For", "9.9.9.9, 6.6.6.6") + req.RemoteAddr = "10.0.0.5:12345" + return req + }(), + expectTrusted: false, + }, } for _, tt := range tests { @@ -2352,6 +2372,7 @@ func TestTrustedIPs(t *testing.T) { }, } opts.TrustedIPs = tt.trustedIPs + opts.TrustedProxyIPs = tt.trustedProxyIPs opts.ReverseProxy = tt.reverseProxy opts.RealClientIPHeader = tt.realClientIPHeader err := validation.Validate(opts) diff --git a/pkg/apis/ip/interfaces.go b/pkg/apis/ip/interfaces.go index 02f3937f..7409b142 100644 --- a/pkg/apis/ip/interfaces.go +++ b/pkg/apis/ip/interfaces.go @@ -5,7 +5,16 @@ import ( "net/http" ) +// TrustedProxies reports whether a given IP belongs to a proxy that is +// trusted to supply forwarded headers. +type TrustedProxies interface { + Has(net.IP) bool +} + // RealClientIPParser is an interface for a getting the client's real IP to be used for logging. type RealClientIPParser interface { - GetRealClientIP(http.Header) (net.IP, error) + // GetRealClientIP parses the configured forwarded-header out of h. remoteAddr is the + // IP of the direct connecting peer (nil if unknown) and trusted is the set of proxies + // allowed to supply forwarded headers (nil if no trusted-proxy restriction is configured). + GetRealClientIP(h http.Header, remoteAddr net.IP, trusted TrustedProxies) (net.IP, error) } diff --git a/pkg/ip/realclientip.go b/pkg/ip/realclientip.go index db8f2595..3e9470d8 100644 --- a/pkg/ip/realclientip.go +++ b/pkg/ip/realclientip.go @@ -33,23 +33,52 @@ type xForwardedForClientIPParser struct { // GetRealClientIP obtain the IP address of the end-user (not proxy). // Parses headers sharing the format as specified by: // * https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For. -// Returns the `` portion specified in the above document. // Additionally, is capable of parsing IPs with the port included, for v4 in the format ":" and for v6 in the // format "[]:". With-port and without-port formats are seamlessly supported concurrently. -func (p xForwardedForClientIPParser) GetRealClientIP(h http.Header) (net.IP, error) { - var ipStr string - if realIP := h.Get(p.header); realIP != "" { - ipStr = realIP - } else { +// +// Each successive proxy may append itself, comma separated, to the end of the header. Blindly trusting the leftmost +// (client-supplied) entry lets an untrusted client spoof it, so instead this walks the chain from the right (the +// most recently appended hop) and skips over entries that are themselves trusted proxies, returning the first entry +// that isn't -- that's the address a trusted proxy is vouching for, which the originating client cannot forge. If +// the direct connecting peer (remoteAddr) is not itself a trusted proxy, the header is ignored entirely and +// remoteAddr is returned, since nothing in a client-controlled header can be believed in that case. +func (p xForwardedForClientIPParser) GetRealClientIP(h http.Header, remoteAddr net.IP, trusted ipapi.TrustedProxies) (net.IP, error) { + raw := h.Get(p.header) + if raw == "" { return nil, nil } - // Each successive proxy may append itself, comma separated, to the end of the X-Forwarded-for header. - // Select only the first IP listed, as it is the client IP recorded by the first proxy. - if commaIndex := strings.IndexRune(ipStr, ','); commaIndex != -1 { - ipStr = ipStr[:commaIndex] + isTrusted := func(candidate net.IP) bool { + return trusted == nil || candidate == nil || trusted.Has(candidate) } - ipStr = strings.TrimSpace(ipStr) + + if !isTrusted(remoteAddr) { + return remoteAddr, nil + } + + hops := strings.Split(raw, ",") + + var lastParsed net.IP + for i := len(hops) - 1; i >= 0; i-- { + hopIP, err := parseHopIP(hops[i], p.header) + if err != nil { + return nil, err + } + if !isTrusted(hopIP) { + return hopIP, nil + } + lastParsed = hopIP + } + + // Every hop was itself a trusted proxy; fall back to the leftmost (oldest) entry, matching the historical + // behavior for when trusted proxies aren't restricted (trust-all default). + return lastParsed, nil +} + +// parseHopIP parses a single comma-separated entry of a forwarded header into an IP, +// stripping surrounding whitespace and an optional port. +func parseHopIP(hop string, header string) (net.IP, error) { + ipStr := strings.TrimSpace(hop) if ipHost, _, err := net.SplitHostPort(ipStr); err == nil { ipStr = ipHost @@ -57,16 +86,19 @@ func (p xForwardedForClientIPParser) GetRealClientIP(h http.Header) (net.IP, err ip := net.ParseIP(ipStr) if ip == nil { - return nil, fmt.Errorf("unable to parse ip (%s) from %s header", ipStr, http.CanonicalHeaderKey(p.header)) + return nil, fmt.Errorf("unable to parse ip (%s) from %s header", ipStr, http.CanonicalHeaderKey(header)) } return ip, nil } // GetClientIP obtains the perceived end-user IP address from headers if p != nil else from req.RemoteAddr. -func GetClientIP(p ipapi.RealClientIPParser, req *http.Request) (net.IP, error) { +func GetClientIP(p ipapi.RealClientIPParser, req *http.Request, trusted ipapi.TrustedProxies) (net.IP, error) { if p != nil { - return p.GetRealClientIP(req.Header) + // Best-effort: an unparseable RemoteAddr becomes nil, which GetRealClientIP treats as trusted + // (e.g. unix sockets, or tests that don't set RemoteAddr), so this never blocks the header path. + remoteAddr, _ := getRemoteIP(req) + return p.GetRealClientIP(req.Header, remoteAddr, trusted) } return getRemoteIP(req) } @@ -91,16 +123,18 @@ func getRemoteIP(req *http.Request) (net.IP, error) { } // GetClientString obtains the human readable string of the remote IP and optionally the real client IP if available -func GetClientString(p ipapi.RealClientIPParser, req *http.Request, full bool) (s string) { +func GetClientString(p ipapi.RealClientIPParser, req *http.Request, full bool, trusted ipapi.TrustedProxies) (s string) { + remoteIP, remoteErr := getRemoteIP(req) + var realClientIPStr string if p != nil { - if realClientIP, err := p.GetRealClientIP(req.Header); err == nil && realClientIP != nil { + if realClientIP, err := p.GetRealClientIP(req.Header, remoteIP, trusted); err == nil && realClientIP != nil { realClientIPStr = realClientIP.String() } } var remoteIPStr string - if remoteIP, err := getRemoteIP(req); err == nil && remoteIP != nil { + if remoteErr == nil && remoteIP != nil { remoteIPStr = remoteIP.String() } diff --git a/pkg/ip/realclientip_test.go b/pkg/ip/realclientip_test.go index 3cbca114..88fa42e7 100644 --- a/pkg/ip/realclientip_test.go +++ b/pkg/ip/realclientip_test.go @@ -55,40 +55,92 @@ func TestXForwardedForClientIPParser(t *testing.T) { p := &xForwardedForClientIPParser{header: http.CanonicalHeaderKey("X-Forwarded-For")} tests := []struct { + name string headerValue string + remoteAddr net.IP + trusted *NetSet errString string expectedIP net.IP }{ - {"", "", nil}, - {"1.2.3.4", "", net.ParseIP("1.2.3.4")}, - {"10::23", "", net.ParseIP("10::23")}, - {"::1", "", net.ParseIP("::1")}, - {"[::1]:1234", "", net.ParseIP("::1")}, - {"10.0.10.11:1234", "", net.ParseIP("10.0.10.11")}, - {"192.168.10.50, 10.0.0.1, 1.2.3.4", "", net.ParseIP("192.168.10.50")}, - {"nil", "unable to parse ip (nil) from X-Forwarded-For header", nil}, - {"10000.10000.10000.10000", "unable to parse ip (10000.10000.10000.10000) from X-Forwarded-For header", nil}, + // No trusted-proxy restriction configured (nil): behaves exactly like the historical + // leftmost-picking parser, regardless of remoteAddr. + {name: "empty header", headerValue: "", expectedIP: nil}, + {name: "single hop", headerValue: "1.2.3.4", expectedIP: net.ParseIP("1.2.3.4")}, + {name: "single hop v6", headerValue: "10::23", expectedIP: net.ParseIP("10::23")}, + {name: "loopback v6", headerValue: "::1", expectedIP: net.ParseIP("::1")}, + {name: "v6 with port", headerValue: "[::1]:1234", expectedIP: net.ParseIP("::1")}, + {name: "v4 with port", headerValue: "10.0.10.11:1234", expectedIP: net.ParseIP("10.0.10.11")}, + {name: "no trusted proxies falls back to leftmost", headerValue: "192.168.10.50, 10.0.0.1, 1.2.3.4", expectedIP: net.ParseIP("192.168.10.50")}, + {name: "unparseable hop", headerValue: "nil", errString: "unable to parse ip (nil) from X-Forwarded-For header"}, + {name: "malformed hop", headerValue: "10000.10000.10000.10000", errString: "unable to parse ip (10000.10000.10000.10000) from X-Forwarded-For header"}, + + // The reported attack: a client sends X-Forwarded-For set to an IP it wants to + // impersonate; the trusted reverse proxy appends the real client IP rather than + // replacing the header. Only the proxy's own /32 is trusted, so the walk must stop at + // the rightmost (attacker) hop instead of trusting the client-supplied leftmost value. + { + name: "spoofed leftmost hop behind a trusted proxy", + headerValue: "9.9.9.9, 6.6.6.6", + remoteAddr: net.ParseIP("10.0.0.5"), + trusted: mustNetSet(t, "10.0.0.5/32"), + expectedIP: net.ParseIP("6.6.6.6"), + }, + // Direct peer isn't a trusted proxy at all: the header must be ignored entirely and + // the real connecting peer used, since nothing in the header can be believed. + { + name: "untrusted direct peer ignores header", + headerValue: "9.9.9.9", + remoteAddr: net.ParseIP("6.6.6.6"), + trusted: mustNetSet(t, "10.0.0.5/32"), + expectedIP: net.ParseIP("6.6.6.6"), + }, + // A chain of two trusted proxies (client -> ProxyA(10.0.0.6) -> ProxyB(10.0.0.5) -> + // us) still resolves to the real (untrusted) client hop, skipping past both trusted + // proxy-appended hops and the attacker's forged leftmost claim. + { + name: "chain of trusted proxies", + headerValue: "9.9.9.9, 6.6.6.6, 10.0.0.6", + remoteAddr: net.ParseIP("10.0.0.5"), + trusted: mustNetSet(t, "10.0.0.5/32", "10.0.0.6/32"), + expectedIP: net.ParseIP("6.6.6.6"), + }, + // Degenerate case: every hop (and the direct peer) is itself a trusted proxy; there's + // no untrusted hop to find, so fall back to the oldest (leftmost) entry. + { + name: "all hops trusted falls back to leftmost", + headerValue: "10.0.0.7, 10.0.0.6", + remoteAddr: net.ParseIP("10.0.0.5"), + trusted: mustNetSet(t, "10.0.0.5/32", "10.0.0.6/32", "10.0.0.7/32"), + expectedIP: net.ParseIP("10.0.0.7"), + }, } for _, test := range tests { - h := http.Header{} - h.Add("X-Forwarded-For", test.headerValue) + t.Run(test.name, func(t *testing.T) { + h := http.Header{} + h.Add("X-Forwarded-For", test.headerValue) - ip, err := p.GetRealClientIP(h) + var trusted ipapi.TrustedProxies + if test.trusted != nil { + trusted = test.trusted + } - if test.errString == "" { - assert.Nil(t, err) - } else { - assert.NotNil(t, err) - assert.Equal(t, test.errString, err.Error()) - } + ip, err := p.GetRealClientIP(h, test.remoteAddr, trusted) - if test.expectedIP == nil { - assert.Nil(t, ip) - } else { - assert.NotNil(t, ip) - assert.Equal(t, test.expectedIP, ip) - } + if test.errString == "" { + assert.Nil(t, err) + } else { + assert.NotNil(t, err) + assert.Equal(t, test.errString, err.Error()) + } + + if test.expectedIP == nil { + assert.Nil(t, ip) + } else { + assert.NotNil(t, ip) + assert.Equal(t, test.expectedIP, ip) + } + }) } } @@ -100,12 +152,22 @@ func TestXForwardedForClientIPParserIgnoresOthers(t *testing.T) { h.Add("X-Real-IP", "10.0.0.1") h.Add("X-ProxyUser-IP", "10.0.0.1") h.Add("X-Forwarded-For", expectedIPString) - ip, err := p.GetRealClientIP(h) + ip, err := p.GetRealClientIP(h, nil, nil) assert.Nil(t, err) assert.NotNil(t, ip) assert.Equal(t, ip, net.ParseIP(expectedIPString)) } +// mustNetSet builds a *NetSet from CIDR/IP strings, failing the test on error. +func mustNetSet(t *testing.T, ipStrs ...string) *NetSet { + t.Helper() + set, err := ParseNetSet(ipStrs) + if err != nil { + t.Fatalf("failed to build NetSet: %v", err) + } + return set +} + func TestGetRemoteIP(t *testing.T) { tests := []struct { remoteAddr string @@ -174,10 +236,10 @@ func TestGetClientString(t *testing.T) { RemoteAddr: test.remoteAddr, } - client := GetClientString(test.parser, req, false) + client := GetClientString(test.parser, req, false, nil) assert.Equal(t, test.expectedClient, client) - clientFull := GetClientString(test.parser, req, true) + clientFull := GetClientString(test.parser, req, true, nil) assert.Equal(t, test.expectedClientFull, clientFull) } } diff --git a/pkg/validation/options.go b/pkg/validation/options.go index 13ce2e0b..aa2da1cf 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -8,6 +8,8 @@ import ( "net/url" "strings" + ipapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/ip" + middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/authentication/hmacauth" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" @@ -89,7 +91,11 @@ func Validate(o *options.Options) error { // Allow the logger to get client IPs logger.SetGetClientFunc(func(r *http.Request) string { - return ip.GetClientString(o.GetRealClientIPParser(), r, false) + var trusted ipapi.TrustedProxies + if scope := middlewareapi.GetRequestScope(r); scope != nil { + trusted = scope.TrustedProxies + } + return ip.GetClientString(o.GetRealClientIPParser(), r, false, trusted) }) }