X-Forwarded-For handling is unsafe

Breaks API, but immensely improves security

Fixes gin-gonic#2473

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.

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())
 

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);",

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 {