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.

CommentFileSizeAuthor
#1 remote_addr.diff6.07 KBdegerrit

Comments

degerrit’s picture

degerrit commented
Version: » 4.6.0
Assigned: Unassigned » degerrit
StatusFileSize
new remote_addr.diff6.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

dries’s picture

dries commented

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'.
Steven’s picture

Steven commented

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.#.#)"

Steven’s picture

Steven commented

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.

degerrit’s picture

degerrit commented

I will re-write the patch with Dries' suggestions, I haven't had much time this week. I found some interesting examples of scripts doing roughly the same, none of them seem to do input checking (which I hadn't thought of either, but is obviously very needed!). Good thing there are people like Dries around, eh!

There's also another possible header: HTTP_CLIENT_IP, I've never seen it but then I don't log all my clients' headers either ;-)

Steven’s picture

Steven commented

In Drupal we tend to store the original information in the db, escaping or formatting on output as needed. All places where the hostname is output should be okay after some recent changes.

robin monks’s picture

robin monks commented
Status: Needs review » Needs work

Moving.....

Robin
I ♥ Bugz

markus_petrux’s picture

markus_petrux commented

X-Forwarded-For can be spoofed without any trouble.

Absolutely. Just create a node using PHP format with the following contents:

drupal_http_request('http://www.example.com', array(
  'X-Forwarded-For' => '1.2.3.4'
));

Just a question, why would you store something that can not be trusted?

-1 for this, also there are several other ways to hide the real IP, so...

degerrit’s picture

degerrit commented

Given the feedback, perhaps this change isn't such a good idea (although most headers would probably be legitimate, it's true that it is much easier to spoof than an IP address).
But I can answer your question : I'm currently hosting my site via a reverse-proxy; so all accesses look like the same IP to my server. I was just 'scratching my own itch' as they say ...

markus_petrux’s picture

markus_petrux commented

Maybe if Drupal (and contrib modules) was always accessing the user IP through a function (say remote_address()), then you could simply adatp it for your particular need. That would be easier than search/replacing all accurrences of $_SERVER['REMOTE_ADDR'].

magico’s picture

magico commented
Version: 4.6.0 » x.y.z
kbahey’s picture

kbahey commented
Version: x.y.z » 6.x-dev
Status: Needs work » Closed (duplicate)

This is now duplicated by http://drupal.org/node/142773