Fixed BannedUsers spam check matches all users if Ip::convertIpStringToBinary returns false

[Steffen]

Affected version

2.2.9

In the method XF\Spam\Checker\BannedUsers::check, if calling Ip::convertIpStringToBinary() returns false (*) then the finder call ->where('ip', $ip) builds the condition `xf_ip`.`ip` = 0 which MySQL evaluates to true for nearly all strings (see e.g. https://stackoverflow.com/questions/22080382/mysql-why-comparing-a-string-to-0-gives-true). As a result, the query matches all banned users (not only those who have a matching IP address).

(*) What XenForo passes to XF\Spam\Checker\BannedUsers::check is the result of XF\Http\Request::getIp with $allowProxied = true, which makes me think that the client may have passed and invalid (or maybe even intentionally spoofed?) value in the "X-Forwarded-For" header. Maybe XenForo should not trust the "X-Forwarded-For" header as it can be easily spoofed to bypass the check (or make it fail in the opposite way like explained above)?

 

[XF Bug Bot]

Thank you for reporting this issue, it has now been resolved. We are aiming to include any changes that have been made in a future XF release (2.2.10).

Change log:

Ensure spam checks handle cases where IP binary conversion fails, and do not trust proxied IP headers

There may be a delay before changes are rolled out to the XenForo Community.