Prestashop: Security: geolocationManagement() must alway use real remote IP

[mvorisek]

geolocationManagement() and other IP based restrictions must alway use real remote IP.

If "X-Forwarded-For" header is desired to be supported, it has to have own config option and must be turned by default off.

Current code for getting the remote IP:
https://github.com/PrestaShop/PrestaShop/blob/develop/classes/Tools.php#L377

From @finlanderid

The bug that I want to report is IF evaluation in the following code. If you look at it, you will see that it has the possibility to fail the IF evaluation, when it should not fail. The goal is to get the remote_addr (the true visitor IP) when a CDN proxy is making website request.

Problem summary: if remote_addr is not set, then the right-hand half of the IF will fail (requiring && operator truth), if remote_addr is from a CDN such as Cloudflare that is using a proxy IP that does not start with 127 or 172 or 192 or 10.

For example, the REMOTE_ADDR for a Cloudflare request is the Cloudflare IP address, but it might not start with 127 or 172 or 192 or 10, so the right-hand half of IF evaluation will fail, then the whole IF will fail, when it should not fail. For example, let's say that Cloudflare fetches a page from a website. The Cloudflare REMOTE_ADDR is 188.114.96.100. The true visitor IP is 67.60.1.2. If the function evaluates with this incorrect Prestashop code, then getRemoteAddr() will return 188.114.96.100 instead of 67.60.1.2. In this case, the function should return 67.60.1.2 (the visitor's true IP).

The reason for erroneous failure of the function:

HTTP_X_FORWARDED_FOR = 67.60.1.2
REMOTE_ADDR = 188.114.96.100

https://www.cloudflare.com/ips/

It is very possible (even likely) that remote_addr WILL be set by the proxy, but will not be set to an IP that starts with 127 or 172 or 192 or 10. In that event, the whole IF statement will fail, when it should not fail.

===

In Tools.php (line 357 on PS 1.7)

public static function getRemoteAddr()
{
    . . . 

    if (isset($_SERVER['HTTP_X_FORWARDED_FOR']) && $_SERVER['HTTP_X_FORWARDED_FOR'] && (!isset($_SERVER['REMOTE_ADDR'])
        || preg_match('/^127\..*/i', trim($_SERVER['REMOTE_ADDR'])) || preg_match('/^172\.16.*/i', trim($_SERVER['REMOTE_ADDR']))
        || preg_match('/^192\.168\.*/i', trim($_SERVER['REMOTE_ADDR'])) || preg_match('/^10\..*/i', trim($_SERVER['REMOTE_ADDR'])))) {
        if (strpos($_SERVER['HTTP_X_FORWARDED_FOR'], ',')) {
            $ips = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']);

            return $ips[0];
        } else {
            return $_SERVER['HTTP_X_FORWARDED_FOR'];
        }
    } else {
        return $_SERVER['REMOTE_ADDR'];
    }
}

[khouloudbelguith]

Hi @mvorisek,

Thanks for your report.
Could you please add steps to reproduce the issue in a new comment.

Thanks!

[mvorisek]

@khouloudbelguith see the link in the code above, the header is read/allowed by default.

So to replicate, send a HTTP request to PS with the X-Forwarded-For headers and notice, that geolocation and other things (like logging) uses this IP which can be very easily spoofed.

[khouloudbelguith]

Is it the same issue with this ticket: https://github.com/PrestaShop/PrestaShop/issues/14020?

Thanks!

[PierreRambaud]

@mvorisek You're right, this headers shouldn't be used when it's not under a trusted proxy, like Symfony does.

[mvorisek]

The desc. of #14020 is horrible, so maybe :) If PS is behind Cloudflare or other CDN, that's where the X-Forwarded-For headers makes sense. but otherwise it can not be trusted. Maybe there can be a list of most common CDN like the PS_GEOLOCATION_WHITELIST list or this list can be maybe used itself for it.

[khouloudbelguith]

@mvorisek, thanks!
@PrestaShop/prestashop-core-developers what do you think?

Thanks!

[finlanderid]

@mvorisek

Up yours. My description in #14020 is comprehensive and precise. Your lack of English language usage/comprehension does not make my description "horrible."

[mvorisek]

@finlanderid Do not take it personally. I am also non-native English speaker. I read half the description and I was reading something like "requiring && operator truth" etc. which is code description which can be much easily read when linked (at least for programmers) and the issue itself is described shortly in 1, 2 sentences.

[PierreRambaud]

@finlanderid Do not take it personally. I am also non-native English speaker. I read half the description and I was reading something like "requiring && operator truth" etc. which is code description which can be much easily read when linked (at least for programmers) and the issue itself is described shortly in 1, 2 sentences.

Since your description link the source code of the develop branch, I can't compare if everything is ok right now :sweat_smile:

I update the description of this PR with the @finlanderid one

[mvorisek]

Since your description link the source code of the develop branch, I can't compare if everything is ok right now 😅

@PierreRambaud updated link - https://github.com/PrestaShop/PrestaShop/blob/ec37517d944d6679aca9ae61e2a83ef24f6a427f/classes/Tools.php#L377 (replaced develop with the latest commit hash at the time writing the description)

the issue is still present and remote IP can be easily spoofed with X-Forwarded-For header