[drupal-devel] [feature] Replace $_SERVER["REMOTE_ADDR"] with a function which understands X-Forwarded-For

[Steven]

Issue status update for http://drupal.org/node/20471

 Project:      Drupal
 Version:      4.6.0
 Component:    base system
 Category:     feature requests
 Priority:     normal
 Assigned to:  degerrit
 Reported by:  degerrit
 Updated by:   Steven
 Status:       patch

Sorry didn't look at the patch carefully enough. Yes, we need XSS
protection: in practice, we always strore remote_addr() in the hostname
column in the db. Wherever that column is displayed, it needs to be
check_plain()ed.




Steven



Previous comments:
------------------------------------------------------------------------

April 14, 2005 - 01:42 : degerrit

In several modules, replace $_SERVER["REMOTE_ADDR"] with a function
which understands X-Forwarded-For.


Proxy servers often (sometimes, always?) insert "X-Forwarded-For"
headers which contains the "real" client IP address. I think it is
useful to log the client IP address in that situation.


This little snippet works for me, and I'd suggest something similar be
made into a function?


$headers = apache_request_headers();
if (array_key_exists('X-Forwarded-For', $headers)){
  $hostname=$headers['X-Forwarded-For'] . ' via ' .
$_SERVER["REMOTE_ADDR"];
} else {
  $hostname=$_SERVER["REMOTE_ADDR"];
}


I can make a patch proposal if people like the idea.




------------------------------------------------------------------------

May 21, 2005 - 23:44 : degerrit

Attachment: http://drupal.org/files/issues/remote_addr.diff (6.07 KB)

Here's my proposal for showing user IP addresses behind proxy servers,
especially in log files. It adds a function remote_addr() and replaces
all the uses of $_SERVER["REMOTE_ADDR"] with that function.


This is my first patch for Drupal, don't be too harsh one me :-)


A few notes:
- I patched session.inc because $_SERVER["REMOTE_ADDR"] is also called
in session.inc and common.inc is not available at that time it seems
- maybe I replaced too vigorously in common.inc and sessions.inc, it
works for me though
- maybe it should be renamed http_remote_addr or apache_remote_addr




------------------------------------------------------------------------

May 22, 2005 - 10:37 : Dries

Couple comments:



Drupal supports non-Apache webservers: apache_request_headers() might
not always be available.
Does this patch need input checking?  Depends on how/when/where the
remote address data is used.  Fact is that we don't want people to
inject XSS-stuff through HTTP headers.  Make sure to double-check this.
I'd rename the function to remote_address().  We usually don't
abbreviate words like 'address'.
The word 'via' can't be translated.
We write
}
else { and not 


} else {Just thinking up loud.  Is it easy to fake or spoof
X-Forwarded-For-headers?  If it is, it would be easy to trick Drupal
(eg. in the statistics.module's log pages we group by remote address). 
For the watchdog it doesn't matter, but for the statistics module it
does.  Maybe we need a toggle so remote_address() can be told not to
return the 'true IP address'.



------------------------------------------------------------------------

May 25, 2005 - 06:37 : Steven

X-Forwarded-For can be spoofed without any trouble. Just connect and
send the fake header. I don't think we should pay attention to it. If
we do, we need to also store the original IP address.


Perhaps we could do "real.ip.#.# (forwarded.ip.#.#)"