Opened 17 years ago

Closed 15 years ago

Last modified 15 years ago

#3872 closed (fixed)

Bug in SetRemoteAddrFromForwardedFor middleware

Reported by: Simon Willison Owned by: Grzegorz Ślusarek
Component: Core (Other) Version: dev
Severity: Keywords: middleware
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (2)

SetRemoteAddrFromForwardedFor.diff (665 bytes ) - added by Grzegorz Ślusarek 17 years ago.
patch against revision 6213
patch.git.diff (525 bytes ) - added by John Moylan 15 years ago.
Patch to use second last IP address in list as real_ip.

Download all attachments as: .zip

Change History (13)

comment:1 by Gary Wilson <gary.wilson@…>, 17 years ago

Triage Stage: UnreviewedAccepted

Marking accepted unless someone can prove otherwise.

comment:2 by Grzegorz Ślusarek, 17 years ago

Owner: changed from nobody to Grzegorz Ślusarek
Status: newassigned

by Grzegorz Ślusarek, 17 years ago

patch against revision 6213

comment:3 by Grzegorz Ślusarek, 17 years ago

Has patch: set

comment:4 by Grzegorz Ślusarek, 17 years ago

Triage Stage: AcceptedReady for checkin

comment:5 by Adrian Holovaty, 17 years ago

Resolution: fixed
Status: assignedclosed

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

comment:6 by Chris Bennett <chrisrbennett@…>, 17 years ago

Resolution: fixed
Status: closedreopened

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?

comment:7 by Jacob, 17 years ago

Resolution: fixed
Status: reopenedclosed

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

comment:9 by John Moylan, 15 years ago

Resolution: fixed
Status: closedreopened

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.

by John Moylan, 15 years ago

Attachment: patch.git.diff added

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

comment:10 by Malcolm Tredinnick, 15 years ago

Resolution: fixed
Status: reopenedclosed

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.

comment:11 by John Moylan, 15 years ago

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 .


Note: See TracTickets for help on using tickets.
Back to Top