joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.77k stars 3.65k forks source link

[4.0] Outdated crypto lib in Joomla\CMS\Crypt #29830

Closed SniperSister closed 4 years ago

SniperSister commented 4 years ago

Steps to reproduce the issue

As reported by @PhilETaylor the crypto classes in /libraries/src/Crypt are using the outdated version of the php-encryption library (see /libraries/php-encryption) which requires mcrypt to work. As mcrypt is removed from PHP in 7.2, the library needs to be refactored to use the up-to-date version of the library (already defined as a dependency in composer).

Expected result

Library Defuse\Crypto is used

Actual result

Old Libray version is used.

Additional comments

Thank you @PhilETaylor for your report! :)

mbabker commented 4 years ago

https://github.com/joomla/joomla-cms/pull/12134

There are B/C breaks between the non-Composer version inlined in the CMS and the Composer version used with the Framework. Either add yet another short notice deprecation in the CMS that has almost no migration path (if you're using the CMS' crypt API, which being frank, I doubt anyone is, then the only potential migration path is to switch to the sodium crypt otherwise you're not going to be able to decrypt something with the older version of the library and re-encrypt it with the new version of the library if you can't have both versions of the code running at the same time) or you accept that both versions of the library need to live in parallel for a sane migration.

PhilETaylor commented 4 years ago

But as this code relies heavily on mcrypt which has been removed from all php versions targeted by Joomla 4 - this code will be unuseable anyway :-)

Unless people add mcrypt back to PHP on their servers - gulp

I guess we leave it then? mark it as deprecated? and add another note to joomla 4 technical requirements to say that mcrypt is an optional requirement if you want to use this code?

mbabker commented 4 years ago

Unless people add mcrypt back to PHP on their servers - gulp

They moved it to PECL, so pecl install mcrypt is perfectly valid (even if abandonware).

I guess we leave it then? mark it as deprecated?

I would mark it deprecated and remove in 5.0. Removing it in 4.0 provides next to no migration path for anyone using this class (your only option is to switch to sodium which is a lot more involved than a find/replace on class names, at least with both the old and new versions running in parallel one can decrypt with the old library and re-encrypt with the new library and continue using that and not be forced to change cipher mechanisms).

PhilETaylor commented 4 years ago

PRed Adding GMP and MCrypt to docker builds so at least Joomla can be tested in the future https://github.com/joomla/docker-joomla/pull/95

SniperSister commented 4 years ago

This also makes mcrypt a system requirement for 4.x I guess?

PhilETaylor commented 4 years ago

It would be insane to make it a "requirement" (as in "it is required") just to run Joomla 4, however, like Webauthn which requires GMP extension, it can be an optional requirement IF you want to use this legacy class.

It just needs communicating (just like the webauthn GMP requirement)

mbabker commented 4 years ago

A comparable list for 4.0 should be built on https://docs.joomla.org/Special:MyLanguage/Optional_Technical_Requirements

That page (linked from the main tech requirements page BTW) was purposefully built to be able to list all of the requirements for all of the optional code integrations (i.e. Redis is supported for cache and sessions, but you don't have to have ext/redis installed to use Joomla unless you're trying to use that integration).

zero-24 commented 4 years ago

It just needs communicating (just like the webauthn GMP requirement)

Seems to be done here: https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4#PHP_mcrypt_Extension in the mean time.

A comparable list for 4.0 should be built on https://docs.joomla.org/Special:MyLanguage/Optional_Technical_Requirements

Given that this is also true for 3.x (without mcrypt installed) it would say at the time we copy / adjust that page to 4.x we have one for 4.x in the mean time the mcrypt notice has been added to that page for 3.x already.

JCrypt | Requires PHP's ext/mcrypt for all ciphers except the SodiumCipher which requires ext/sodium

So what is left here?

zero-24 commented 4 years ago

Here is the PR to deprecate CryptoCipher for Joomla 5.0: https://github.com/joomla/joomla-cms/pull/31231 Closing this here for now.