From an email from Benjamin Schrauwen

> As for the 172 issue - this has driven me crazy frankly - has nothing to do with mollom. When I switched hosts about 90 days ago, all of the comments, trackbacks, everything said the 172 address. No one on drupal forums, email support list could seem to help me. Finally my hosting provider told me it's because I am hosted in the cloud and that I have to pass along another code instead of asking for the ip normally. The code is:
> $_SERVER['HTTP_X_CLUSTER_CLIENT_IP']
>
> So I went into trackback module and comment module and changed the code in the actual module. And it started working great immediately - I saw the ip addresses correctly. Then yesterday, I looked again and it's back to 172 again. Now I can't figure out why. Any ideas would be very much appreciated.

Yes, getting a correct IP address of a HTTP connection seems to be an
issue all over the place. I found this list of variables which might
hold the visitor's IP address:

HTTP_CLIENT_IP
HTTP_X_FORWARDED_FOR
HTTP_X_CLUSTER_CLIENT_IP
HTTP_PROXY_USER
REMOTE_ADDR

Could you try too see which works.

With kind regards,
Benjamin.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dries’s picture

Dries’s picture

Version: 6.x-dev » 7.x-dev
Status: Active » Needs review
FileSize
1.75 KB

This is a problem for Mollom so I came up with a fix for DRUPAL-6 and CVS HEAD.

I don't have a cluster environment so I can't actually test the patch. I doubt anyone has.

Dries’s picture

*My turn to bump a patch*

R.Muilwijk’s picture

FileSize
4.44 KB

Adds a test for the ip address function

keith.smith’s picture

Minor style issue: if you have to reroll, please end comments with periods so that they form complete sentences. The variations of IP, ip and Ip should all probably be standardized as IP (I guess).

Dries’s picture

Version: 7.x-dev » 6.x-dev
Status: Needs review » Reviewed & tested by the community

I've made the changes suggested by Keith and committed the patch to CVS HEAD. The bootstrap.inc part should go into DRUPAL-6 as well as we're not always capturing the right address.

John Morahan’s picture

Status: Reviewed & tested by the community » Needs work

I don't know what happens in a cluster environment, but under normal circumstances this header can be spoofed.

grandcat’s picture

That's right, I'm of the same opinion like John Morahan. If it's a well configured cluster, the real IP should be passed through to the servers behind the proxy. And so, it's their task to perform this step. And I'm also sure that all these servers like APACHE or LIGHTTPD support this feature.

Dries’s picture

Status: Needs work » Fixed

You were right. The brackets were not placed properly. I fixed this in CVS HEAD. Thanks.

Gábor Hojtsy’s picture

Hm, what HEAD? What would be the patch to commit to 6.x?

Gábor Hojtsy’s picture

Status: Fixed » Needs work
John Morahan’s picture

Status: Needs work » Needs review
FileSize
3 KB

This one, I guess? This is a combination of #4 (minus the test) and http://cvs.drupal.org/viewvc.py/drupal/drupal/includes/bootstrap.inc?r1=...

John Morahan’s picture

FileSize
3 KB

Here's one with the style changes from #5 too.

markus_petrux’s picture

if (array_key_exists('HTTP_X_CLUSTER_CLIENT_IP', $_SERVER)) {
  $ip_address = $_SERVER['HTTP_X_CLUSTER_CLIENT_IP'];
}

The above code is a bit "dangerous". The X-Cluster-Client-Ip header ought to be checked against a whitelist (the same way it is done for the X-Forwarded-For header).

Maybe checks here could reuse the values stored in $reverse_proxy_addresses (see default.settings.php), but then the name of that variable would have to be changed to something more generic.

c960657’s picture

Version: 6.x-dev » 7.x-dev
Priority: Normal » Critical

Raising to critical, because this patch introduced a security issue in HEAD.

I think there need to be a variable containing the name of the HTTP header that can be trusted. If the reverse proxy is e.g. a Squid, X-Forwarded-For can be trusted, but X-Cluster-Client-Ip can not.

Status: Needs review » Needs work

The last submitted patch failed testing.

Bart Jansens’s picture

x-cluster-client-ip is just another non-standard header used by a load balancer. Calling it a cluster may sound fancy but it is no different from any other reverse proxy setup, so why add special code for it? As mentioned before, this is a security hole so this either needs to be fixed or rolled back.

Attached is a proposed fix: add another variable that can be set in settings.php to override which header to check. It defaults to x-forwarded-for as that is more or less the standard way of passing the client ip to the application, but can be changed to whatever header the hosting provider uses.

Bart Jansens’s picture

Status: Needs work » Needs review
Dries’s picture

Should we write tests for this? It seems like one of those things we'd want to have tests for ...

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

This should be fixed (or backed out) prior to the release of D7.

alexanderpas’s picture

how about we only check the HTTP_X_FORWARDED_FOR header (or a custom one like in #17) if REMOTE_ADDR seems to be an RFC 1918 address (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16)

markus_petrux’s picture

@alexanderpas: You may not want to trust all the nodes on an intranet. ;-)

sun.core’s picture

This sounds critical, tests for it are critical, and it should keep critical. (just wandering through the critical queue)

catch’s picture

Marked #621748: ip_address() is broken for multi-tier architectures as duplicate since this is much the same.

sun.core’s picture

Still critical. @Dries or anyone involved with this actual change: Any chance to write tests for starters? It's much harder for strangers to properly understand + test the proper behavior. We just need a baseline, others can fill out.

Bart Jansens’s picture

Actually there already is a test for this, it was added by the original patch.

#17 just needs a reroll and probably some explanation to go in default.settings.php

grendzy’s picture

Status: Needs work » Needs review
FileSize
3.85 KB

reroll with documentation in default.settings.php.

Status: Needs review » Needs work
Issue tags: -Security Advisory follow-up

The last submitted patch, 258397_reroll.patch, failed testing.

grendzy’s picture

Status: Needs work » Needs review
Issue tags: +Security Advisory follow-up

#28: 258397_reroll.patch queued for re-testing.

c960657’s picture

Status: Needs review » Needs work

I agree on the approach here. Just a few nits:

+      if (array_key_exists($reverse_proxy_header, $_SERVER)) {

I know the array_key_exists() was there already, but could you change this to empty() instead? This is more in line with what we do elsewhere.

+ * Set this value if your proxy server sends the client IP in a header other
+ * than HTTP_X_FORWARDED_FOR.

Elsewhere in default.settings.php the header is referred to as “X-Forwarded-For”. For consistency, I think the same should be used here (possibly with a brief mention about how the string is prefixed and converted to upper-case). When “X-Forwarded-For” is first mentioned in each function/file, it may be worth adding that the actual header used is configurable.

I think it should be mentioned that the header value format must be the same as that used by X-Forwarded-For, i.e. if the header contains more than one IP address (separated by comma), the last one is used. It is important, because it may be possible to circumvent the check if a proxy uses another convention.

+ * reverse_proxy_header accepts a string.

The data type is obvious from the example code line below. We don't specify the data type for other variables in default.settings.php, except for reverse_proxy and reverse_proxy_addresses (for the latter, the current description is wrong). I suggest we remove these altogether.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.86 KB

Re-roll.. changed to !empty() and tried to improve the docs a bit.

Status: Needs review » Needs work

The last submitted patch, 258397_1.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Can't reproduce the test fails, lets try again...

Berdir’s picture

#32: 258397_1.patch queued for re-testing.

c960657’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

teastburn85’s picture

Why does ip_address() use the rightmost entry from X-Forwarded-For header? Isn't the purpose of ip_address() to get the end-user/client's IP address? The documentation for X-Forwarded-For on wikipedia seems to state that the client's IP is the first mentioned IP (leftmost) and any other servers that touch the request append their IPs to the end of the list (rightmost). Am I misunderstanding how X-Forwarded-For is used in this situation?

Selecting the rightmost IP is breaking our Drupal setup where our servers are behind a CDN that passes the client's IP as the first IP in the X-Forwarded-For header.

Also posted this question on http://drupal.org/node/309586 since they are newest issues I could find.

Thanks in advance

grendzy’s picture

I think the reason is the left-most one can be forged. Perhaps an additional $conf variable is needed that instructs Drupal how many levels can be trusted?

It might help to know more about your setup. If the remote IP is on the left, what address is on the right in X-Forwarded-For? Does the CDN itself connect through another proxy?

teastburn85’s picture

That's true, the leftmost IP is the most easily forged. But what's the purpose of getting the proxy server's address (the rightmost) if what you need is the client's IP, forged or not? If the worry is a forged IP, then presumably the forger is trying to get around your IP ban, in which case retrieving your proxy's IP is rather useless. I think I'm not understanding something here because it really doesn't add up for me.

Our CDN configuration is an origin pull set up with our DNS pointing at their servers, which in turn reach out to ours when necessary. So for every request the user makes that actually gets to our servers (e.g. for pages that specify no caching at all or are dynamic), the CDN appends the X-Forwarded-For header with the user's IP first, then the the IP of the CDN server that fielded the call. An edited header from our site (with fake IPs):

GET /polls/my-poll-name HTTP/1.1
Host: mysite.example.com
User-Agent: Mozilla/5.0 (iPhone; U; CPU iPhone OS 3_1_2 like Mac OS X; en-us) AppleWebKit/528.18 (KHTML, like Gecko) Version/4.0 Mobile/7D11 Safari/528.16
Referer: http://mysite.example.com/
Accept: */*
Accept-Language: en-us
Accept-Encoding: gzip, deflate
Cookie: has_js=1; session_api_session=flbovpo95s73bt5d727tf7ru17
Via: [...some via headers with CDN hostnames...]
X-Forwarded-For: 22.231.113.64, 194.66.82.11
Connection: keep-alive

This request came from the CDN to our servers, so for us the $_SERVER['REMOTE_ADDRESS'] variable is useless for tracking a user by IP. The X-Forwarded-For header is in this form:

X-Forwarded-For: [END_USER_IP], [CDN_RECEIVER_IP]

Drupal's ip_address function returns [CDN_RECEIVER_IP] which only varies between the number of servers our CDN uses to accept user requests to our domain. When looking for the client's IP, I don't understand why the rightmost IP makes any sense or is helpful, even if the leftmost is forged.

roychri’s picture

Status: Needs work » Closed (fixed)

Same problem here. We have some LB pointing to proxies pointing to a proxy and then the origin server.
The rightmost ip on the list is NOT the client's IP.

Someone posted a patch on a issue (duplicate to this one) where he was removing all trusted ips from the list before using the leftmost rightmost. That would seem more prudent/broad, no?

http://drupal.org/files/issues/7X29YcLCtUMmeqrdQz67VW3RXwU.patch

Edited after @grendzy mentioned my typo.

grendzy’s picture

Status: Closed (fixed) » Needs work

This patch looks pretty good to me, though it no longer applies and I haven't tested it yet. (btw roychri I think you mean the *rightmost* address after removing the trusted IPs, which is what the patch you linked to implements.) Additional test coverage for multiple values in XFF would be good.

BTW the link above came from #621748: ip_address() is broken for multi-tier architectures

roychri’s picture

Status: Closed (fixed) » Needs work

@grendzy - A patch was already committed so I find it normal that the patch I mentioned cannot apply now.

I was not implying that this patch was better than the one that was committed. I only wanted to show a reference to something that was working in our case (where we have multiple proxies which appear in the XFF).

I would expect a new patch that would modify the existing code. Maybe I'll have some free time later to propose a patch.

catch’s picture

Status: Needs work » Closed (fixed)

Hmm, looks like this issue never actually covered the case on #621748: ip_address() is broken for multi-tier architectures, so we should rather unduplicate that one and leave this closed.

grendzy’s picture

teastburn85, roychri: a new patch is up at #621748: ip_address() is broken for multi-tier architectures, if you have time to test it out that would be appreciated.

teastburn85’s picture

@grendzy That patch looks good, but unfortunately doesn't apply to my situation. Since we are behind a CDN, it is bad practice and just plain unlikely for us to know all the IPs of server's that could be serving our content. The list would be very long since our provider has farms all over the world. Also, neither would they want to nor would we want them to notify us every time an IP changes or they add a new server or remove an old one.

I think there is a fundamental problem here in the approach, since it seems to work well for the vast majority of Drupal users, but doesn't work well for the ones that need to scale Drupal, which as I understand is the minority. Also, at this time in our particular situation, we're not worried about IP spoofing so much of the ip_address function isn't worth much for us. I don't know if this is the approach you might want to take in the future, but we've implemented a few $conf variables that allow bypassing of the reverse proxy address list check and instead might introduce a callback function or a hook to take care of the work so Drupal doesn't have to cover every situation on its own.

Thanks again

Damien Tournoud’s picture

@teastburn85: your problem is completely unrelated to what is being solved here. Your CDN probably has a way to give you the correct IP address of the client in a non-spoofable way. Drupal cannot and will not support every proprietary headers of every CDN over the world.

You can implement the logic you need to get the correct value from $_SERVER['REMOTE_ADDR'] right into your settings.php file, without any need to patch core.

roychri’s picture

@teastburn85: I agree with Damien. Even if you are not concerned with spoofed IP, the code should still handle that.
And the workaround of settings.php still works for you in your special case.

teastburn85’s picture

@Damien I wouldn't say my issue is completely unrelated, but maybe unrelated enough to be out of the scope of this discussion. In any case, I appreciate your help, but if $_SERVER['REMOTE_ADDR'] was all I needed you can trust I wouldn't be here now. I am here because the only way I could get Drupal to play nice with a standard XFF header was to hack the ip_address function, which I thought Drupal devs might like to know about. Agreed that Drupal shouldn't support every CDN, thats why my suggestion of a hook was included. If Drupal wants to be seriously considered as a production CMS for high traffic sites, it will have to grapple with these very same CDN issues.

Thanks

grendzy’s picture

but if $_SERVER['REMOTE_ADDR'] was all I needed you can trust I wouldn't be here now.

I think you misread Damien's post. What you need can be accomplished very simply without hacking core. The suggestion is assigning something to $_SERVER['REMOTE_ADDR'] when custom logic is needed. For example, if you must have the left-most address regardless of possible forgery, this one line in settings.php will solve your problem without hacking core:

$_SERVER['REMOTE_ADDR'] = current(array_map('trim', explode(',', $_SERVER['HTTP_X_FORWARDED_FOR'])));
joelcollinsdc’s picture

I found this very confusing and agree with teastburn85. For the future would allowing a user to specify a header as being "trusted" be a solution?

I created an issue to improve the documentation related to this:
http://drupal.org/node/1310250