Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.0] Removing MCrypt class #38713

Open
wants to merge 7 commits into
base: 6.0-dev
Choose a base branch
from
Open

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Sep 7, 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.

@wilsonge
Copy link
Contributor

wilsonge commented Sep 7, 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.

@HLeithner
Copy link
Member

@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
Copy link
Contributor

wilsonge commented Oct 10, 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

@Hackwar
Copy link
Member Author

Hackwar commented Oct 11, 2022

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

@HLeithner
Copy link
Member

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

@Hackwar Hackwar added the Feature label Apr 6, 2023
@HLeithner HLeithner added the b/c break This item changes the behavior in an incompatible why. HEADS UP label Apr 7, 2023
@Hackwar Hackwar closed this Apr 18, 2023
@Hackwar Hackwar reopened this Apr 18, 2023
@HLeithner
Copy link
Member

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 HLeithner changed the base branch from 5.0-dev to 5.1-dev September 30, 2023 22:51
@HLeithner
Copy link
Member

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

@Hackwar Hackwar changed the base branch from 5.1-dev to 6.0-dev November 13, 2023 20:56
….0-mcrypt

# Conflicts:
#	libraries/src/Encrypt/AES/Mcrypt.php
#	libraries/src/Encrypt/Aes.php
@Hackwar
Copy link
Member Author

Hackwar commented Nov 13, 2023

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

@HLeithner
Copy link
Member

do we have a removal documentation pr?

@wilsonge
Copy link
Contributor

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

@Hackwar
Copy link
Member Author

Hackwar commented Feb 15, 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.

@Hackwar Hackwar changed the title [5.0] Removing MCrypt class [6.0] Removing MCrypt class Feb 15, 2024
@brianteeman
Copy link
Contributor

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
Copy link
Member Author

Hackwar commented Feb 15, 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.

@HLeithner
Copy link
Member

ok for me, mcrypt should really not used anymore

@HLeithner
Copy link
Member

@laoneo laoneo removed the PR-5.0-dev label Feb 15, 2024
@wilsonge
Copy link
Contributor

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
Copy link
Member Author

Hackwar commented Feb 16, 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?

@laoneo
Copy link
Member

laoneo commented Feb 20, 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.

@wilsonge
Copy link
Contributor

@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
Copy link
Member

laoneo commented Feb 23, 2024

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

@wilsonge
Copy link
Contributor

wilsonge commented Mar 1, 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.

@laoneo
Copy link
Member

laoneo commented Mar 22, 2024

@Hackwar can you implement the suggestion from @wilsonge ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
b/c break This item changes the behavior in an incompatible why. HEADS UP Feature PR-6.0-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants