X-Forwarded-For handling is unsafe

Breaks API, but immensely improves security

Fixes gin-gonic#2473

README.md

@@ -2115,6 +2115,43 @@ func main() {
 }
 ```
 
+## Don't trust all proxies
+
+Gin lets you specify which headers to hold the real client IP (if any),
+as well as specifying which proxies (or direct clients) you trust to
+specify one of these headers.
+
+The `TrustedProxies` slice on your `gin.Engine` specifes the clients
+truest to specify unspoofed client IP headers. Proxies can be specified
+as IP's, CIDR's, or hostnames. Hostnames are resolved on each query,
+such that changes in your proxy pool take effect immediately. The
+hostname option is handy, but also costly, so only use if you have no
+other option.
+
+```go
+import (
+    "fmt"
+
+    "github.com/gin-gonic/gin"
+)
+
+func main() {
+
+	router := gin.Default()
+	router.TrustedProxies = []string{"192.168.1.2"}
+
+    router.GET("/", func(c *gin.Context) {
+
+		// If the client is 192.168.1.2, use the X-Forwarded-For
+		// header to deduce the original client IP from the trust-
+		// worthy parts of that header.
+		// Otherwise, simply return the direct client IP
+        fmt.Printf("ClientIP: %s\n", c.ClientIP())
+    })
+
+    router.Run()
+}
+```
 
 ## Testing
 

context.go

@@ -713,14 +713,12 @@ 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.ForwardedByClientIP && c.engine.RemoteIPHeaders != nil {
+		for _, header := range c.engine.RemoteIPHeaders {
+			ipChain := filterIPsFromUntrustedProxies(c.requestHeader(header), c.Request, c.engine)
+			if len(ipChain) > 0 {
+				return ipChain[0]
+			}
 		}
 	}
 
@@ -730,11 +728,82 @@ 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, ",")
+	} else {
+		return []string{}
+	}
+	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
+		}
+		//		out = prependString(ip.String(), 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,11 +1380,23 @@ func TestContextClientIP(t *testing.T) {
 	c, _ := CreateTestContext(httptest.NewRecorder())
 	c.Request, _ = http.NewRequest("POST", "/", 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 "
+	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")
+		}
+		if host == "baz" {
+			return []string{"thisshouldneverhappen"}, nil
+		}
+		return []string{}, nil
+	}
+
+	resetContextForClientIPTests(c)
 
+	// Legacy tests (validating that the defaults don't break the
+	// (insecure!) old behaviour)
 	assert.Equal(t, "20.20.20.20", c.ClientIP())
 
 	c.Request.Header.Del("X-Forwarded-For")
@@ -1404,6 +1416,74 @@ func TestContextClientIP(t *testing.T) {
 	// no port
 	c.Request.RemoteAddr = "50.50.50.50"
 	assert.Empty(t, c.ClientIP())
+
+	// Tests exercising the TrustedProxies functionality
+	resetContextForClientIPTests(c)
+
+	// No trusted proxies
+	c.engine.TrustedProxies = []string{}
+	c.engine.RemoteIPHeaders = []string{"X-Forwarded-For"}
+	assert.Equal(t, "40.40.40.40", c.ClientIP())
+
+	// Last proxy is trusted, but the RemoteAddr is not
+	c.engine.TrustedProxies = []string{"30.30.30.30"}
+	assert.Equal(t, "40.40.40.40", c.ClientIP())
+
+	// Only trust RemoteAddr
+	c.engine.TrustedProxies = []string{"40.40.40.40"}
+	assert.Equal(t, "30.30.30.30", c.ClientIP())
+
+	// All steps are trusted
+	c.engine.TrustedProxies = []string{"40.40.40.40", "30.30.30.30", "20.20.20.20"}
+	assert.Equal(t, "20.20.20.20", c.ClientIP())
+
+	// Use CIDR
+	c.engine.TrustedProxies = []string{"40.40.25.25/16", "30.30.30.30"}
+	assert.Equal(t, "20.20.20.20", c.ClientIP())
+
+	// Use hostname that resolves to all the proxies
+	c.engine.TrustedProxies = []string{"foo"}
+	assert.Equal(t, "20.20.20.20", c.ClientIP())
+
+	// Use hostname that returns an error
+	c.engine.TrustedProxies = []string{"bar"}
+	assert.Equal(t, "40.40.40.40", c.ClientIP())
+
+	// X-Forwarded-For has a non-IP element
+	c.engine.TrustedProxies = []string{"40.40.40.40"}
+	c.Request.Header.Set("X-Forwarded-For", " blah ")
+	assert.Equal(t, "40.40.40.40", c.ClientIP())
+
+	// Result from LookupHost has non-IP element. This should never
+	// happen, but we should test it to make sure we handle it
+	// gracefully.
+	c.engine.TrustedProxies = []string{"baz"}
+	c.Request.Header.Set("X-Forwarded-For", " 30.30.30.30 ")
+	assert.Equal(t, "40.40.40.40", c.ClientIP())
+
+	c.engine.TrustedProxies = []string{"40.40.40.40"}
+	c.Request.Header.Del("X-Forwarded-For")
+	c.engine.RemoteIPHeaders = []string{"X-Forwarded-For", "X-Real-IP"}
+	assert.Equal(t, "10.10.10.10", c.ClientIP())
+
+	c.engine.RemoteIPHeaders = []string{}
+	c.engine.AppEngine = true
+	assert.Equal(t, "50.50.50.50", c.ClientIP())
+
+	c.Request.Header.Del("X-Appengine-Remote-Addr")
+	assert.Equal(t, "40.40.40.40", c.ClientIP())
+
+	// no port
+	c.Request.RemoteAddr = "50.50.50.50"
+	assert.Empty(t, c.ClientIP())
+}
+
+func resetContextForClientIPTests(c *Context) {
+	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 "
+	c.engine.AppEngine = false
 }
 
 func TestContextContentType(t *testing.T) {

gin.go

@@ -82,6 +82,8 @@ type Engine struct {
 	// handler.
 	HandleMethodNotAllowed bool
 	ForwardedByClientIP    bool
+	RemoteIPHeaders        []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
@@ -139,11 +143,14 @@ func New() *Engine {
 		RedirectFixedPath:      false,
 		HandleMethodNotAllowed: false,
 		ForwardedByClientIP:    true,
+		RemoteIPHeaders:        []string{"X-Forwarded-For", "X-Real-IP"},
+		TrustedProxies:         []string{"0.0.0.0/0"},
 		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);",
@@ -362,6 +369,11 @@ func (engine *Engine) RunFd(fd int) (err error) {
 func (engine *Engine) RunListener(listener net.Listener) (err error) {
 	debugPrint("Listening and serving HTTP on listener what's bind with address@%s", listener.Addr())
 	defer func() { debugPrintError(err) }()
+	//	if engine.TrustedProxies != nil && len(engine.TrustedProxies) == 1 &&
+	//	engine.TrustedProxies[0] == "0.0.0.0/0" {
+	//fmt.Fprintf(DefaultWriter, "[GIN-WARNING] TrustProxies is set to \"0.0.0.0\" +"
+	//                          "allowing anyone to spoof their source address.")
+	//	}
 	err = http.Serve(listener, engine)
 	return
 }