ip_address still buggy

A possible alternative would to move the option to settings.php, but then it would have to be asked at install time, maybe.

PS: Another issue that's somehow related: 185302 (trust only known proxy IPs).

Hi again,

hmm... if D6 is going to have to code to offer support of the XFF header, then I guess it needs a bit more attention. Flagging as critical, then you decide ;)

The fact is that if you use Squid in accellerator mode, then you know that Remote-Addr must match a known IP address, so you shouldn't trust the XFF header if that doesn't match. Please, see 185302 which I think is related to this issue, affects the same code, so I marked that one as a dup of this.

This is of course besides what's been mentioned by killes above.

Cheers

So, what you will want to do is to move the stuff into the install configure form, so it can be written into settings.php.

Not sure about this. Drupal.org moved to squid after being operational for over 6 years. That means that a field in the configure form wouldn't have helped us.

Well, that means a message on the settings form then stating "please update your settings.php". Not ideal. But this is a very advanced setting, if you can set up squid, you surely are advanced enough.

Maybe you could add an advanced settings section to the install form and then write settings.php accordingly. You need to provide two options: $reverse_proxy and (maybe) $reverse_proxy_ips which would be a comma separated list of your proxy server IPs.

You could simply add these variables with default values in a new section in settings.php with enough documentation on it, so an installation based on proxy server has the possibility to set these values and Drupal does the rest, within ip_address(), which is somehow "incomplete" now.

This issue does not break how Drupal works, so I don't think why it is critical. I tried to understand this comment where markus_petrux decided this is critical, but I cannot:

if D6 is going to have to code to offer support of the XFF header, then I guess it needs a bit more attention. Flagging as critical, then you decide ;)

Drupal 6 writes code itself? :)

Well, we offering it as a feature, but it does not work. We should not ship D6 without either fixing it or removing the feature. So, yes, I think it is indeed critical.

Acknowledged. So it seems as nobody provides a fix, we should remove this feature.

Well, the problem now (in current HEAD) I think, is that ip_address() has been "extended" with an incomplete solution, and that may allow spoofing the client address, if the web server can be reached directly. This should be removed or fixed.

If feature is removed, then one can simply parse/fix the $_SERVER array in settings.php, where these PHP globals have already been sanitized, and almost before than anything else.

If feature is fixed for D6+, then one possible approach could be to move the options used inside ip_address() to settings.php, then all you need to provide is a bit of doc for those that use a proxy/web server environments. In the future, this could be somehow included in install script or whatever...

Can you help with providing a patch of what should be removed? (It does not seem like there are workable suggestions for improvements above, so I am looking at removing the bugos functionality).

Not in patch form, 'cause I don't have the environment for that at this moment, but I can help you by checking current code in D6 and posting 2 options: 1) removing the feature, 2) moving the reverse proxy settings to settings.php, which I think can solve the issue.

Then you decide how to proceed.

...be back later, downloading D6 and checking...

Hi, here's the changes to remove the feature:

Step 1: Edit file modules/system/system.admin.inc and remove the following lines:

  $form['reverse_proxy'] = array(
    '#type' => 'fieldset',
    '#title' => t('Reverse proxy'),
    '#description' => t('Proper extraction of client IP addresses when Drupal is behind a reverse proxy.'),
  );

  $form['reverse_proxy']['reverse_proxy'] = array(
    '#type' => 'radios',
    '#title' => t('Reverse proxy'),
    '#default_value' => variable_get('reverse_proxy', FALSE),
    '#options' => array(t('Disabled'), t('Enabled')),
    '#description' => t('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.'),
  );

Step 2: Edit file includes/bootstrap.inc and remove the following lines from function ip_address():

    if (variable_get('reverse_proxy', 0) && array_key_exists('HTTP_X_FORWARDED_FOR', $_SERVER)) {
      // If there are several arguments, we need to check the most
      // recently added one, ie the last one.
      $ip_address = array_pop(explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']));
    }

I believe that's it. Now, I'll post option 2, which would be "fixing" the feature.

Here's option 2. Well, this is just a possible way to do it.

Step 1: Edit file modules/system/system.admin.inc and remove the following lines:

  $form['reverse_proxy'] = array(
    '#type' => 'fieldset',
    '#title' => t('Reverse proxy'),
    '#description' => t('Proper extraction of client IP addresses when Drupal is behind a reverse proxy.'),
  );

  $form['reverse_proxy']['reverse_proxy'] = array(
    '#type' => 'radios',
    '#title' => t('Reverse proxy'),
    '#default_value' => variable_get('reverse_proxy', FALSE),
    '#options' => array(t('Disabled'), t('Enabled')),
    '#description' => t('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.'),
  );

Step 2: Edit file includes/bootstrap.inc and change the code in function ip_address() like this:

function ip_address() {
  static $ip_address = NULL;

  if (!isset($ip_address)) {
    $ip_address = $_SERVER['REMOTE_ADDR'];
    if (variable_get('reverse_proxy', 0) && array_key_exists('HTTP_X_FORWARDED_FOR', $_SERVER)) {
      // 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)) {
        // If there are several arguments, we need to check the most
        // recently added one, ie the last one.
        $ip_address = array_pop(explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']));
      }
    }
  }

  return $ip_address;
}

Step 3: Edit file sites/default/default.settings.php and the reverse proxy settings there. A possible way to do it could be coding the default $conf values that are commented like this:

# $conf = array(
#   'site_name' => 'My Drupal site',
#   'theme_default' => 'minnelli',
#   'anonymous' => 'Visitor',
#   /**
#    * reverse_proxy accepts a boolean value.
#    *
#    * 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.
#    */
#   'reverse_proxy' => TRUE,
#   /**
#    * reverse_proxy accepts an array of IP addresses.
#    *
#    * Each element of this array is the IP address of any of your reverse
#    * proxies. Filling this array Drupal will trust the information stored
#    * in the X-Forwarded-For headers only if Remote IP address is one of
#    * these, that is the request reaches the web server from one of your
#    * reverse proxies. Otherwise, the client could directly connect to
#    * your web server spoofing the X-Forwarded-For headers.
#    */
#   'reverse_proxy_addresses' => array('a.b.c.d', ...),
# );

More or less, I believe that's it. I hope that heps, at least to see the "problem".

Cheers

Well, seems like we can fix this with settings in settings.php, and we can expect advance people (who set up proxies) to be able to edit the settings there, so option B seems to be workable. Anybody up for doing that in patch form?

I just went ahead and created a patch for markus_petrux's option 2 listed above. I myself don't know anything about actually using it or testing it, just wanted to help with the grunt work of patch creation.

add1sun: Thanks!

Patch looks good, but it obviously needs some testing. Gerhard?

I've applied the patch on scratch and configured the site accordingly. Testing indicates that the correct IP gets found.

Great, thanks, committed.

There are more issues when caching is enabled, see http://drupal.org/node/219825