Feature PR-5.0-dev b/c break PR-6.0-dev Pending

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
7 Sep 2022

Summary of Changes

I'm currently checking our codebase for validity against PHP 7.2 and there are a few things which have come up. One of them is that the mcrypt library has been removed in PHP 7.2.0 completely.
php.net/manual/en/function.mcrypt-get-iv-size.php
This library does not exist for any version of PHP that Joomla supports and thus this class has not been possible to be used with MCrypt instead of OpenSSL. Since that means no one has been using this, we can remove this now.

This is the updated version of #38527.

avatar Hackwar Hackwar - open - 7 Sep 2022
avatar Hackwar Hackwar - change - 7 Sep 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 7 Sep 2022
Category Unit Tests Repository Administration com_associations com_banners com_categories com_config com_contact com_content com_contenthistory com_fields com_finder com_installer com_joomlaupdate com_languages
avatar wilsonge
wilsonge - comment - 7 Sep 2022

It's worth noting that mcrypt isn't installed by default in PHP - but it wasn't deleted - just relegated to a pecl install https://pecl.php.net/package/mcrypt so it's still possible to use it in Joomla - it will just depend on your host setup

We will need to understand the impact this has on old 2FA users before removing this who might have had keys utilising mcrypt from a 3.x version.

avatar HLeithner
HLeithner - comment - 4 Oct 2022

@wilsonge @Hackwar I would like to run a motion to remove mcrypt in 5.0 and not in 6.0 is there any reason why we should keep it? it's unmaintained more or less since 2007.
It's hard to find out if Joomla it self uses mcrypt or what was the last version it uses it for encoding things. Also beside the removal done by hannes here, the libraries/php-encryption folder could be removed too because it holds only Crypto which is basically a wrapper around mcrypt and I can't find any usage in the CMS for it.

avatar HLeithner HLeithner - change - 4 Oct 2022
Labels Added: ? ?
avatar joomla-cms-bot joomla-cms-bot - change - 4 Oct 2022
Category Unit Tests Repository Administration com_associations com_banners com_categories com_config com_contact com_content com_contenthistory com_fields com_finder com_installer com_joomlaupdate com_languages Libraries
avatar wilsonge
wilsonge - comment - 10 Oct 2022

@wilsonge @Hackwar I would like to run a motion to remove mcrypt in 5.0 and not in 6.0 is there any reason why we should keep it? it's unmaintained more or less since 2007.

I'm unclear on whether we would have issues with old 2FA setups. If you can validate that old 2FA keys continue to work/are migrated correctly without it then I think we are all good. Else should be for v6 (or even later) given this isn't even a developer b/c break but an website user b/c break (in the sense you'd potentially loose ability to auth)

the libraries/php-encryption folder could be removed too because it holds only Crypto which is basically a wrapper around mcrypt and I can't find any usage in the CMS for it.

https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/Crypt/Cipher/CryptoCipher.php#L47 This entire cipher uses it - which we called the only secure implementation for a large stretch of j3.x development

avatar Hackwar
Hackwar - comment - 11 Oct 2022

Do you know in which versions we used MCrypt for 2FA?

avatar HLeithner
HLeithner - comment - 8 Mar 2023

@wilsonge @Hackwar do you found out which version of joomla is relevant? and could we test if old 2fa keys would be effected?

avatar Hackwar Hackwar - change - 18 Apr 2023
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2023-04-18 12:06:28
Closed_By Hackwar
Labels Added: ? PR-5.0-dev b/c break
Removed: ?
avatar Hackwar Hackwar - close - 18 Apr 2023
avatar Hackwar Hackwar - change - 18 Apr 2023
Status Closed New
Closed_Date 2023-04-18 12:06:28
Closed_By Hackwar
Labels Removed: ?
avatar Hackwar Hackwar - change - 18 Apr 2023
Status New Pending
avatar Hackwar Hackwar - reopen - 18 Apr 2023
avatar HLeithner
HLeithner - comment - 31 Aug 2023

What's gone happen here? can we find consent? beta1 comes soon and it's time to merge a removal or it will stay till 6.0

avatar HLeithner
HLeithner - comment - 30 Sep 2023

This pull request has been automatically rebased to 5.1-dev.

avatar Hackwar Hackwar - change - 13 Nov 2023
Labels Added: Feature
Removed: ?
avatar Hackwar Hackwar - change - 13 Nov 2023
Labels Added: PR-6.0-dev
avatar Hackwar
Hackwar - comment - 13 Nov 2023

Ok, I updated the PR and it would be ready to be merged into 6.0.

avatar HLeithner
HLeithner - comment - 13 Nov 2023

do we have a removal documentation pr?

avatar wilsonge
wilsonge - comment - 14 Nov 2023

Have we tested older 2fa keys. Unless we can document the workarounds we still shouldn’t be merging this

avatar Hackwar
Hackwar - comment - 15 Feb 2024

So wherever I look where we use the AES library, we are using only new AES($key, 256) and don't define an encryption format. We also didn't use something else in the past and the AES library was also never changed to change the encryption order. I think it is safe to say that we never used this. I would say this is ready to be merged.

avatar Hackwar Hackwar - change - 15 Feb 2024
Title
[5.0] Removing MCrypt class
[6.0] Removing MCrypt class
avatar Hackwar Hackwar - edited - 15 Feb 2024
avatar brianteeman
brianteeman - comment - 15 Feb 2024

I think it is safe to say that we never used this. I would say this is ready to be merged.

Just because core may not have used something doesnt mean that the library was not used in the ecosystem

avatar Hackwar
Hackwar - comment - 15 Feb 2024

Yes, which is why we want to remove it in the next major release, after it has been deprecated for at least a complete major release. If someone still needs this in 6.0, they have to copy the class to their own code.

avatar HLeithner
HLeithner - comment - 15 Feb 2024

ok for me, mcrypt should really not used anymore

avatar wilsonge
wilsonge - comment - 15 Feb 2024

https://github.com/joomla/joomla-cms/blob/3.2.7/libraries/fof/encrypt/aes.php

When AES was introduced with fof (which we used directly - and got permission to migrate into core in 4.0) and as you can see was hardcoded to use mcrypt back then

The PR called “2fa handeling for mcrypt and openssl” #12497 added it. Google really isn’t hard.

So yes we really did use it for 2fa between 3.2.0 and 3.6.4. So users who have logged in since 3.6.4 should be migrated those from before need testing and a recovery mechanism placed in core just like we need for the old password formats.

avatar Hackwar
Hackwar - comment - 16 Feb 2024

The thing is, that this is broken since at least 4.2.0, where any switch to mcrypt was removed as far as I can see. I would even say that it is broken for a lot longer. If there is old 2fa keys like that out there, they aren't working right now either. I don't know how a migration of the potential old data could look like, especially considering that we can't assume to have the mcrypt library available. Didn't we need to convert all old keys in 4.2.0 anyway?

avatar laoneo
laoneo - comment - 20 Feb 2024

@wilsonge when you want to support users which haven't been logged in with a release which was shipped 7 years ago, I doubt we shouldn't support that.

avatar wilsonge
wilsonge - comment - 22 Feb 2024

@wilsonge when you want to support users which haven't been logged in with a release which was shipped 7 years ago, I doubt we shouldn't support that.

I want to make sure they have a way to restore their accounts and not just making it seem like things have broken and they have to guess their way into their accounts. If that means we have to say the sites encryption means they need to reset up mfa sobeit.

FWIW i have lots of things in my 1password vault i've not logged into for much more than 7 years :)

avatar wilsonge
wilsonge - comment - 1 Mar 2024

Can we detect and remove that during the update if such a mfa code is used?

I'm not sure - but I think that's an acceptable solution. Force a reset of codes and show users a message like "to increase our site security we've upgraded our encryption library" or something that sounds good to them.

avatar laoneo
laoneo - comment - 22 Mar 2024

@Hackwar can you implement the suggestion from @wilsonge ?

Add a Comment

Login with GitHub to post a comment