[#34365] - IPHelper gets the wrong IP address from proxies
- Closed
- 3 Jun 2021
- Medium
- Build: staging
- # 34365
Steps to reproduce the issue
- Put a site behind a proxy, e.g. CloudFlare, Sucuri or Varnish
- Get an IP address using
Joomla\Utilities\IPHelper::getIp()with allow IP overrides enabled (default)
Expected result
You get the connecting client's address
Actual result
You get the IP address of the proxy
System information (as much as possible)
Irrelevant.
Additional comments
The Joomla Framework has copied code from a very old version of FOF which had a bug in detectAndCleanIP.
This line https://github.com/joomla-framework/utilities/blob/master/src/IpHelper.php#L452 should actually read array_shift, not array_pop.
The reason is how the de-facto X-Forwarded-For standard works, see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For Each proxy in the chain appends its IP to the header. Using array_pop we retrieve the last IP address which is the IP address of the last proxy in the chain. Since we are trying to find the visitor's real IP address we should instead be doing an array_shift to get the first IP in the chain.
How to fix this issue
I am tagging @wilsonge as the only active, common maintainer between the CMS and Framework repositories.
First, please restore the copyright header on the file. Removing the copyright from the code you copied verbatim, without any significant changes, is in violation of the GNU General Public License v2, article 1. Kindly remember that the Joomla Contributor Agreement (JCA) I signed years ago DOES NOT cover FOF's code which was included in Joomla as a third party library on Joomla's initiative. It was not submitted as a core contribution. I am treating this as an honest mistake, not deliberate copyright infringement. Please address this issue; with full reservation of my and Akeeba Ltd's rights under copyright law and any relevant international treaties for the covered work.
Then, change line 452 from array_pop to array_shift.
Finally, update the Joomla CMS repo, 4.0-dev branch, to use the new version of the library.
Joomla 3
This bug also affects Joomla 3 but NOT any of its core features. The only instance of that code is in FOF 2.x library included with Joomla, only to be used by third parties. This version of FOF is End of Life since June 2016 and has not been updated since April 2015. It's dead as a dodo. Since it's already on its way out and nobody complained about this in the past I assume nobody is using it in third party code. Therefore you can safely ignore it on Joomla 3.
You cannot ignore it on Joomla 4 where this code is used in the application initialisation, action logs model, articles model, the formatted text logger and the reCAPTCHA plugins.
joomla-cms-bot
- | Labels |
Added:
?
|
||
@brianteeman its the same "area" but no, its not the same thing. My PR was to turn OFF the overrides by default which fixed the security issue reported. The actual determining of the IP address was untouched by #32866
There is this in that file :
* This class is adapted from the `FOFUtilsIp` class distributed with the Joomla! CMS as part of the FOF library by Akeeba Ltd.
* The original class is copyright of Nicholas K. Dionysopoulos / Akeeba Ltd.
If thats not good enough, please provide the original to be added please, I dont know where to get that from :)
@PhilETaylor ok - so close but no cigar
Sorry, I had missed that comment there. Thanks, phpStorm – that's not a comment I wanted you to collapse in me. Sigh. It's good enough.
Regarding the actual code that determines the IP address, Phil wrote the code to enable it in the application's framework.php files but didn't touch the code to determine it.
Joomla was still using the very first version of that code from 2015, before it was put into actual use. At that point in time I'd abandoned FOF 2 and was working on FOF 3. I had spotted the issue and fixed it in FOF 3 but Joomla never backported that code since it wasn't using it at that time. Another four years passed, Joomla forked the code and it was based off the wrong implementation. Luckily only that small thing is broken. Everything else works and it had been field tested before in Admin Tools in the wild.
So, just one line to change.
Ive PR'ed upstream here joomla-framework/utilities#35 - once thats merged and back ported to Joomla 4 via composer this issue is resolved :)
I've made a release for staging. Need to merge that branch upto 2.x tomorrow and do a release there for J4 (and do composer updates of both into J branches).
@nikosdion I wasn't involved in the original copyright notices being figured out and not a legal person - so just copied it out of the FOF stuff we use in staging. Would be appreciated if you could confirm what I did was OK?
@wilsonge Yeah, the copyright notice is fine, phpStorm had collapsed it and I didn't notice that. Between working on a different computer than usual, Dark Mode and not having slept in two days I completely missed that collapsed comment. we're all good. Sorry to bother you about this :/
Don't stress :)
I've changed it to be the same as how FOF v2 is in core now so it's more prominent. Tagged beta2 for v2 and 1.6.2 for staging. This just requires updating the composer packages in core now.
| Status | New | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-06-03 14:07:07 |
| Closed_By | ⇒ | richard67 |
Is this the same thing @PhilETaylor wrote code for #32866
(I have no actual clue if it is but it sounded similar)