[6.0] Removing MCrypt class

[Hackwar]

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.

[wilsonge]

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.

[HLeithner]

@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.

[wilsonge]

@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

[Hackwar]

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

[HLeithner]

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

[HLeithner]

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

[HLeithner]

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

[HLeithner]

do we have a removal documentation pr?

[wilsonge]

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

[Hackwar]

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.

[brianteeman]

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

[Hackwar]

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.

[HLeithner]

ok for me, mcrypt should really not used anymore

[HLeithner]

@Hackwar can you please create a PR for Manual at https://github.com/joomla/Manual/blob/main/migrations/54-60/removed-backward-incompatibility.md

[wilsonge]

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.

[Hackwar]

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?

[laoneo]

@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.

[wilsonge]

@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 :)

[laoneo]

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

[wilsonge]

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.

[laoneo]

@Hackwar can you implement the suggestion from @wilsonge ?

[Bodge-IT]

Looks like we're close on this @Hackwar, can you please review @laoneo, @wilsonge requests?

[HLeithner]

@wilsonge I would like to merge this code, when the code hannes removes here is everything which interact with the old mencypt encryption then it's already not possible to login with an old hash. Or where do I miss the code that checks if the password hash or 2fa hash is using the old mcrypt algo and uses then the mcrypt lib?

please can up with a proposal what we can and should do before alpha2 else we would merge it and maybe ad a preupdate check which checked for users which haven't logged in for 2 years? or what every the best way is to detect this broken accounts.

[Hackwar]

So, I checked this again and if there are 2FA codes out there which use MCrypt, they don't work right now either, since there is no situation where the MCrypt class is used. This PR is now almost 3 years old and the situation hasn't really changed in all that time. It was broken 3 years ago, it is broken now. Lets please get this moving, merge this and not mess around with additional changes. I'm going to create a documentation PR.

[wilsonge]

To be clear without informing site admins who can and cannot log in I'm still against removing this. I understand the users already likely can't log in because of PHP requirements but that still doesn't justify silent removals in my opinion.

[Hackwar]

The thing is, that even if their PHP version would support this, our code already doesn't handle those passwords. We are basically never loading the MCrypt class. They've been logged out since at least Joomla 4.0, most likely even a lot longer than that. The only thing that we should do in regards to informing people would be to add it to our migration documentation.

[laoneo]

Agree here with Hannes. Especially as when 6.0 will be out, J4 is eol then. A note in the migration doc would be enough.