search

go-chi / chi

lightweight, idiomatic and composable router for building Go HTTP services
https://go-chi.io
MIT License
18.4k stars 983 forks source link

Problems with use of X-Forwarded-For and other headers to get the "real" client IP #711

Open adam-p opened 2 years ago

adam-p commented 2 years ago

go-chi/chi/middleware/RealIP and go-chi/httprate suffer from problems getting the "real" client IP. I'll try to break it down into categories.

RealIP isn't necessarily "security-related", but it depends on how the dev uses it. I would argue that rate limiting -- and so httprate -- is security-related.

For a full explanation of the problems mentioned here, please see my blog post about it. (Here's the chi-specific section, but all the rest is informative.)

Headers are untrustworthy, unless added by a reverse proxy you control

The RealIP middleware has this comment:

You should only use this middleware if you can trust the headers passed to you (in particular, the two [three, actually] headers this middleware uses), for example because you have placed a reverse proxy like HAProxy or nginx in front of Chi. If your reverse proxies are configured to pass along arbitrary header values from the client, or if you use this middleware without a reverse proxy, malicious clients will be able to make you very sad (or, depending on how you're using RemoteAddr, vulnerable to an attack of some sort).

This is a good warning, but a) there's no such warning on chi's httprate.KeyByIP (even though it does the same thing), and b) it's not a comprehensive enough warning.

Some reverse proxies, like AWS ALB, lets all header values through that it doesn't set itself. So it will let through True-Client-IP and X-Real-IP. An attacker can spoof either of those headers, and that will end up as the IP that is reported by RealIP or the IP that's gets rate-limited by httprate.

X-Forwarded-For is tricky

Leftmost vs Rightmost

Also, X-Forwarded-For is only appended to by reverse proxies, so the only IPs in it that are trustworthy are the ones that were added by reverse proxies that you control. httprate, however, takes the first IP in XFF. So an attacker can trivially spoof it. The leftmost XFF IP should never be used for security-related purposes.

Switching to rightmost will introduce an XFF multiple-headers problem

Go's http.Header.Get returns the first instance of a header, but to get the rightmost IP from XFF, you need to get the last. (More info here.)

Leftmost values can be garbage

If you really want to use the leftmost IP, you need to be aware that it could be either a private-space IP address or complete garbage.

A suggested algorithm for how to get it is here.

Parsing the XFF IP list

httprate splits the XFF list by comma-space rather than just comma. RFC 2616 only says "comma-separated" for list headers. (And RealIP splits by comma. And I've seen reverse proxies append without a space. And I've looked at about ten other projects and haven't seen another example of splitting by comma-space.)

Results

RealIP can easily be made to return a spoofed header.

httprate can be bypassed by spoofing different IPs. Possibly the server memory can be exhausted by spoofing very large fake IP values.

mfridman commented 2 years ago

There was a similar proposal opened in #708 to implement a more advanced middleware, maybe worth taking a look at that proposal and jotting down your thought?

adam-p commented 2 years ago

I put this in #708 but I'm going to repeat it here because this issue is also about httprate...

I wrote a library to help with extracting the "real" client IP from a request. I intended that it be useful in cases like this, so please let me know if it's suitable (and if it's not we'll figure out how to make it better). https://github.com/realclientip/realclientip-go

lrstanley commented 2 years ago

I wasn't aware of this issue directly, but I did reach out to @pkieltyka a few days ago around this topic specifically in relation to httprate, about always reading real-ip related headers in some of the default methods as not secure due to how easy it is to bypass rate limiting. He implemented a fix in https://github.com/go-chi/httprate/pull/17 -- simply by moving the logic into separate methods (KeyByRealIP and LimitByRealIP). That change means that the behavior, at least with httprate, is now secure by default (reading only r.RemoteAddr), unless using the real-ip methods. As long as the real-ip implemention used updates r.RemoteAddr, httprate will work as expected.

RealIP still checking headers regardless of source (and a handful of other important items) is an issue, and I think it does still warrant either a new RealIP implementation, or a variant, that is more secure. I have my own implementation as well which explicitly allows enabling a subset of the headers, allows matching CIDR ranges that are trusted, as well as built-in bogon IP trust support for when you can trust your local network. Definitely not perfect, so hopefully a solution that includes all of the gotchas mentioned by @adam-p can be achieved, and hopefully be built-in and recommended by go-chi as well.