From 12da875b5ac3264711b4cf5ebca94481588d92fd 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 --- README.md | 37 ++++++++++++++++++++ context.go | 91 +++++++++++++++++++++++++++++++++++++++++++------ context_test.go | 88 ++++++++++++++++++++++++++++++++++++++++++++--- gin.go | 12 +++++++ 4 files changed, 213 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index fc1d01cf3f..74725ee0fc 100644 --- a/README.md +++ b/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 diff --git a/context.go b/context.go index 95b1807d72..2b33c207cc 100644 --- a/context.go +++ b/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. diff --git a/context_test.go b/context_test.go index 1a5a3c5ff5..7ffc3aafb0 100644 --- a/context_test.go +++ b/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) { diff --git a/gin.go b/gin.go index 1e12617904..4841009537 100644 --- a/gin.go +++ b/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 }