Drupal does not fully work when using a reverse proxy

This is a good idea.

However, does this idea need to be expanded more for other uses of the IP address?

I can see that $_SERVER['REMOTE_ADDR'] is used in many places in these files in core:

includes/bootstrap.inc
includes/common.inc
includes/session.inc
modules/comment/comment.module
modules/poll/poll.module
modules/statistics/statistics.module

Not to mention contrib.

So, perhaps we need to have a Drupal specific variable called $drupal_ip_address or something that all modules can use. The raw HTTP variables will still be there if a module needs it.

If others agree on this, then I can work on it. The issue title will need to change too.

Assigning to myself.

Gerhard, can you please run this script from a browser on any machine on *.d.o that is using FCGI?

foreach ($_SERVER as $name => $value) {
  print "$name = $value\n";
}

Email me privately if you see anything that is sensitive.

Found an older issue for this. Says that the header can be spoofed.

http://drupal.org/node/20471

Here is a patch that should be portable. It does NOT require PHP as an Apache module unlike the previous patch (linked above), and will work with PHP as an FCGI.

What it does is replace all occurances of $_SERVER['REMOTE_ADDR'] with a call to the new remote_addr() function. remote_addr() checks if we have an X-Forwarded-For header, and takes the IP address in it. Otherwise, REMOTE_ADDR will be used.

I am not sure if filter_xss() is needed here or not. I left it in just in case.

Umm, it always helps if you have patches that actually work.

Here is one that at least does not produce errors.

If someone with ssh access can create a D6 site on drupal1 or drupal2, and install this patch and verify that the correct IP address is used in the session, comment, watchdog, poll and statistics modules.

Looks good, but can we rename remote_addr() to remote_address() or ip_address() ? We try not to abbreviate simple words. Thanks.

+  // If Drupal is behind a proxy, we use the X-Forwarded-For header
+  // instead of the straight IP address

should probably be part of the PHPdoc, rathern than an inline comment. I'd replace 'straight IP address' with the name of the actual header.

In the documentation, it would be great if we could mention the RFC that documents this behavior, although this is really, really minor.

Also, as per Gerhard's suggestion, we'll want to add an entry to the CHANGELOG.txt.

Here is an updated version.

- Added a new settings (under performance) that makes this explicit, to avoid header injection
- Changed function name to ip_address()
- Added a few missing places (in poll.module)
- Removed the check_plain, in accordance with the policy of filter on output, not input
- Added comment in CHANGELOG.txt

This text seems redudant:

+    '#description' => t('Is Drupal running behind a reverse proxy?')

The form description has typos/errors -- it doesn't read well.

+    '#description' => t('If Drupal is running to be behind a reverse proxy, enabled this setting. For example, if you have Squid installed, the IP addresses will not be correct in many part of Drupal, including watchdog, poll, statistics and comments. Enabling this setting will cause Drupal to use the X-Forwarded-For headers provided by the reverse proxy.'),

Either way, we need to keep in mind that the majority of the Drupal users have never heard about a "reverse proxy". For them, this settings will be super-confusing. I suggest we make add a short description of what a "reverse proxy" is, how they can figure out whether they are using one, and a "if unsure, disable this feature or it might open up a security hole" style message. To make a long story short: let's make this a bit easier to grok for people like our mothers. ;-)

We need to sanitize the X-Forwarded-For header: see http://osvdb.org/displayvuln.php?osvdb_id=23480 and http://secunia.com/advisories/18930/. I think we do this on output, but it might be good to double-check.

According to http://en.wikipedia.org/wiki/X-Forwarded-For, the X-Forwarded-For header can be a comma-separated list of IP addresses. Looks like the code isn't capable of handling that scenario.

Dries,

Remember that proxies can be implemented in two places. One is at the client side (e.g. an organization wants to cache content for users to reduce traffic, like AOL does), or server side (like we do with Drupal.org).

Anyways, your suggestions are implemented in this version of the patch.

- Better documentation for the user.
- Updated CHANGELOG.txt
- Use check_plain() on the address.
- Use the leftmost argument only.

killes,

If you can test this patch on s.d.o, that would be great.

kbahey: that will be a lot easier to grok for Joe Average. Thanks.

I'll wait for Gerhard to test this, and maybe for other people to suggest more subtle but important textual improvements. The code itself looks RTBC.

I'm sorry I don't have time to do a more complete review or to roll a modified patch right now, but I'm posting this slightly modified text in the hopes it may help.

Original:
In some cases involving large sites, Drupal may be running behind a reverse proxy. A reverse proxy is used to enhance the performance of heavily visited sites, and is normally used in dedicated hosting or multi-server environment. Examples are Squid and Pound. This setting lets Drupal examine the X-Forwarded-For headers and use the correct IP address of the remote client. The IP address of the remote client is used in various parts of Drupal, such as sessions, watchdog, poll, statistics and comments. If you are sure that Drupal is running to be behind a reverse proxy, enable this option. If you are on a shared host, or do not know what a reverse proxy is, then it is safe to keep this setting disabled.

Modified:
Enable this setting to determine the correct IP address of the remote client by examining information stored in the X-Forwarded-For headers. X-Forwarded-For headers are a standard mechanism for identifying client systems connecting through a reverse proxy server; reverse proxy servers are often used to enhance the performance of heavily visited sites and may also provide other site caching, security or encryption benefits. If this Drupal installation operates behind a reverse proxy, this setting should be enabled so that correct IP address information is captured in Drupal's session management, logging, statistics and access management systems; if you are unsure about this setting, or Drupal operates in a shared hosting environment, this setting should be disabled.

Slight modifications to Keith's version:

Enable this setting to determine the correct IP address of the remote client by examining information stored in the X-Forwarded-For headers. X-Forwarded-For headers are a standard mechanism for identifying client systems connecting through a reverse proxy server; such as Squid or Pound. Reverse proxy servers are often used to enhance the performance of heavily visited sites and may also provide other site caching, security or encryption benefits. If this Drupal installation operates behind a reverse proxy, this setting should be enabled so that correct IP address information is captured in Drupal's session management, logging, statistics and access management systems; if you are unsure about this setting, do not have a reverse proxy, or Drupal operates in a shared hosting environment, this setting should be set to disabled.

Any other English speakers who want to chip in? Angie?

(hit refresh as I was walking out the door).

Even better. Looks good, with the small nitpick that "reverse proxy server; such as Squid or Pound." should probably be "reverse proxy server, such as Squid or Pound." with a comma rather than a semicolon.

I've never used reverse proxy, I can't test the patch but I want to say something about the code.

• I think $remote_ip should be static.
• I don't know the differences between array_key_exists and isset but I think isset is faster and works same here.

Here is a version that implements Keith's wording improvements, as well as Erdem's suggestions.

I did not bother with isset() though, since now we only hit this code once per request, so the difference would be negligible.

Patch attached.

killes or dww, if you can try it on s.d.o that would be great.

Cool. Is that on d.o or s.d.o?

Can you say it is RTBC?

Any one else willing to test this?

Few people will be able to test this patch. If we're using it on drupal.org and that seems to work, we should be OK. I'm willing to commit it, and if it breaks, we can still fix it later on. :)

Patch no longer applies. Needs to be re-rolled.

Rerolled against today's HEAD.

Actually, there is a conflict in the patch -- look at the last blob.

Rerolled against current HEAD.

Committed to CVS HEAD. Thanks! :)

This topic came to my radar, so i will give some more input... i've missed many more PROXY checks... here is a more complete list of variables available on the net...

Sorry, the following is perl logic with regex... but it gives you and idea what is required to check. the order is impotant... the order or regex is important and will use the last match as the real client IP. Additional the committed patch misses SSL check. if SSL is active we don't need to check any proxy vars... i cannot see anything about this in the code (short review only).

my $ip=$ENV{'REMOTE_ADDR'} || '0.0.0.0';

$ip=$ENV{'HTTP_VIA'} if($ENV{'HTTP_VIA'} && $ENV{'HTTP_VIA'}=~s/.*\s(\d+)\.(\d+)\.(\d+)\.(\d+)/$1.$2.$3.$4/);

$ip=$ENV{'HTTP_X_FORWARDED_FOR'} if($ENV{'HTTP_X_FORWARDED_FOR'} && $ENV{'HTTP_X_FORWARDED_FOR'}=~s/^(\d+)\.(\d+)\.(\d+)\.(\d+)(\D*).*/$1.$2.$3.$4/ ); # e.g. Technical University of Darmstadt (Cisco Proxy), Germany, Network 130.83.0.0

$ip=$ENV{'HTTP_FORWARDED'} if($ENV{'HTTP_FORWARDED'} && $ENV{'HTTP_FORWARDED'}=~s/.*\s(\d+)\.(\d+)\.(\d+)\.(\d+)/$1.$2.$3.$4/g);

$ip=$ENV{'HTTP_CLIENT_IP'} if($ENV{'HTTP_CLIENT_IP'} && $ENV{'HTTP_CLIENT_IP'}=~s/^(\d+)\.(\d+)\.(\d+)\.(\d+)(\D*).*/$1.$2.$3.$4/ ); # e.g. T-Online Proxy (*largest* Germany ISP)

hass,

The patch is designed for the case where Drupal itself is running BEHIND a reverse proxy, not when the CLIENT is behind a proxy server.

In this case, the sysadmin is sure of the situation and that indeed a proxy is in front of the server. In case of a client, these headers can be easily forged.

There are other issues about proxies in general, please search for them and post your comment there.

The implementation chosen here was buggy, see http://drupal.org/node/169263

Guys, could someone please comment: https://drupal.org/node/1358312

not able to front a drupal site via proxypass and proxypassreverse

Thanks!