419908 - Problems with determining IP of the end user

Status: RESOLVED FIXED

(Infrastructure & Operations Graveyard :: WebOps: Other, task)

Description

[Paul Booker]

Hello ,

We have a problem with spreadfirefox.com for a while now in that something has changed such that drupal cannot determine the ip of the end user with $_SERVER['REMOTE_ADRR'] but which seems to corrected by changing to $_SERVER['HTTP_FORWARDED_FOR'].

The problem is that we are having to modify code to watchdog ,affiliates and now were looking at having to change the code in various places for the spam module
as today it has flagged the IP of some other machine sitting between  the end user and the spreadfirefox server as a spam ip resulting in all requests being blocked.

Is it possible to change the environment of the spreadfirefox server so that things works as before and we can revert all of our code changes for ip determination back to $_SERVER['REMOTE_ADRR']   

Thanks Paul

Comment 1

[Jeremy Orem [:oremj]]

I'm pretty sure Wil gave you instructions on how to do this in another bug.

Ccing Wil.

Comment 2

[Paul Booker]

I am thinking that there should be no instructions for me to do anything to resolve this problem.

We can go through the code modifying core/contributed modules but if its not necessary i would rather not do that .

Would you let me know please if you know how this problem originated as nobody has actually said something like 'i understand whats going on here that'll be the caching server in front of the spreadfirefox web head' or something similar.

Thanks Paul

Comment 3

[Wil Clouser [:clouserw]]

I gave you a fix for this in bug 416415 comment #8

Comment 4

[Paul Booker]

Thanks Wil.I'll investigate how best to use hook_init() method to allow reassignment of $_SERVER['REMOTE_ADDR'] in the morning. 

Would you please let me know what caused the problem
with ip determination with  $_SERVER['REMOTE_ADDR'] and if it could not be corrected without modifying drupal code (where to be determined) ?

I am thinking that the drupal core and contributed modules use  $_SERVER['REMOTE_ADDR']  explicitly and there is probably good reasons for the drupal community to be doing 
this the below suggest that the reasons are one of security .


///////
From PHP.net ...

SECURITY RISK !

Never ever trust the values that comes from $_SERVER.

HTTP_X_FORWARDED, HTTP_X_FORWARDED_FOR, HTTP_FORWARDED_FOR, HTTP_FORWARDED, etc.. can be spoofed !

To get the ip of user, use only $_SERVER['REMOTE_ADDR'], otherwise the 'ip' of user can be easily changed by sending a HTTP_X_* header, so user can escape a ban or spoof a trusted ip.

Of course this is well know, but I don't see it mentioned in these notes..

If you use the ip only for tracking (not for any security features like banning or allow access to something by ip), you can also use HTTP_X_FORWARDED to get user's ip what are behind proxy.
///////

-Paul

Comment 5

[Paul Booker]

Hello,

I have done some research on drupal.org and have decided what i am going to do.

Working on the premise that there is a reverse proxy server in front of the spreadfirefox which means we can trust the X-Forwarded-For header i am going to roll a patch for Drupal 5 that has currently been released with Drupal 6 to solves this type of problem comprehensively


http://drupal.org/node/142773
http://en.wikipedia.org/wiki/X-Forwarded-For

Ill submit the patch here later today 

-Paul Booker

Comment 6

[Paul Booker]

Hello ,

Would you please commit the following to production ....

includes/bootstrap.inc
includes/common.inc
includes/session.inc
modules/comment/comment.module
modules/poll/poll.module
modules/statistics/statistics.module
sites/all/modules/sfx/sfx_affiliates/sfx_affiliates.module
sites/all/modules/spam/spam.module

Modifications allow the IP forwarded by our reverse proxy server to be captured
everywhere as the IP of the remote user/proxy for drupal core modules and the contributed spam module. Also our sfx_affiliates.module has also be modified to 
use the new code for determining remote IP's

Would you also delete all of the records in all of the spam_* tables and we will setup the filters again when the spam module is renabled.

Thanks for  your help.

I'll attach patches shortly ..

Paul

Comment 7

[Paul Booker]

Attachment: bootstrap.inc.1 patch

Comment 8

[Paul Booker]

Attachment: bootstrap.inc.2 patch

Comment 9

[Paul Booker]

Attachment: comment.module patch

Comment 10

[Paul Booker]

Attachment: common.inc patch

Comment 11

[Paul Booker]

Attachment: poll.module patch

Comment 12

[Paul Booker]

Attachment: session.inc patch

Comment 13

[Reed Loden [:reed]]

None of these patches are valid patches.

Comment 14

[Paul Booker]

Attachment: sfx_affiliates.module patch

Comment 15

[Paul Booker]

Attachment: spam.module patch

Comment 16

[Paul Booker]

Attachment: statistics.module patch

Comment 17

[Paul Booker]

yep , ill do that again ..

Comment 18

[Paul Booker]

Attachment: bootstrap.inc.1 patch

Comment 19

[Paul Booker]

Attachment: bootstrap.inc.2 patch

Comment 20

[Paul Booker]

Attachment: comment.module patch

Comment 21

[Paul Booker]

Attachment: common.inc patch

Comment 22

[Paul Booker]

Attachment: poll.module patch

Comment 23

[Paul Booker]

Attachment: session.inc patch

Comment 24

[Paul Booker]

Attachment: sfx_affiliates.module patch

Comment 25

[Paul Booker]

Attachment: spam.module patch

Comment 26

[Michael Morgan [:morgamic]]

Paul why didn't you do svn diff > diff.patch and submit one recursive diff?

Comment 27

[Paul Booker]

Attachment: statistics.module patch

Comment 28

[Paul Booker]

(In reply to comment #26)
> Paul why didn't you do svn diff > diff.patch and submit one recursive diff?
> 

same reason i haven't had a cup of tea this afternoon (lol)

Comment 29

[Michael Morgan [:morgamic]]

Paul I need you to change directories to the root of your check out and run:
svn diff > diff.patch then upload the resulting diff.patch.  I think you're missing some items and this would pick up those items and simplify the patch.

Comment 30

[Paul Booker]

Hi Micheal ,

The command "svn diff" does not result in anything that can be piped to diff.patch as i have commited all my local changes with "svn commit" and so there is no difference now with my working copy and what is in the subversion repository

What items are missing Micheal ?

Thank Paul

Comment 31

[Paul Booker]

Just letting you know Micheal i am going offline for a couple of hours , thanks for your help   .

Comment 32

[Jeremy Orem [:oremj]]

You can specify the revisions with svn diff using the -r option.

`svn help diff`

Comment 33

[Michael Morgan [:morgamic]]

1) you should not commit code that hasn't been reviewed or tested -- even in Firefox or other web apps we don't do this

2) generating diffs and understanding the release/deployment cycle and source control (SVN) is very important for a developer, and I don't think things are lining up here

For 1) I need you to hold off on committing code that is not tested or reviewed.  If you can't do this, we will revoke your SVN access.

For 2) the ability to diff any revision is available.  In this case:
svn diff -r10629:10801 > diff.patch

Instructions on how to do this are in the SVN manual:
http://svnbook.red-bean.com/en/1.0/ch03s05.html#svn-ch-3-sect-4.3.2

Comment 34

[Paul Booker]

1. Commiting code to subversion only when it has been reviewed makes sense lets do that in future (is there a wiki page that you can point me to regarding procedure and ill read it ) 

2. Yes i know that  ,thanks


Can i clarify what you want me to do now , shall we revert back what i have commited to subversion today and ill make my changes again to my working copy and submit a diff or can you appraise the changes that i have commited to subversion  (this time)

Just let me know what you want me to do and ill do it

KR
Paul

Comment 35

[Paul Booker]

Just to be clear  the reason i commit my changes to subversion is so that i can test on stage (as stage is updated periodically from subversion ) what i have tested on my local machine.

In fact i am often asked have you tested this on stage .

Comment 36

[Michael Morgan [:morgamic]]

Stage is for staging, and by the time it gets there the code should be reviewed and tested in a development environment.  Stage is NOT a development environment.  Every change you work on locally should be tested locally and committed after review.  When it hits stage whether or not it will work should not be up in the air -- there should be high confidence that it will work as expected.

As far as what to do:
* make all your patches obselete, nobody is going to review 12 patches or whatever individually
* upload the unified recursive diff
* request review on that diff

Comment 37

[Paul Booker]

Everything is clear , thanks  .

Comment 38

[Paul Booker]

Please review the following patch and let me know when i can push to the stage server , thanks

Comment 39

[Paul Booker]

Attachment: ip_address patch

please review

Comment 40

[Michael Morgan [:morgamic]]

Paul -- there is no way to do this without changing all these modules?

Comment 41

[Paul Booker]

It may be possible to use one of the bootstrap.inc hook_init() methods to reassign the global variable $_SERVER['REMORE_ADDR'], i would say that could work . 

The current patch we have works so i would propose we do this as the approach is simple and straightforward and has been reviewed by a number of drupal developers and commited into D6 by Dries.

I am thinking it may be possible to configure the proxy so that what we see with $_SERVER['REMORE_ADDR'] on spreadfirefox is in fact the remote IP. Then we would have to make no changes to the scripts and everything would work as it did before .

Let me know what you want me to do .

Regards
Paul Booker
 

Comment 42

[Jeremy Orem [:oremj]]

Why don't we just stick $_SERVER['REMOTE_ADDR'] = $_SERVER['HTTP_X_FORWARDED_FOR']; in settings.php?

settings.php is loaded in the first bootstrap phase "DRUPAL_BOOTSTRAP_CONFIGURATION".

Comment 43

[Paul Booker]

Thats sounds like a good idea , let me look into that .

Thanks Jeremy.

Comment 44

[Paul Booker]

Yes that will do nicely Jeremy .

Lets add .. 

$_SERVER['REMOTE_ADDR'] = $_SERVER['HTTP_X_FORWARDED_FOR'];
to settings.php as you advised

I tried this approach on my local machine with a '123.456.789' and it works fine.

Would you please add the above line to settings.php on the staging server as the settings.php configuration file is not in subversion and i will then test on staging server

This is the simplest of all approaches , thanks

Paul  

Comment 45

[Jeremy Orem [:oremj]]

Sounds good.  Morgamic or Wil are you alright with this approach?

Comment 46

[Jeremy Orem [:oremj]]

I think the whole addition should be:
    if (array_key_exists('HTTP_X_FORWARDED_FOR', $_SERVER)) {
      $ip_array = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']);
      // If there are several arguments, the leftmost one is the farthest client
      $_SERVER['REMOTE_ADDR'] = $ip_array[0];
    }

Comment 47

[Paul Booker]

Thanks again  , that is something we need to keep .

Works ok , on my local machine .

Comment 48

[Wil Clouser [:clouserw]]

(In reply to comment #45)
> Sounds good.  Morgamic or Wil are you alright with this approach?
> 

According to http://drupal.org/node/169263 your code in comment #46 is wrong and you should be taking the last element, not the first.  I haven't tested either way - it sounds like Paul has.  

Paul - are you saying the code in comment #46 is correct?

Comment 49

[Jeremy Orem [:oremj]]

I'm pretty sure the Netscaler always replaces the whole thing with just the clients IP, so we could probably skip the explode altogether.

Comment 50

[Paul Booker]

> According to http://drupal.org/node/169263 your code in comment #46 is wrong
> and you should be taking the last element, not the first.  I haven't tested
> either way - it sounds like Paul has.  
> 
> Paul - are you saying the code in comment #46 is correct?

i can't confirm that the code is correct either way it looks as though in light of the above information that we now need to use ..

 if (array_key_exists('HTTP_X_FORWARDED_FOR', $_SERVER)) {
      $ip_array = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']);
      // If there are several arguments, the leftmost one is the farthest
client
      $_SERVER['REMOTE_ADDR'] = array_pop($ip_array);
    }

$remote_ip = array_pop($ip_array); 

Is it possible for you guys to test this script ?

Comment 51

[Paul Booker]

ignoring ..

"$remote_ip = array_pop($ip_array); "

Comment 52

[Michael Morgan [:morgamic]]

Comment on attachment 306389 [details] [diff] [review]
ip_address patch

-r, there is a cleaner and simpler way to do this.

Comment 53

[Paul Booker]

Are we going with adding ...

 if (array_key_exists('HTTP_X_FORWARDED_FOR', $_SERVER)) {
      $ip_array = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']);
      // If there are several arguments, the leftmost one is the farthest
client
      $_SERVER['REMOTE_ADDR'] = array_pop($ip_array);
    }

in settings.php  ?

Comment 54

[Jeremy Orem [:oremj]]

I vote yes.  Does anyone object?

Comment 55

[Paul Booker]

I vote yes as

 if (array_key_exists('HTTP_X_FORWARDED_FOR', $_SERVER)) {
      $ip_array = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']);
      $_SERVER['REMOTE_ADDR'] = array_pop($ip_array);
 }


is consistent with what is in D6 ..


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, i.e. the last one.
        $ip_address = array_pop(explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']));
      }
    }
  }

  return $ip_address;
}

Comment 56

[Paul Booker]

Any progress here we really need to get this IP issue resolved so that we can (on Alix's directive) reenable the spam module so that we can be effective with handling spam which is a critical problem on spreadfirefox and should be addressed as soon as possible in my opinion.

Paul

Comment 57

[Jeremy Orem [:oremj]]

Morgamic or Wil are you okay with the approach?

Comment 58

[Wil Clouser [:clouserw]]

Seems like it should work

Comment 59

[Michael Morgan [:morgamic]]

Works for me.  Where's the patch?

Comment 60

[Paul Booker]

Attachment: revised settings.php

This file is not in subversion.

Comment 61

[Michael Morgan [:morgamic]]

Comment on attachment 307754 [details]
revised settings.php 

Paul if there is a dist file in SVN you should update that, too.

Comment 62

[Jeremy Orem [:oremj]]

Made the change.  You should probably commit that change to settings.php.dist

Comment 63

[Paul Booker]

Attachment: settings.php.dist patch

Commmited immediately into subversion as this has already been reviewed.

Comment 64

[Paul Booker]

Thanks Jeremy . Good call .

I have noticed that this patch is showing some hidden characters ^M . I don't know 
what has caused this but as soon as i have figured what that's about i'll resubmit the patch again .

Regards Paul.

Comment 65

[Paul Booker]

Discovered a problem in watchdog with the reassignment of $_SERVER['REMOTE_ADDR']  / Hostanme...

Type	access denied
Date	Thursday, March 6, 2008 - 20:33
User	Anonymous
Location	http://www.spreadfirefox.com/user/183458
Referrer	http://www.spreadfirefox.com/
Message	user/183458
Severity	warning
Hostname	75.21.155.78, 66.249.84.14

It looks as though were getting the full $_SERVER['HTTP_X_FORWARDED_FOR'] for the hostname. I don't know for certain but i am thinking that maybe we need to define $_SERVER['REMOTE_ADDR'] as global in settings.php ,i.e ..


global $_SERVER['REMOTE_ADDR'];

if (array_key_exists('HTTP_X_FORWARDED_FOR', $_SERVER)) {
  $ip_array = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']);
  $_SERVER['REMOTE_ADDR'] = array_pop($ip_array);
}

If you have any insights on this , please let me know

Investigating ...

Comment 66

[Jeremy Orem [:oremj]]

I see this on line 717 of bootstrap.php:
  db_query("INSERT INTO {watchdog} (uid, type, message, severity, link, location, referer, hostname, timestamp) VALUES (%d, '%s', '%s', %d, '%s', '%s', '%s', '%s', %d)", $user->uid, $type, $message, $severity, $link, $request_uri, referer_uri(), $_SERVER['HTTP_X_FORWARDED_FOR'], time());

Comment 67

[Paul Booker]

http://viewvc.svn.mozilla.org/vc/projects/spreadfirefox.com/trunk/includes/bootstrap.inc?view=markup 

db_query("INSERT INTO {watchdog} (uid, type, message, severity, link, location, referer, hostname, timestamp) VALUES (%d, '%s', '%s', %d, '%s', '%s', '%s', '%s', %d)", $user->uid, $type, $message, $severity, $link, $request_uri, referer_uri(), $_SERVER['REMOTE_ADDR'], time());

It would be great if we could push all of our changes through to production . We will then have the site working correctly .

Some notes..
Before Morgmaic's directive on submitting patches for review and when to commit
i had commited changes to a selection of drupal pages that changed $_SERVER['REMOTE_ADDR']/$_SERVER['HTTP_X_FORWARDED_FOR'] to ip_address() i have since commited a further change to revert ip_address() everywhere back to $_SERVER['REMOTE_ADDR'] so that there is no chance of ip_address() being pushed to production.

Comment 68

[Jeremy Orem [:oremj]]

Would you please list all the bugs that need to be pushed.

Comment 69

[Paul Booker]

It's all related to this bug

I just need to push these files so ip_address() is reverted
back to $_SERVER['REMOTE_ADDR'] 

includes/bootstrap.inc
includes/common.inc
includes/session.inc
modules/comment/comment.module
modules/poll/poll.module
modules/sfx/sfx_affiliates/sfx_affiliates.module
modules/statistics/statistics.module
sites/all/modules/spam/spam.module

we can then move forward with other things

Comment 70

[Jeremy Orem [:oremj]]

U    includes/bootstrap.inc

Nothing else differed from -r HEAD except modules/sfx/sfx_affiliates/sfx_affiliates.module which has a lot of changes.

Comment 71

[Paul Booker]

Whats is the difference betweeen what is in subversion/stage and what is on production for sfx_affiliates.module ?

Thanks for your help here Jeremy

Comment 72

[Jeremy Orem [:oremj]]

It is at Revision: 10397

Comment 73

[Paul Booker]

I don't see that revision number .. 

http://viewvc.svn.mozilla.org/vc/projects/spreadfirefox.com/trunk/modules/sfx/sfx_affiliates/sfx_affiliates.module?view=log

Confused , sorry .

Comment 74

[Jeremy Orem [:oremj]]

[root@mradm01 www.spreadfirefox.com]# svn info modules/sfx/sfx_affiliates/sfx_affiliates.module 
Path: modules/sfx/sfx_affiliates/sfx_affiliates.module
Name: sfx_affiliates.module
URL: http://svn.mozilla.org/projects/spreadfirefox.com/trunk/modules/sfx/sfx_affiliates/sfx_affiliates.module
Repository Root: http://svn.mozilla.org
Repository UUID: 4eb1ac78-321c-0410-a911-ec516a8615a5
Revision: 10397
Node Kind: file
Schedule: normal
Last Changed Author: [e-mail]
Last Changed Rev: 10328
Last Changed Date: 2008-02-14 02:45:03 -0800 (Thu, 14 Feb 2008)
Text Last Updated: 2008-02-15 15:33:01 -0800 (Fri, 15 Feb 2008)
Checksum: 4ca7fc602bea0f30fa2ef0a70e00088b

So I guess the actual rev would be at 10328

Comment 75

[Jeremy Orem [:oremj]]

Changes:
http://viewvc.svn.mozilla.org/vc/projects/spreadfirefox.com/trunk/modules/sfx/sfx_affiliates/sfx_affiliates.module?r1=10328&r2=11042