Erroneous Invalid IP Exception Due to IP Parsing Bug

ScottHamper commented 5 years ago

Hey All,

In commit 900db3a, line 133 in Controller.php attempts to determine if the visitor's IP address is a v4 or v6 address. It is assumed that if the address is not IPv4, then it must be IPv6. However, there are situations where the value returned from Uri::ip is neither a valid IPv4 nor IPv6 address. For example, the HTTP_X_FORWARDED_FOR environment variable can contain a comma delimited list of IP addresses. When this happens, Grav thinks it's an IPv6 address and calls Utils::getSubnet on it, which then promptly throws an exception on line 1494.

This situation is currently happening to me on a base Serverpilot installation, and I've had to revert back to 3.0.1 of the login plugin.

According to the blog post above, it may be adequate to take the last IP address listed in the comma delimited list of IP addresses stored in the HTTP_X_FORWARDED_FOR environment variable. I'm not familiar enough with the different PHP environment variables to know if that's accurate, or if there will be similar issues with the other variables that Grav looks at. Regardless, at a minimum, the Uri::ip function should probably be updated to use the filter_var function before deciding to return an environment variable's value. Something like:

if (filter_var(getenv('HTTP_CLIENT_IP'), FILTER_VALIDATE_IP)) {
    return getenv('HTTP_CLIENT_IP');
} // And so on...

rhukster commented 5 years ago

I had already removed the exception and just returned the IP received if it didn't pass the filter: https://github.com/getgrav/grav/commit/be558ccac9223dcc24ec4c04612e5a479714d686

If you would like to improve with some smarter logic, a PR would be appreciated!

ScottHamper commented 5 years ago

Oh, word! My apologies for not looking at HEAD, or discovering #2507 (I promise I looked at issues before posting! Apparently not closely enough...)

Changing the throw to a return seems like a reasonable enough solution. The only other thought I had earlier today was to turn $isIPv4 into $isIPv6 and flip the ternary conditional around in Controller.php, since we only want to do something special when its confirmed that we have a v6 address. The end result ends up being the same!

I personally kind of liked that getSubnet threw an exception, as it seems more semantically correct (and it should be the responsibility of clients to confirm something is a valid address before attempting to call getSubnet on it). Semantically, it seems out of place for a function called "getSubnet" to return something that is not actually a subnet. Regardless, it's not something I care to push for - I am merely tossing my opinion out here.

Beyond that, there's still a bug present in Uri::ip in that it can return something that's not an IP address - I would argue that that's the real bug that needs fixing, and the current fix/the fix above are only fixing a side effect of it. While part of the API for Uri::ip seems to be that it can return "UNKNOWN" in certain cases, it's not necessarily clear from the API that it could/would return other non-IP values. In an ideal world, the function would never return a string that's not an IP. I don't really like suggesting it should return null, though, because null sucks, but if PHP has some kind of "Maybe" monad, then this would be a great place to use it.

Anyways, thanks for the quick response!

rhukster commented 5 years ago

Thanks for your thoughts Scott, i'm still traveling at the moment, but as soon as I get back i'll look at this in more detail.

rhukster commented 5 years ago

BTW, the initial issue https://github.com/getgrav/grav/issues/2507 was brought about via the UNKNOWN IP address being sent. At that point, we just can't do much with it. It's generally just used as a string, so not a huge deal.

Vivalldi commented 5 years ago

@rhukster - Regarding getSubnet's handling of invalid IPs I don't fully understand why it was changed to just return the ip on failure. I agree with @ScottHamper that it's the responsibility of the clients to confirm something is valid before calling getSubnet. The login plugin is currently the only plugin to make use of this util but it could easily be used elsewhere and returning the input without error would be a bit misleading.

@ScottHamper - if you want help crafting a PR to address this just ping me in discord. https://getgrav.org/discord - nickname vivalldi#9468

rhukster commented 5 years ago

I just have not had a chance to go back and look at this yet. Heading back to USA Today.

getgrav / grav-plugin-login

Erroneous Invalid IP Exception Due to IP Parsing Bug #211