ip_address() is broken for multi-tier architectures

+++ includes/bootstrap.inc	2009-10-29 16:08:09 +0000
@@ -1439,15 +1439,28 @@
+      // Turn XFF header into an array.
+      $forwarded = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']);
+      
+      // Tack direct client IP onto end of forwarded array.
+      $forwarded[] = $ip_address;
+      
+      // Trim the forwarded IPs; they may have been delimited by commas and spaces.
+      $forwarded = array_map('trim', $forwarded);

I would do the array_map() on the same line as the explode. Is there a need to trim the IP address from REMOTE_ADDR?

+++ includes/bootstrap.inc	2009-10-29 16:08:09 +0000
@@ -1439,15 +1439,28 @@
+      // Eliminate all trusted IPs.
+      $untrusted = array_diff($forwarded, $reverse_proxy_addresses);
+      
+      // The right-most IP is the most specific we can trust.
+      $ip_address = array_pop($untrusted);

This do not perform as strictly as intended:

+    // Only use parts of the X-Forwarded-For (XFF) header that have followed a trusted route.
+    // Specifically, identify the leftmost IP address in the XFF header that is not one of ours.
+    // An XFF header is: X-Forwarded-For: client1, proxy1, proxy2

We need something like:

do {
  $candidate = array_pop($forwarded);
}
while (in_array($candidate, $reverse_proxy_addresses));

$ip_address = $candidate;

This review is powered by Dreditor.

This is a duplicate of #621748: ip_address() is broken for multi-tier architectures - which has tests. Marking duplicate so they can be combined.

Whoops, meant #258397: IP address identification not broad enough

7X29YcLCtUMmeqrdQz67VW3RXwU.patch queued for re-testing.

Back to duplicate.

The other issue was never combined with this one and didn't fix the specific issue, but has since been committed, so re-opening this, sorry :(

David's fix, with additional test for the multi-tier environment. REMOTE_ADDR is no longer trimmed. Also includes some minor style / doc fixes.

Committed to CVS HEAD for inclusion in Drupal 7. This should probably be backported to Drupal 6.

subscribe from #705718: ip_address() in bootstrap.inc returns invalid ip address when using 2 proxies
Direct backport from 7.x attached

I think it's good that the reverse_proxy_header variable is included in the backport, but there's some additional documentation that goes with it in default.settings.php

added in the documentation to default.settings.php

I think the testing bot is ignoring the patch due to the -D6 in the name. Attaching same patch from #18, renamed.

@janusman
This patch is for 6.x it was already applied for 7.x

Have you tested this on 6.x yet?

bad bot! This is a D6 patch.

This is a problem for Pantheon as well. Subscribing.

Did this patch make it into Drupal7 _release_? Or, n/a 'til next point release ... ?

I use this patch; anyone else want to test and give it the thumbs up so we can RTBC this?

This is in D7

#14: 621748_multi_tier.patch queued for re-testing.

#19: drupal-621748-19.patch queued for re-testing.
(edit: just trying to figure out how to get this patch tested.)

@verta
That patch needs to be re-rolled from git.

OK, thanks.

What about situation when we don't know exact IP address of trusted proxy? But we're sure that proxy exists. For example, we have a load balancer with dynamic internal IP address. So, we can't just add its IP to the list of reverse_proxy_addresses in settings.php, because it would work until the next IP address change.

My suggestion is to support "*" in reverse_proxy_addresses variable in addition to fixed IP addresses.

Dmitriy.trt: see #1310250: Improve reverse proxy ip address handing commenting and documentation.

Setting $_SERVER['REMOTE_ADDR'] directly in settings.php is not perfect solution, but it is acceptable. Thanks.

No action for over a year indicates this isn't critical; also if this is still present in 7.x or 8.x, should be marked 8.x and backported as appropriate.

This is fixed in 7.x (#15) and thus 8.x as well.
Main reason this isn't getting a lot of attention is most people who have this issue switch to pressflow. I'll see if I get some time today and do a re-roll of this.

Updated the patch. Would appreciate some testing.

Rob,

I'm going to test the patch this week. I'll let you know how it goes.

Best regards,

John Kealy

Rob,

Patch passed test on staging and was moved to production. The patch has solved the infinite redirect problem we were getting earlier on IP based logins. Thanks!

Cheers,

JK

Can confirm patch #36 works in production.