Walk backwards through the ip list to avoid mocking by the client

[nickveenhof]

https://husobee.github.io/golang/ip-address/2015/12/17/remote-ip-go.html mentions:

Instead of walking from the left to right, walk backwards through the number of ip addresses by the number of proxies you have in your environment to the internet. That way, you will be adverse to any mucking with the X-Forwarded-For header by the client.

So the code that he/she suggests is the following:

func getIPAdress(r *http.Request) string {
    for _, h := range []string{"X-Forwarded-For", "X-Real-Ip"} {
        addresses := strings.Split(r.Header.Get(h), ",")
        // march from right to left until we get a public address
        // that will be the address right before our proxy.
        for i := len(addresses) -1 ; i >= 0; i-- {
            ip := addresses[i]
            // header can contain spaces too, strip those out.
            realIP := net.ParseIP(strings.Replace(ip, " ", "", -1))
            if !realIP.IsGlobalUnicast() && !isPrivateSubnet(realIP) {
                // bad address, go to next
                continue
            }
            return ip
        }
    }
    return ""
}

Thoughts?

[sebest]

Hi,

I already thought about this.
It really depends on the use case, so i think that this behaviour could be configurable.

You can have a setup where you chain proxy, so the real public IP of the client is the one from the first proxy, not the last one.
Also you you should only accept this header from trusted IP, that's why there is an option to list ip/network that you trust.
When i configure my loadbalancer i often overwrite (if any) the X-Forwarded-For header at the LB level and on my application/backend server i only trust the IPs of my loadbalancer.
But in some setup you can have multiple level of proxy.
So my recommendation would be to be able to define the parsing order as a configuration parameter (like the trusted) IPs with a note on the impact of this parameter.

what do you think?

[nickveenhof]

Making that configurable would be great! I haven't run into the issue itself yet so the only source I have is that blogpost. We just started to use your middleware and it works perfectly so far. What do you mean with "Only accept header from trusted IP's"? In our case we only use it for logging purposes and obviously validate the values before storing any of it. Having said that, I suppose it matters from use-case to use-case what you want to do with it.

[sebest]

Only accept header from trusted IP's: this header is set by a proxy/lb that is in front of you app, you should only accept this header if it comes from a proxy/lb that you manage (identified by its IP).

For your usecase it does not really matter if you use it for logging purpose, but let's imagine that you were using the value for authentication or to do some geoip lookup and then allow access based on country: you'll have to make sure that the end user cannot provide a fake IP through this header, so you proxy/lb should ignore the XFF header, get the IP of the client and set the XFF header with this value, in you application code, you can add the IP of the proxy/lb in the list of the trusted IP.
If you application code is only on a private network and you trust this network, it does not really matter to define a list of trusted IP.

If you want to read more about the topic, this blog post describe an attack that was used againt stackoverflow using XFF: http://blog.ircmaxell.com/2012/11/anatomy-of-attack-how-i-hacked.html

[powerman]

It looks like best way to get client IP is by making two changes to current implementation:

1. Process XFF from right to left, as was mentioned by @nickveenhof.
2. In addition to skipping right-most local/trusted IPs it make sense to also skip IPs of well-known large ISP proxies (which also can be trusted to some extend) - see https://meta.wikimedia.org/wiki/XFF_project and https://meta.wikimedia.org/w/extensions/TrustedXFF/trusted-hosts.txt

[zerkms]

But in some setup you can have multiple level of proxy.

why not iterate through them then? If there are multiple proxies - then just check whether the next rightmost address is in the trusted network - then go next.

[LahiruSenevirathne]

Why get both XFF and X real both isnt that enought to iterate through XFF and I dont understand the logic behind that

[hermanbanken]

Not having this as the default is a huge security risk!

Please regard how NGINX is parsing the XFF in realip. That is the only safe way IMO: you can only prune the rightmost value if the current RemoteAddr is in your allowed list. Then recurse, until you have an untrusted address.

By fully trusting the RemoteAddr you're placing your bets on any intermediate to not screw up.

Example:

hacker => hacker controlled rogue intermediary proxy => Google Load Balancer => your application
127.0.0.1 => 80.0.0.1 => 34.0.0.1 => 10.0.0.1

the result will be:

XFF: 127.0.0.1 => 80.0.0.1
RemoteAddr: 34.0.0.1

ergo, this library trusts the RemoteAddr is 127.0.0.1.

A better implementation is to take 80.0.0.1 as the resulting IP, as that is the last known safe one.

I hate having to learn this by browsing the code, instead of from the readme.

[adam-p]

The current untrusted-IP behaviour can lead to rate limiter evasion, memory exhaustion, access control bypass, etc. It should be clearly documented at the very least and probably changed to be a configurable rightmost-ish strategy (or strategies).
For details, see: https://adam-p.ca/blog/2022/03/x-forwarded-for/

I'm working on a library to help with this (but it's not quite done): https://github.com/realclientip/realclientip-go