The implementation to check the XFF header is buggy in

http://drupal.org/node/142773

We need to check the most recently added IP.

$remote_ip = array_pop($ip_array);

CommentFileSizeAuthor
#1 ip.patch911 bytesGerhard Killesreiter
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gerhard Killesreiter’s picture

Status: Active » Needs review
FileSize
911 bytes
Dries’s picture

I think you have a point. How did you ran into this problem? Code looks good but it would be great if khalid (original author) could confirm this.

kbahey’s picture

When I wrote the original, I went by what sparse documentation I could find on the net for X-Forwarded-For. I did not have a proxy setup so I can test it.

This http://en.wikipedia.org/wiki/X-Forwarded-For says that the left most argument is the farthest address, and hence the client.

X-Forwarded-For: client1, proxy1, proxy2

where the value is a comma+space separated list of IP addresses, the left-most being the farthest downstream client, and each successive proxy that passed the request adding the IP address where it received the request from. In this example, the request passed proxy1, proxy2, and proxy3 (proxy3 appears as the client).

The source for this page is here http://www.openinfo.co.uk/apache/index.html which in turn takes it from http://web.warhound.org/mod_extract_forwarded/README (an Apache extension for parsing X-Forwarded-For). I can't find a formal RFC that describes this header, and even if we find that, this does not mean that all implementations would follow the RFC standard.

It seems that it did work except for some subtle differences uncovered by the http:BL module, which Gerhard took care of.

What Gerhard's patch does is do the opposite: pick the rightmost IP address. If this works on scratch or on d.o itself, then it seems I don't see why we should not use it.

Dries’s picture

I logged into drupal.org, and created a little test script to print the HTTP_X_FORWARDED_FOR header. Turns out that it only contains the client's IP (my IP) -- the IP address of the Squid proxy was not part of the string. In other words, the function ip_address() properly parses the IP address from that string. Gerhard's proposed patch also parses out the correct ip_address(). As only one IP is present, the change is irrelevant though.

Gerhard: are you sure this fixed a problem for you? How come your HTTP_X_FORWARDED_FOR header contained multiple IP addresses? Is that a bug or side-effect of http:bl?

Gerhard Killesreiter’s picture

Status: Needs review » Reviewed & tested by the community

The header contains more than one IP if the user also sits behind a proxy, ie:

user <--> user proxy <--> squid <-> drupal1

The problem was indeed discovered by use of http:bl, but not caused by it.

The wikipedia entry is quite clear in that the rightmost IP is the one we want.

I also discussed this with Eric Searcy...

Gábor Hojtsy’s picture

Check i2c_realip(), the implementation the PHP.net website uses: http://www.php.net/source.php?url=/include/ip-to-country.inc It might give you some impression on what works best there.

Gerhard Killesreiter’s picture

My patch is about fixing a very pparet bug in the current implementtion. It is not about improving it (which probably could be done).

Why is that patch important?

Quite easy: Without it you can bypass all checks that rely on ip_address() if the website is behind a reverse proxy. You only need to put some random IP into that header and you can spam such a Drupal site silly because the flood threshold won't kick in (for example).

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Sure, thanks for the patch. Committed.

Anonymous’s picture

Status: Fixed » Closed (fixed)