- Patch #621748 by grendzy, David Strauss: ip_address() is broken for multi-tier architectures.

2 files changed, 35 insertions, 15 deletions

diff --git a/includes/bootstrap.inc b/includes/bootstrap.inc
index 7760c3242..e0913cd49 100644
--- a/includes/bootstrap.inc
+++ b/includes/bootstrap.inc
@@ -2493,13 +2493,21 @@ function ip_address() {
         // If an array of known reverse proxy IPs is provided, then trust
         // the XFF header if request really comes from one of them.
         $reverse_proxy_addresses = variable_get('reverse_proxy_addresses', array());
-        if (!empty($reverse_proxy_addresses) && in_array($ip_address, $reverse_proxy_addresses, TRUE)) {
-          // The "X-Forwarded-For" header is a comma+space separated list of IP addresses,
-          // the left-most being the farthest downstream client. If there is more than
-          // one proxy, we are interested in the most recent one (i.e. last one in the list).
-          $ip_address_parts = explode(',', $_SERVER[$reverse_proxy_header]);
-          $ip_address = trim(array_pop($ip_address_parts));
-        }
+
+        // Turn XFF header into an array.
+        $forwarded = explode(',', $_SERVER[$reverse_proxy_header]);
+
+        // Trim the forwarded IPs; they may have been delimited by commas and spaces.
+        $forwarded = array_map('trim', $forwarded);
+
+        // Tack direct client IP onto end of forwarded array.
+        $forwarded[] = $ip_address;
+
+        // 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);
       }
     }
   }
diff --git a/modules/simpletest/tests/bootstrap.test b/modules/simpletest/tests/bootstrap.test
index 043cc4a3a..d8a9d8c1b 100644
--- a/modules/simpletest/tests/bootstrap.test
+++ b/modules/simpletest/tests/bootstrap.test
@@ -16,8 +16,9 @@ class BootstrapIPAddressTestCase extends DrupalWebTestCase {
 
     $this->remote_ip = '127.0.0.1';
     $this->proxy_ip = '127.0.0.2';
-    $this->forwarded_ip = '127.0.0.3';
-    $this->cluster_ip = '127.0.0.4';
+    $this->proxy2_ip = '127.0.0.3';
+    $this->forwarded_ip = '127.0.0.4';
+    $this->cluster_ip = '127.0.0.5';
     $this->untrusted_ip = '0.0.0.0';
 
     drupal_static_reset('ip_address');
@@ -42,23 +43,23 @@ class BootstrapIPAddressTestCase extends DrupalWebTestCase {
     // Test the normal IP address.
     $this->assertTrue(
       ip_address() == $this->remote_ip,
-      t('Got remote IP address')
+      t('Got remote IP address.')
     );
 
     // Proxy forwarding on but no proxy addresses defined.
     variable_set('reverse_proxy', 1);
     $this->assertTrue(
       ip_address() == $this->remote_ip,
-      t('Proxy forwarding without trusted proxies got remote IP address')
+      t('Proxy forwarding without trusted proxies got remote IP address.')
     );
 
     // Proxy forwarding on and proxy address not trusted.
-    variable_set('reverse_proxy_addresses', array($this->proxy_ip));
+    variable_set('reverse_proxy_addresses', array($this->proxy_ip, $this->proxy2_ip));
     drupal_static_reset('ip_address');
     $_SERVER['REMOTE_ADDR'] = $this->untrusted_ip;
     $this->assertTrue(
       ip_address() == $this->untrusted_ip,
-      t('Proxy forwarding with untrusted proxy got remote IP address')
+      t('Proxy forwarding with untrusted proxy got remote IP address.')
     );
 
     // Proxy forwarding on and proxy address trusted.
@@ -67,7 +68,16 @@ class BootstrapIPAddressTestCase extends DrupalWebTestCase {
     drupal_static_reset('ip_address');
     $this->assertTrue(
       ip_address() == $this->forwarded_ip,
-      t('Proxy forwarding with trusted proxy got forwarded IP address')
+      t('Proxy forwarding with trusted proxy got forwarded IP address.')
+    );
+
+    // Multi-tier architecture with comma separated values in header.
+    $_SERVER['REMOTE_ADDR'] = $this->proxy_ip;
+    $_SERVER['HTTP_X_FORWARDED_FOR'] = implode(', ', array($this->untrusted_ip, $this->forwarded_ip, $this->proxy2_ip));
+    drupal_static_reset('ip_address');
+    $this->assertTrue(
+      ip_address() == $this->forwarded_ip,
+      t('Proxy forwarding with trusted 2-tier proxy got forwarded IP address.')
     );
 
     // Custom client-IP header.
@@ -76,8 +86,10 @@ class BootstrapIPAddressTestCase extends DrupalWebTestCase {
     drupal_static_reset('ip_address');
     $this->assertTrue(
       ip_address() == $this->cluster_ip,
-      t('Cluster environment got cluster client IP')
+      t('Cluster environment got cluster client IP.')
     );
+
+    // Verifies that drupal_valid_http_host() prevents invalid characters.
     $this->assertFalse(drupal_valid_http_host('security/.drupal.org:80'), t('HTTP_HOST with / is invalid'));
     $this->assertFalse(drupal_valid_http_host('security\\.drupal.org:80'), t('HTTP_HOST with \\ is invalid'));
     $this->assertFalse(drupal_valid_http_host('security<.drupal.org:80'), t('HTTP_HOST with &lt; is invalid'));