Skip to content

Commit

Permalink
X-Forwarded-For handling is unsafe
Browse files Browse the repository at this point in the history
Breaks API, but immensely improves security

Fixes gin-gonic#2473
  • Loading branch information
sorenisanerd committed Aug 22, 2020
1 parent b94d23d commit de0f9eb
Show file tree
Hide file tree
Showing 4 changed files with 208 additions and 15 deletions.
37 changes: 37 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
91 changes: 80 additions & 11 deletions context.go
Original file line number Diff line number Diff line change
Expand Up @@ -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]
}
}
}

Expand All @@ -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.
Expand Down
88 changes: 84 additions & 4 deletions context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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) {
Expand Down
7 changes: 7 additions & 0 deletions gin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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);",
Expand Down

0 comments on commit de0f9eb

Please sign in to comment.