? ? Pending

User tests: Successful: Unsuccessful:

avatar zero-24
zero-24
17 Feb 2021

Pull Request for an issue reported to the JSST by DangKhai from Viettel Cyber Security

Summary of Changes

Make sure we dont allow IP overrides by the X-Forwarded-For or Client-Ip HTTP headers for the com_content vote feature.

Testing Instructions

Setup voting for an article
make sure voting works
apply the patch
make sure voting still works

Actual result BEFORE applying this Pull Request

It was possible to override the ip using the X-Forwarded-For or Client-Ip HTTP headers

Expected result AFTER applying this Pull Request

that behavior is disabled now

Documentation Changes Required

None

avatar zero-24 zero-24 - open - 17 Feb 2021
avatar zero-24 zero-24 - change - 17 Feb 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 17 Feb 2021
Category Front End com_content
avatar zero-24 zero-24 - change - 17 Feb 2021
Labels Added: ?
avatar richard67 richard67 - test_item - 17 Feb 2021 - Tested successfully
avatar richard67
richard67 - comment - 17 Feb 2021

I have tested this item successfully on 8b0103e


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32452.

avatar zero-24 zero-24 - change - 18 Feb 2021
The description was changed
avatar zero-24 zero-24 - edited - 18 Feb 2021
avatar sandewt
sandewt - comment - 19 Feb 2021

For the test I changed the following code in ...\libraries\vendor\joomla\utilities\src\IpHelper.php

public static function setAllowIpOverrides($newState)
{
    // self::$allowIpOverrides = $newState ? true : false; // original code
    self::$allowIpOverrides = $newState ? false : false; // changed code
}

The test remains successful.
In other words, does the concerned method do what it is supposed to do?

// Make sure we don't allow IP overrides
IpHelper::setAllowIpOverrides(false);


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32452.
avatar PhilETaylor
PhilETaylor - comment - 19 Feb 2021

Ok I'll bite. This is plain wrong.

X-Forwarded-For and Client-Ip should be allowed - but ONLY when behind a load balancer, SSL terminator, or proxy servers.

Also the testing instructions are inadequate as they do not test Joomla used in circumstances when these headers would normally occur, and are needed - I.e behind load balancer, SSL terminator, or proxy servers !

If you merge this, then Joomla sites behind a load balancer, SSL terminator, or proxy server will then record the wrong IP address when votes are cast. This will lead to the voting being unusable behind a load balancer, SSL terminator, or proxy server.

Do people use Joomla because a load balancer, SSL terminator, or proxy server ? HELL YES, YOU BET THEY DO!

By merging this you will break all voting when the site is behind a load balancer, SSL terminator, or proxy server , as the site will not get the correct users IP address, but the internal IP address of the load balancer, SSL terminator, or proxy server


If IpHelper::getIp has been called before (eg in a 3pd plugin, or even within Joomla itself (not checked)) this change (the calling of IpHelper::setAllowIpOverrides(false);), then the IP address is already stored as a static in the class and is then not returned "correctly" as the IP has already been detected and stored before the disabling of the override.

So even that alone means this PR is not resolving the issue reported.


The correct fix could include a new option in Joomla Global Configuration for specifically declaring that Joomla is behind a load balancer, SSL terminator, or proxy server That new setting could be used to toggle this issue, as well as the SSL Check when enabling SSL (which can only be done if Joomla detects its on a ssl, which might not be true, as it could be behind a load balancer, SSL terminator, or proxy server that is terminating the SSL, but Joomla still needs to generate https:// prefixed urls.

avatar brianteeman
brianteeman - comment - 19 Feb 2021

and that will include joomla.org

avatar zero-24
zero-24 - comment - 19 Feb 2021

Understood. I'm happy to hear proposals to fix this issue. We might have to do some kind of configuration where we get the info from that we run behind that proxy setup?

avatar PhilETaylor
PhilETaylor - comment - 19 Feb 2021

Understood. I'm happy to hear proposals to fix this issue. We might have to do some kind of configuration where we get the info from that we run behind that proxy setup?

I gave you a proposal which fixes the reported issue and improves Joomla's use in the situation behind a load balancer. This has been a long running bodge...

The correct fix could include a new option in Joomla Global Configuration for specifically declaring that Joomla is behind a load balancer, SSL terminator, or proxy server That new setting could be used to toggle this issue, as well as the SSL Check when enabling SSL (which can only be done if Joomla detects its on a ssl, which might not be true, as it could be behind a load balancer, SSL terminator, or proxy server that is terminating the SSL, but Joomla still needs to generate https:// prefixed urls.

avatar sandewt
sandewt - comment - 19 Feb 2021

Understood. I'm happy to hear proposals to fix this issue.

Maybe you can do something with this, see:

if (!filter_var($ip, FILTER_VALIDATE_IP))

avatar PhilETaylor
PhilETaylor - comment - 25 Mar 2021

@zero-24 Please can you share the actual Proof Of Concept the JSST received from DangKhai/Viettel Cyber Security so that we can understand what "vulnerability" this is trying to resolve, else any work on this issue by anyone else other than those privileged - is just guess work. Happy to receive to phil@phil-taylor.com by GPG.

This PR as proposed cannot be merged for the reasons stated.

I have time this weekend to do this (Joomla behind reverse proxy) correctly once and for all (Including the ability to set SEF/SSL urls behind proxy when Joomla is on a server with no SSL installed, but SSL Terminated on the load balancer)

avatar zero-24
zero-24 - comment - 25 Mar 2021

Its about bypassing the ip restrictions on voting by bypassing it using the mentiond headers.

avatar PhilETaylor
PhilETaylor - comment - 25 Mar 2021

ok so this is simple then.

What you have also then failed to undertake is a code review of other uses of the IP, because sending headers like reported would also allow you to fake other IP based logging/actions like the Action Logs... doh...

I'll add a new global configuration setting to enable "Features required when behind a loadbalancer/reverse proxy" which will then handle the IP correctly, only allowing override when behind a proxy, and allowing SSL to be enabled even if ssl not available on the server Joomla is on. I will also take into account reCaptcha IP handling, Action Log IP logging and Article Voting..

The default value for $allowIpOverrides in the IpHelper should be FALSE and only enabled by this new feature above.

avatar PhilETaylor
PhilETaylor - comment - 25 Mar 2021

Here is my proposed replacement PR #32866

avatar zero-24
zero-24 - comment - 25 Mar 2021

see: #32866

avatar zero-24 zero-24 - change - 25 Mar 2021
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2021-03-25 21:31:57
Closed_By zero-24
Labels Added: ?
avatar zero-24 zero-24 - close - 25 Mar 2021

Add a Comment

Login with GitHub to post a comment