search

Solutions-Nitriques / anti_brute_force

Secure your Symphony CMS login page against brute force attacks
http://symphonyextensions.com/extensions/anti_brute_force/
Other
10 stars 12 forks source link

ABF::isIPValid does not work with IPv6 addresses #40

Closed michael-e closed 7 years ago

michael-e commented 7 years ago

ABF::isIPValid will return false for IPv6 addresses. Therefore, it is not possible to add an IPv6 address to the whitelist. (There may be other negative effects as well.)

nitriques commented 7 years ago

IPv6 is currently not supported at all. Everything is coded with IPv4 in mind. (at the time, IPv6 was not in production).

I do not have any Apache that listens on IPv6 (it is always the proxy that does it), so I might not be able to fix that soon. If you want to send a PR, I'll gladly review it.

BTW, do you think that is should support both version or ipv6 support would need to be enabled (which would disable ipv4 support) ?

michael-e commented 7 years ago

IPv6 is currently not supported at all. Everything is coded with IPv4 in mind. (at the time, IPv6 was not in production).

Really? In https://github.com/Solutions-Nitriques/anti_brute_force/issues/36 we have already fixed a database issue with IPv6 some time ago. :-)

I do not have any Apache that listens on IPv6 (it is always the proxy that does it), so I might not be able to fix that soon.

But the proxy will probably pass the IP to the Apache backend, e.g. using the X-Forwarded-For header — otherwise you would only see 127.0.0.1 in your Apache logs!

I am currently experimenting with IPv6 in my "big system", and actually ABF seems to work fine, apart from the mentioned glitch.

BTW, do you think that is should support both version or ipv6 support would need to be enabled (which would disable ipv4 support) ?

Support both! It is clear that the positive effect of ABF is weaker with IPv6, because an attacker might have a lot of IPv6 addresses to choose from. But this is true for any anti-brute-force mechanism — it can never be an excuse for bad passwords.

If you want to send a PR, I'll gladly review it.

Not sure when I would find the time for this.

Do you see any reason why we shouldn't just pimp the function to return true for IPv6 addresses as well? (I am asking this because I don't know what you had in mind with this validation.)

nitriques commented 7 years ago

Really? In #36 we have already fixed a database issue with IPv6 some time ago. :-)

I totally forgotten that ! Sorry!

otherwise you would only see 127.0.0.1 in your Apache logs!

Yeah that's what I get ;)

Do you see any reason why we shouldn't just pimp the function to return true for IPv6 addresses as well?

No I do not see any!

I am asking this because I don't know what you had in mind with this validation.

I wanted to make sure I was inserting ip addresses, not some other field, to protect myself from myself. If IPv6 is already supported, then everything should work with IPv6 ;)

If you do not find the time to do it right now, it is not a problem, I may do it when I get the chance ;)

michael-e commented 7 years ago

Yeah that's what I get ;)

Are you interested in how it would work to have IP addresses in the logs? I could explain, at least for an nginx/Apache setup.

I'll see if I find the time to create a PR.

michael-e commented 7 years ago

Verbose and complicated fix now available! :-)

nitriques commented 7 years ago

Are you interested in how it would work to have IP addresses in the logs? I could explain, at least for an nginx/Apache setup.

My apache logs are deleted every 24 hours so I rather do something else with our both our time ;) But thanks for the offer tho, I appreciate it !

nitriques commented 7 years ago

Released and signed as 2.0.4 ;) Thanks again.