User tests: Successful: Unsuccessful:
Pull Request for an issue reported to the JSST by DangKhai from Viettel Cyber Security
Make sure we dont allow IP overrides by the X-Forwarded-For or Client-Ip HTTP headers for the com_content vote feature.
Setup voting for an article
make sure voting works
apply the patch
make sure voting still works
It was possible to override the ip using the X-Forwarded-For or Client-Ip HTTP headers
that behavior is disabled now
None
Status | New | ⇒ | Pending |
Category | ⇒ | Front End com_content |
Labels |
Added:
?
|
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);
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.
and that will include joomla.org
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?
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.
Understood. I'm happy to hear proposals to fix this issue.
Maybe you can do something with this, see:
@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)
Its about bypassing the ip restrictions on voting by bypassing it using the mentiond headers.
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.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-03-25 21:31:57 |
Closed_By | ⇒ | zero-24 | |
Labels |
Added:
?
|
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.