From 33aac7f2f89d964e2e1634af2fd07a5f05d83126 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20L=2E=20Hansen?= Date: Thu, 20 Aug 2020 23:51:33 +0200 Subject: [PATCH] X-Forwarded-For handling is unsafe Breaks API, but immensely improves security Fixes #2473 --- context.go | 86 ++++++++++++++++++++++++++++++++++++++++++------- context_test.go | 38 ++++++++++++++++++---- gin.go | 10 ++++-- logger_test.go | 2 ++ 4 files changed, 117 insertions(+), 19 deletions(-) diff --git a/context.go b/context.go index 95b1807d72..99a99b9d64 100644 --- a/context.go +++ b/context.go @@ -713,14 +713,10 @@ func (c *Context) ShouldBindBodyWith(obj interface{}, bb binding.BindingBody) (e // X-Real-IP and X-Forwarded-For in order to work properly with reverse-proxies such us: nginx or haproxy. // Use X-Forwarded-For before X-Real-Ip as nginx uses X-Real-Ip with the proxy's IP. func (c *Context) ClientIP() string { - if c.engine.ForwardedByClientIP { - clientIP := c.requestHeader("X-Forwarded-For") - clientIP = strings.TrimSpace(strings.Split(clientIP, ",")[0]) - if clientIP == "" { - clientIP = strings.TrimSpace(c.requestHeader("X-Real-Ip")) - } - if clientIP != "" { - return clientIP + if c.engine.RemoteIPHeader != "" { + ipChain := filterIPsFromUntrustedProxies(c.requestHeader(c.engine.RemoteIPHeader), c.Request, c.engine) + if len(ipChain) > 0 { + return ipChain[0] } } @@ -730,11 +726,79 @@ func (c *Context) ClientIP() string { } } - if ip, _, err := net.SplitHostPort(strings.TrimSpace(c.Request.RemoteAddr)); err == nil { - return ip + ip, _ := getTransportPeerIPForRequest(c.Request) + + return ip +} + +func filterIPsFromUntrustedProxies(XForwardedForHeader string, req *http.Request, e *Engine) []string { + var items, out []string + if XForwardedForHeader != "" { + items = strings.Split(XForwardedForHeader, ",") + } + if peerIP, err := getTransportPeerIPForRequest(req); err == nil { + items = append(items, peerIP) } - return "" + for i := len(items) - 1; i >= 0; i-- { + item := strings.TrimSpace(items[i]) + ip := net.ParseIP(item) + if ip == nil { + return out + } + + out = prependString(ip.String(), out) + if !isTrustedProxy(ip, e) { + return out + } + } + return out +} + +func isTrustedProxy(ip net.IP, e *Engine) bool { + for _, trustedProxy := range e.TrustedProxies { + if _, ipnet, err := net.ParseCIDR(trustedProxy); err == nil { + if ipnet.Contains(ip) { + return true + } + continue + } + + if proxyIP := net.ParseIP(trustedProxy); proxyIP != nil { + if proxyIP.Equal(ip) { + return true + } + continue + } + + if addrs, err := e.lookupHost(trustedProxy); err == nil { + for _, proxyAddr := range addrs { + proxyIP := net.ParseIP(proxyAddr) + if proxyIP == nil { + continue + } + if proxyIP.Equal(ip) { + return true + } + } + } + } + return false +} + +func prependString(ip string, ipList []string) []string { + ipList = append(ipList, "") + copy(ipList[1:], ipList) + ipList[0] = string(ip) + return ipList +} + +func getTransportPeerIPForRequest(req *http.Request) (string, error) { + var err error + if ip, _, err := net.SplitHostPort(strings.TrimSpace(req.RemoteAddr)); err == nil { + return ip, nil + } + return "", err } // ContentType returns the Content-Type header of the request. diff --git a/context_test.go b/context_test.go index 1a5a3c5ff5..1ea040d36d 100644 --- a/context_test.go +++ b/context_test.go @@ -1380,21 +1380,47 @@ func TestContextClientIP(t *testing.T) { c, _ := CreateTestContext(httptest.NewRecorder()) c.Request, _ = http.NewRequest("POST", "/", nil) + c.engine.lookupHost = func(host string) ([]string, error) { + if host == "foo" { + return []string{"40.40.40.40", "30.30.30.30"}, nil + } + if host == "bar" { + return nil, errors.New("hostname lookup failed") + } + return []string{}, nil + } + c.Request.Header.Set("X-Real-IP", " 10.10.10.10 ") c.Request.Header.Set("X-Forwarded-For", " 20.20.20.20, 30.30.30.30") c.Request.Header.Set("X-Appengine-Remote-Addr", "50.50.50.50") c.Request.RemoteAddr = " 40.40.40.40:42123 " - assert.Equal(t, "20.20.20.20", c.ClientIP()) + c.engine.RemoteIPHeader = "X-Forwarded-For" + assert.Equal(t, "40.40.40.40", c.ClientIP()) - c.Request.Header.Del("X-Forwarded-For") - assert.Equal(t, "10.10.10.10", c.ClientIP()) + c.engine.TrustedProxies = []string{"30.30.30.30"} + assert.Equal(t, "40.40.40.40", c.ClientIP()) - c.Request.Header.Set("X-Forwarded-For", "30.30.30.30 ") + c.engine.TrustedProxies = []string{"40.40.40.40"} assert.Equal(t, "30.30.30.30", c.ClientIP()) - c.Request.Header.Del("X-Forwarded-For") - c.Request.Header.Del("X-Real-IP") + c.engine.TrustedProxies = []string{"40.40.25.25/16", "30.30.30.30"} + assert.Equal(t, "20.20.20.20", c.ClientIP()) + + c.engine.TrustedProxies = []string{"foo"} + assert.Equal(t, "20.20.20.20", c.ClientIP()) + + c.engine.TrustedProxies = []string{"bar"} + assert.Equal(t, "40.40.40.40", c.ClientIP()) + + c.Request.Header.Set("X-Forwarded-For", " blah ") + assert.Equal(t, "40.40.40.40", c.ClientIP()) + + c.engine.TrustedProxies = []string{"40.40.40.40"} + c.engine.RemoteIPHeader = "X-Real-IP" + assert.Equal(t, "10.10.10.10", c.ClientIP()) + + c.engine.RemoteIPHeader = "" c.engine.AppEngine = true assert.Equal(t, "50.50.50.50", c.ClientIP()) diff --git a/gin.go b/gin.go index 1e12617904..acaa9e6c5d 100644 --- a/gin.go +++ b/gin.go @@ -81,7 +81,9 @@ type Engine struct { // If no other Method is allowed, the request is delegated to the NotFound // handler. HandleMethodNotAllowed bool - ForwardedByClientIP bool + RemoteIPHeader string + + TrustedProxies []string // #726 #755 If enabled, it will thrust some headers starting with // 'X-AppEngine...' for better integration with that PaaS. @@ -103,6 +105,8 @@ type Engine struct { // See the PR #1817 and issue #1644 RemoveExtraSlash bool + lookupHost func(string) ([]string, error) + delims render.Delims secureJSONPrefix string HTMLRender render.HTMLRender @@ -138,12 +142,14 @@ func New() *Engine { RedirectTrailingSlash: true, RedirectFixedPath: false, HandleMethodNotAllowed: false, - ForwardedByClientIP: true, + RemoteIPHeader: "", + TrustedProxies: []string{}, AppEngine: defaultAppEngine, UseRawPath: false, RemoveExtraSlash: false, UnescapePathValues: true, MaxMultipartMemory: defaultMultipartMemory, + lookupHost: net.LookupHost, trees: make(methodTrees, 0, 9), delims: render.Delims{Left: "{{", Right: "}}"}, secureJSONPrefix: "while(1);", diff --git a/logger_test.go b/logger_test.go index 0d40666ec3..925e1e5584 100644 --- a/logger_test.go +++ b/logger_test.go @@ -185,6 +185,8 @@ func TestLoggerWithConfigFormatting(t *testing.T) { buffer := new(bytes.Buffer) router := New() + router.TrustedProxies = []string{"192.0.2.1"} + router.RemoteIPHeader = "X-Forwarded-For" router.Use(LoggerWithConfig(LoggerConfig{ Output: buffer, Formatter: func(param LogFormatterParams) string {