Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#3872 closed (fixed)

Bug in SetRemoteAddrFromForwardedFor middleware

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

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 gregorth 8 years ago.
patch against revision 6213
patch.git.diff (525 bytes) - added by JohnMoylan 6 years ago.
Patch to use second last IP address in list as real_ip.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 8 years ago by Gary Wilson <gary.wilson@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Marking accepted unless someone can prove otherwise.

comment:2 Changed 8 years ago by gregorth

  • Owner changed from nobody to gregorth
  • Status changed from new to assigned

Changed 8 years ago by gregorth

patch against revision 6213

comment:3 Changed 8 years ago by gregorth

  • Has patch set

comment:4 Changed 8 years ago by gregorth

  • Triage Stage changed from Accepted to Ready for checkin

comment:5 Changed 8 years ago by adrian

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:6 Changed 8 years ago by Chris Bennett <chrisrbennett@…>

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 Changed 8 years ago by jacob

  • Resolution set to fixed
  • Status changed from reopened to closed

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

comment:8 Changed 8 years ago by leosoto <leo.soto@…>

comment:9 Changed 6 years ago by JohnMoylan

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

Changed 6 years ago by JohnMoylan

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

comment:10 Changed 6 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from reopened to closed

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 Changed 6 years ago by JohnMoylan

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