Skip to content
This repository has been archived by the owner on Jun 10, 2019. It is now read-only.

mcrypt is deprecated in PHP7.1 #98

Open
benjamw opened this issue Jan 4, 2017 · 14 comments
Open

mcrypt is deprecated in PHP7.1 #98

benjamw opened this issue Jan 4, 2017 · 14 comments

Comments

@benjamw
Copy link
Contributor

benjamw commented Jan 4, 2017

Fatal error: Uncaught Exception: Deprecated functionality: Function mcrypt_module_open() is deprecated in \lib\Varien\Crypt\Mcrypt.php on line 63 in \app\code\core\Mage\Core\functions.php on line 245

Not sure exactly what to do about this as I'm not sure if these classes can be overridden.

As an interim solution, you can add

error_reporting(E_ALL ^ E_DEPRECATED);

to the top of \lib\Varien\Crypt\Mcrypt.php.

@icurdinj
Copy link
Contributor

icurdinj commented Jan 4, 2017

Can you give just a bit more context of when that fatal error occured? Mcrypt should throw deprecated warnings in PHP 7.1, but not crash...

@benjamw
Copy link
Contributor Author

benjamw commented Jan 4, 2017

I have my error reporting turned all the way up (E_ALL), so I'm not hiding anything when I code. If your settings are different, you may not get these errors.

@Alanaktion
Copy link

Magento defaults to throwing an Exception on all errors (including warnings, notices, and deprecations) if the developer mode is enabled (with SetEnv or similar), which, if uncaught, would be a fatal error.

Ideally the class could be overridden, but I don't think it'd be trivial to make a compatible drop-in replacement for the whole thing. It looks like mcrypt functions are called from a few different places in lib/, including a couple third-party libraries that are referenced in core code.

@benjamw
Copy link
Contributor Author

benjamw commented Jan 6, 2017

I do have developer mode enabled on my local machine... so that may be why I'm getting those fatal errors.

But yeah, I figured it would be a pretty tedious fix, considering how often mcrypt is used in Magento.

@csdougliss
Copy link

Is there anyway to ignore depreciation messages for mcrypt only? Or can we just patch to use a different function?

@icurdinj
Copy link
Contributor

@ivanweiler found a way to patch and not use mcrypt. What I've seen so far looks great, I'm awaiting the pull request from him.

@csdougliss
Copy link

@icurdinj any updates?

@ivanweiler
Copy link
Contributor

Hello all, this is going to be a long one, sorry :)

There are 3 things using mcrypt in M1:

  1. Varien_Crypt_Mcrypt used by main Core_Model_Encryption
  2. older version of lib/phpseclib used by Paypal_Model_Report_Settlement -> Varien_Io_Sftp
  3. install.xml - installation is checking if mcrypt exists

My initial (optimistic) idea was to write new Openssl adapter and change main encryption model to use it (same is mentioned for LTS project).
However, EE is using it's own Enterprise_Pci_Model_Encryption, so we need to be compatible with that too? Furthermore

  1. Enterprise_Pci_Model_Encryption is using MCRYPT_ constants. If you disable mcrypt, it will break.

Mcrypt / Openssl compatibility

It's important to note that mcrypt always used zero padding for encryption, which is considered bad practice these days. Openssl is using PKCS#7, but it can be forced to use less secure zero padding.

  • Magento CE is using Blowfish/ECB/Zero padding - openssl can be fully compatible with previous mcrypt encrypted values only if encryption key is 16 chars or more (the one you set on install)
  • Magento EE is using Rijndael-256/CBC/Zero padding, but it's also compatible with Rijndael-128/ECB/Zero padding, most likely from previous EE versions. I think mcrypt Rijndael (AES) can be fully compatible with openssl, with again, zero padding.

Solution 1
If we want to be 100% compatible with old mcrypt (avoid changing any encrypted values in db), and since openssl can't do Blowfish with key below 16, I think best solution is to use

phpseclib/mcrypt_compat - basically polyfill for mcrypt

This requires adding Composer support in M1!!, but no code change is required, MCRYPT_ constants in EE are compensated so you can disable mcrypt on server.
New phpseclib that uses openssl is also installed along polyfill (requirement), so Varien_Io_Sftp can be fixed with one line of code (I tested it, it works). For Blowfish keys below 16, phpseclib uses native php implementation, so everything should be 100% compatible with old encrypted values.

Ofc, mcrypt_compat is also using zero padding since it's trying to be fully compatible with old mcrypt.

Solution 2
Forget about compatibility and write new Encryption model that will use latest encryption standards (AES/CBC like EE?) and setup script that will "convert" all old encrypted values to new more secure encrypted values.
But, I don't think this is something PHP7 compatibility module should handle !? @icurdinj

Varien_Io_Sftp can't be switched to openssl without updating phpseclib.

install.xml can't be "rewritten" to remove mcrypt dependency from install, model that loads install.xml can, so it's a dirty hack either way.

Anyway, I would like to hear what others think is the best way to handle this, since I'm not sure at this point :)

P.S. I see that openssl is going to be deprecated in 7.2 by libsodium? Will this be used for AES in the future, should we wait a bit?

P.S.2. I'm curious to see how M2 is going to handle this, since mcrypt code and (EE) encryption adapter was just ported to M2

@icurdinj
Copy link
Contributor

A master has spoken :-)

I've just implemented solution 1 to our composer.json - it now suggests installing phpseclib/mcrypt_compat. Install that, disable mcrypt, and it should just work.

Having said that, it's better to simply not bother with PHP 7.1: https://github.com/Inchoo/Inchoo_PHP7/wiki/RunningOnPHP7.1

About solution 2: yes, it's out of scope of this module, but I can't wait to see @ivanweiler making a better security module based on libsodium...

@postadelmaga
Copy link

Any update on this ?

I thinks this is the M2 related bug:
magento/magento2#5880

@mklooss
Copy link

mklooss commented Dec 15, 2017

@ivanweiler, @icurdinj but with "mcrypt_compat" you have to update phpseclib.
The reason is easy, magento use an old version of phpseclib and will missing some classes.
I've placed the current version of phpseclib in app/code/community and just found in "lib/Varien/Io/Sftp.php" the reason for phpseclib. non other magento-base file use phpseclib.

i've created an simple "module" wich doing this job well. based on Solution 2.

https://github.com/mklooss/Magento_Varien_Crypt_Mcrypt
it's an proof of concept module.

but i will also take a look at an other solution based on openssl, but the convert job will be the hardest part

@postadelmaga
Copy link

postadelmaga commented Dec 19, 2017

This presentation an these 2 slides in particular should be helpful to explore other solutions based on openSSL

I think the start point is to create a class in /lib/Varian/Crypt/OpenSSL.php implenting the methods init() encrypt() , decrypt() so something like this:

<?php
/**
 * Class Varien_Crypt_OpenSSL
 */
class Varien_Crypt_OpenSSL extends Varien_Crypt_Abstract
{
    CONST CIPHER_AES256 = 'aes-256-gcm';
    CONST IV_SIZE_DEFAULT = 16;  // size for aes-256-gcm

    /**
     * Initialize OPENSSL module
     *
     * @param string $key cipher private key
     * @return Varien_Crypt_Mcrypt
     */
    public function init($key)
    {
        $this->setIvSize(self::IV_SIZE_DEFAULT);
        if (!$this->getCipher()) {
            $this->setCipher(self::CIPHER_AES256);
        }

        if (!$this->getMode()) {
            $this->setMode(OPENSSL_RAW_DATA);
        }

        if (!$this->getKey()) {
            $this->setKey($key);
        }

        return $this;
    }

    /**
     * Encrypt data
     *
     * @param string $data source string
     * @return string
     */
    public function encrypt($data)
    {
        if (strlen($data) == 0) {
            return $data;
        }

        $iv = random_bytes($this->getIvSize());
        $data = openssl_encrypt(
            $data,
            $this->getCipher(),
            $this->getKey(),
            $this->getMode(),
            $iv,
            $tag // return values
        );

        if (false === $data) {
            throw new Exception ('Error on encrypt');
        }
        return $iv . $tag . $data;
    }

    function decrypt($data)
    {
        if (strlen($data) == 0) {
            return $data;
        }

        $ivsize = $this->getIvSize();

        $data = openssl_decrypt(
            mb_substr($data, 32, null, '8bit'),
            $this->getCipher(),
            $this->getKey(),
            $this->getMode(),
            mb_substr($data, 0, $ivsize, '8bit'),
            mb_substr($data, $ivsize, $ivsize, '8bit')
        );

        if (false === $data) {
            throw new Exception('Error on decrypt');
        }

        return $data;
    }
}

@ivanweiler
Copy link
Contributor

ivanweiler commented Feb 5, 2018

Little update regarding mcrypt.

We wrote openssl adapter that should be fully compatible with both CE (blowfish) and EE (aes) on php >= 7.1.8.
Lower php versions have openssl bug and adapter is fully compatible on CE only if encryption key is >=16 bytes.

Code can be seen in current develop branch
https://github.com/Inchoo/Inchoo_PHP7/tree/develop

For devs that are using composer with Magento1, mcrypt_compat will still be recommended way to move away from mcrypt. For devs that don't want to use composer, this should work just fine for encryption/decryption.

Anyway, if someone care to test a bit (on latest CE && EE 1.x versions), that would be great !! We're planning new version of extension one of these days.

It works out of the box on CE, on EE please uncomment this line to replace default EE encryptor:

<!--<encryption_model>Inchoo_PHP7_Model_EncryptionEE</encryption_model>-->

Regards

@mklooss
Copy link

mklooss commented Feb 9, 2018

thanks for the workaround, just one request, could you move the code pool to community
otherwise its not possible to implement own stuff oder other encryption models etc.

or keep Varien_Crypt compatible to override the enc. model

    static public function factory($method='mcrypt')
    {
        $uc = str_replace(' ','_',ucwords(str_replace('_',' ',$method)));
        $className = 'Varien_Crypt_'.$uc;
        return new $className;
    }

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants