Moodle

Some of our users are even listed as 127.0.0.1.

Hi Matthew,

as far as I know that depends heavily of the configuration of your net. Are you running any inverse proxy? If there are intermediates from the client and the server, you must be 100% sure that those intermediates are passing the proper headers with the client IP properly populated. Those header are, in order of priority:

HTTP_CLIENT_IP
HTTP_X_FORWARDED_FOR
REMOTE_ADDR

And the first one found is the one inserted to logs.

Ciao

Assigning to Mathieu to follow reported comments...TIA!

Mat, I've added some suggestions about the way we process those headers at MDL-14236 .

The proposed changes could IMO improve a bit the detection of original IPs. Ciao

Here is an example of an IP reported by Moodle in the activity logs that do not match with Apache.

Moodle reports the user's IP address is 172.16.18.29

this address does not resolve if you do a DNS lookup

From our Apache log, his actual IP address is 209.43.2.10 which, by DNS lookup, I can confirm is his actual IP address since it resolves to his school.

I hope this helps!

172.16.* are addresses reserved for local use (addresses that are not routed on the internet, and thus don't resolve outside of the local network). Is this address (172.168.18.29) the address of the gateway/router in your network, or the address of the server itself? (this might help debugging...)

No, the 172.16.* IP does not correspond to any IP I know of. Our entire IP space is within 137.112.*

We've also seen, on occasion, a user's activity report has 127.0.0.1, which is localhost, and the user definitely did not have a local address.

Apache seems to always log the correct user IP address.

Oh, I also noticed, the affected version for this bug is 1.8.4. We are actually running the latest stable of 1.9.x.

Thanks again for the help!

Are you using a proxy, like squid, on your network?

No, no proxies.

Matthew, Ryan, can you give me more information about the systems you are using? LAMP? WAMP? IIS? Is this all IPV4, or is anything using IPV6?

Mat,

We are using WAMP....all of the latest versions of the components. Our Apache httpd.conf file is pretty standard...we use the combined logfile format. I hope this helps!

I really think this is some sort of HTTP_CLIENT_IP being populated somehow. As that variable is the fist one being considered by Moodle in getremoteaddr()....

And that has been the order since http://cvs.moodle.org/moodle/lib/moodlelib.php?r1=1.348&r2=1.349

Do we really need that HTTP_CLIENT_IP there? Ryan, can you check what happens if you disable (comment) that check in your getremoteaddr() ? Do you get, then, the correct 209.43.2.10 ?

As commented in MDL-14236 I would propose to chain the tests, skipping private addresses under HTTP_CLIENT_IP and HTTP_X_FORWARDED_FOR (each element).

And also, would consider seriously dropping HTTP_CLIENT_IP unless anybody knows some proxy using it (I don't).

Ciao

Ok, we have got the correct IP displaying now....but to get it to work, I had to comment out the $_SERVER['HTTP_CLIENT_IP'] and the $_SERVER['HTTP_X_FORWARDED_FOR'] checks.

$_SERVER['REMOTE_ADDR'] gives the correct IP address. I really don't see why the other checks exist. Everywhere I check online for gathering IP addresses says to use the REMOTE_ADDR variable.

I would do just as Eloy suggests : drop HTTP_CLIENT_IP and drop HTTP_X_FORWARDED_FOR as well. I can't find any trace of the first one in the CGI specifications (is it IIS-specific?) and the second one is just unreliable, can be spoofed, etc. If you are using a proxy, you can have logs for that proxy directly, which are going to be more precise anyway. I understand the chaining suggestion of Eloy, but I'm not sure how it would solve the spoofing problem...

REMOTE_ADDR is the only variable that I can see for this in the CGI specifications at http://hoohoo.ncsa.uiuc.edu/cgi/env.html

With only REMOTE_ADDR, as suggested, our site records correct IP addresses! For the previous example above, we now see 209.43.2.10 for the user that was reporting 172.16.18.29.

Hopefully we can get this fix in the official Moodle release soon!

Hi Ryan,

while this is working perfectly to you, there are a lot of institutions running their web servers below reversed proxies. So, those institutions cannot live without HTTP_X_FORWARDED_FOR at all. And, for them, the information in that header is 99% secure (because it's the original REMOTE_ADDR they received from outsiders).

So, what if we make this:

1) Create one new setting, say, getremoteaddrconf, with this values:
0 - current approach, use HTTP_CLIENT_IP, HTTP_X_FORWARDED_FOR and REMOTE_ADDR
1 - use HTTP_X_FORWARDED_FOR and REMOTE_ADDR
2 - use HTTP_CLIENT and REMOTE_ADDR
3 - use REMOTE_ADDR

2) We hack the getremoteaddr() function to:
a) - respect the settings above.
b) - chain the tests, jumping to the next if a private address is detected.

That way, I think the getremoteaddr() function will be less generic, as it seems that different server architectures can need different combinations and more people will be happy by adjusting that setting to their own combination.

Eloy,

That sounds like the perfect fix. It will make it accurate for all of the configurations out there.

Here's a tentative patch. Can you have a look at it, Eloy? The wording of the admin setting might need to be improved a bit...

Patch is looking really good. I think the setting area should be changed to a checkbox format. Let the administrator decide which checks to make instead of giving them a list of four choices.

[_] - HTTP_CLIENT_IP (Descriptions of when it is good to have this checked)
[_] - HTTP_X_FORWARDED_FOR (Descriptions of when it is good to have this checked)
[_] - HTTP_CLIENT (Descriptions of when it is good to have this checked)
[_] - REMOTE_ADDR (Descriptions of when it is good to have this checked)

Wow, just get this because of your update today!

Looks very good for me. +1 for 19_STABLE and HEAD!

Thanks!

Thanks Eloy - committed to head.

This needs to be reopened and fixed ASAP.

/////////////////////////////////////////////////////
moodlelib.php

line 7491

Change

switch ($CFG->getremoteaddr) {

to

switch ($CFG->getremoteaddrconf) {

///////////////////////////////////////////////////////////////

Also, I don't like the fact that this wasn't tested before being pushed into the STABLE branch. Problems like this one have been dormant for months and then a patch comes in and suddenly before it can get tested it is pushed into STABLE. There needs to be a little more care taken that future bugs aren't being introduced. That being said...once this mistake is fixed, the update fixes our problems. Thanks.

Thanks Matthew,

this was detected and fixed some hours ago (in HEAD). After testing it a bit more, I've backported it to 19_STABLE some minutes ago.

Be noted that we use to perform weekly (Wednesday) releases where this sort of problems should be 99% out, because we use to perform, also, one weekly code review before releasing:

http://docs.moodle.org/en/Development:Weekly_Code_Review

It's highly recommended to use only those weekly releases (tagged as MOODLE_XX_WEEKLY) in the CVS repository for production sites.

Thanks again for your quick-find, absolutely! Ciao

I have just committed an improvement to the code, to reduce duplication, fix a notice and use named constants. Since I have changed the code, I should not now be the person to QA this.

Tested with unset, 0, 1, 2 and 3. Works well under all cases. BTW, I hate bitwise operators. :-P

Thanks Tim!, closing now. Ciao