Do not check the IP address with session protection disabled

[ArthurHoaro]

This allows the user to stay logged in if his IP changes.

Fixes #1106

Note: this setting labelled « Disabled protection » can obviously become a security issue.

[virtualtam]

As the PR addresses (pun intended :) ) IPv6 privacy extensions, I'd suggest we:

• start reworking IP-related operations to distinguish IPv4 and IPV6 addresses
• introduce IPv4- and IPv6-specific configuration options to avoid catch-all cases that will be hard to document and debug

Resources:

• https://secure.php.net/manual/en/filter.filters.flags.php
• https://tools.ietf.org/html/rfc4941
• https://medium.com/@xchewtoyx/token-ipv6-post-8ad39d645636

Related PHP libs:

• https://github.com/darsyn/ip
• https://github.com/akalongman/php-ip-tools
• https://github.com/rlanvin/php-ip

[ArthurHoaro]

I don't understand your point, from my understanding there is no difference between how we treat IPv4 and IPv6 addresses. We use them to:

1. See if it has changed
2. Generate a hash

[virtualtam]

The point is that IPv6 behaves differently than IPv4, as local client address renewal is part of the protocol (vs. static local IPv4 addresses). I'm not sure how web frameworks handle this so it might be worth looking under the hood (and maybe switch to a well-maintained 3rd-party component to handle authentication?)

[ArthurHoaro]

Oh I didn't knew that. I'll look around for implementations, but I don't think we can find a third party lib which fits our needs and doesn't require a database.

[wcypierre]

any update on this? as I am facing the same issue as well

[ArthurHoaro]

So, I looked at Symfony implementation, and read a few SO thread.

Symfony includes two implementations, and none of them includes an IP check:

• TokenBasedRememberMeServices, a simple one, which basically do the same as we do.
• PersistentTokenBasedRememberMeServices which stores a token and regenerates it at every auto login.

From my understanding, tying the IP to the authentication cookie is not such a good idea as it can change for valid reasons. And if the address changes with IPv6 protocol, then it can't work at all.

In any case, I'm going to merge this, as it does not lower the security for users who have the option enabled, and as it prevents valid users to use the feature properly.

A few threads:

• https://security.stackexchange.com/questions/29902/are-there-modules-to-harden-symfony-2-sessions
• https://security.stackexchange.com/questions/131584/hashed-username-and-password-in-remember-me-token
• https://stackoverflow.com/questions/8419332/proper-session-hijacking-prevention-in-php