Last modified: 2013-04-22 16:14:45 UTC
For example: "xx.xxx.xx.xxx, xx.xxx.xxx.xx" or even an array, it simply cannot handle it an denies access to all users who connect with this.
Shouldn't take much to change wfGetIP() in ProxyTools.php to handle this. However, it'd be nice to know what's generating these kinds of REMOTE_ADDRs and why. In particular, if we get multiple IPs in REMOTE_ADDR, which one should we use? The first? The last? Something else?
You should use the LAST technically, as under apache the FIRST is the X_FORWARDED_BY. Or perhaps you should test against X_FORWARDED_FOR ? Because that ALSO returns two IPs... grrr
TESTCASE: http://dpaste.com/hold/294530/
Created attachment 7954 [details] A quick draft of a patch to fix this (not tested) Here's a quick patch that should at least stop it from crashing. I'm still not quite sure _how_ we should properly interpret such input, though.
Should you not canonicalize the parts then reformat it? @Iimari Karonen
I was assuming the loop below was doing that already, but now I see that I'd been reading it wrong. New patch coming soon.
What kind of setup is generating such REMOTE_ADDR? The tcp connection can only be established with one remote ip. That seems oddly moved into HTTP_X_REAL_IP header.
Created attachment 7955 [details] Patch v2, use last IP in REMOTE_ADDR and canonicalize it properly
It *shouldn't* be possible for REMOTE_ADDR to contain more than one IP, but it looks like it can happen if the server's using something like this apache module: http://stderr.net/apache/rpaf/ or this nginx module: http://wiki.nginx.org/HttpRealIpModule These will replace the value used to fill REMOTE_ADDR with the X-Forwarded-For value, and it's a good bet that they're not actually thinking to validate the value and format it correctly... (the apache module I found definitely doesn't). If there was another proxy in the chain, then the X-Forwarded-For header will have two or more IPs in it, and they can get copied right in. This isn't an ideal thing to do in your server config -- it's meant to fix 3rd-party apps looking only at REMOTE_ADDR, but is then just as likely to break them in the multi-IP case -- and it really should be fixed in that module or whatever similar system is triggering it. But, when we *do* find ourselves receiving such a bogus value, it's probably best to treat it like an XFF from a trusted proxy: use the most recent value in the stack, since it's the IP that the trusted proxy saw itself and can verify. Patch v2 looks like it should be fine.
(In reply to comment #9) > Patch v2 looks like it should be fine. So should Ilmari commit it?
Ilmari, I guess you should check for IPv6 compatibility and anything else that's changed in the last several months and commit to Git.
Moved patch into Gerrit (also adjusted for current version because wfGetIP has been deprecated and moved to WebRequest::getRawIP). https://gerrit.wikimedia.org/r/21135
Bug assigned to code submitter (Ilmari Karonen).
Thanks to Ilmari and to Tyler! https://gerrit.wikimedia.org/r/21135 is now merged so I'm closing this as fixed.
