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?

damon.underground wrote:

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

damon.underground wrote:

TESTCASE: http://dpaste.com/hold/294530/

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.

attachment bug_26585_sketch.diff ignored as obsolete

damon.underground wrote:

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.

Patch v2, use last IP in REMOTE_ADDR and canonicalize it properly

Attached:

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.

sumanah wrote:

(In reply to comment #9)

Patch v2 looks like it should be fine.

So should Ilmari commit it?

sumanah wrote:

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).

sumanah wrote:

Thanks to Ilmari and to Tyler! https://gerrit.wikimedia.org/r/21135 is now merged so I'm closing this as fixed.