Bug in SetRemoteAddrFromForwardedFor middleware

[Simon Willison]

The middleware contains the following:

# HTTP_X_FORWARDED_FOR can be a comma-separated list of IPs.
# Take just the first one.
real_ip = real_ip.split(",")[0]

I'm pretty sure it should be taking the LAST element in the list, not the first - at least going by Bob Ippolito's description here:

​http://bob.pythonmac.org/archives/2005/09/23/apache-x-forwarded-for-caveat/

This could be a security issue, as it may allow people to forge an X-Forwarded-For header and provide a fake IP address to Django.

[Gary Wilson <gary.wilson@…>]

Marking accepted unless someone can prove otherwise.

[Grzegorz Ślusarek]

patch against revision 6213

[Adrian Holovaty]

(In [6364]) Fixed #3872 -- Fixed incorrect handling of HTTP_X_FORWARDED_FOR in SetRemoteAddrFromForwardedFor. Thanks, Simon Willison and gregorth

[Chris Bennett <chrisrbennett@…>]

Wait a minute... client IS the first IP listed.
​http://en.wikipedia.org/wiki/X-Forwarded-For

And I'm looking at request on a machine being proxied by Apache right now:
HTTP_X_FORWARDED_FOR: 66.162.32.x, 127.0.0.1

Also, some reverse proxies may pass X-Forwarded-For and its capitalization variants in place of HTTP_X_FORWARDED_FOR.

Though, in either case, the origin client is clearly first. Revert?

[Jacob]

(In [6397]) Fixed #3872, which turns out to not have been a bug in the first place, by reverting [6364].

[leosoto <leo.soto@…>]

Jacob: please consider my comments on ​http://groups.google.com/group/django-developers/browse_thread/thread/b3b2deb687a3e885

[John Moylan]

Hi, I'm using Apache and Django with Squid as a reverse proxy. Squid appends it's IP to the "END" of the X_FORWARDED_FOR header. The real ip shoudl be the second last in the list.

EG

'HTTP_X_FORWARDED_FOR': '10.162.50.55, 213.233.159.69, 89.207.56.145',

89.207.56.145 is Squid

In this sort of setup the SetRemoteAddrFromForwardedFor should use that second last IP as the real IP

real_ip = real_ip.split(",")[-2].strip()

and not the current

real_ip = real_ip.split(",")[0].strip()

Having real_ip as the first IP in the list assumes that the client has not gone through multiple proxies.

[John Moylan]

Patch to use second last IP address in list as real_ip.

[Malcolm Tredinnick]

Your patch doesn't generalise. What if there's another such proxy in the middle. Then you have to look at the third-last address. And so on. There's no standards here and no way to work it out. So we do the best we can and no better. Going to reclose this, since, as indicated by the various changes, there are conflicting setups.

Ultimately, if what is there doesn't work for you, don't use it. If there's some circumstance where Django won't operate with this feature (i.e. it's not ignorable, as opposed to no useful), then that would be a bug and a new ticket would be appropriate.

[John Moylan]

Maybe the issue is a documentation problem. I had mistakenly assumed that this middleware was for use when you have a django app sitting behind a reverse proxy. In such a settup you have to use the X-Forwarded-For last IP that is before your own reverse proxy ip as the remote-ip.

If you want to try and get the original Client IP - real_ip.split(",")[0].strip() then the current code is probably a good attempt but in real world situations may not be as useful as just getting the last proxy IP address because it will be more prone to issues with proxy anonomizers or caches set up to strip X-Forwarded-For .