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

Several Cryptography Flaws in Magento 2 #5701

Open
paragonie-scott opened this Issue Jul 19, 2016 · 26 comments

Comments

@piotrekkaminski

This comment has been minimized.

Show comment
Hide comment
@piotrekkaminski

piotrekkaminski Jul 19, 2016

Contributor

Thanks. We are aware of the mcrypt issues and have plans to replace mcrypt with more modern library (phpseclib? libsodium?) pretty soon.

Contributor

piotrekkaminski commented Jul 19, 2016

Thanks. We are aware of the mcrypt issues and have plans to replace mcrypt with more modern library (phpseclib? libsodium?) pretty soon.

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Jul 19, 2016

In the initial comment, you said Magento is not encrypting credit card data, but it was edited before I could reply.

I do want to point out, for the record, that Magento does appear to be decrypting credit card data here.

paragonie-scott commented Jul 19, 2016

In the initial comment, you said Magento is not encrypting credit card data, but it was edited before I could reply.

I do want to point out, for the record, that Magento does appear to be decrypting credit card data here.

@piotrekkaminski

This comment has been minimized.

Show comment
Hide comment
@piotrekkaminski

piotrekkaminski Jul 19, 2016

Contributor

@paragonie-scott to be exact, none of the payment methods included with Magento stores card data.

Contributor

piotrekkaminski commented Jul 19, 2016

@paragonie-scott to be exact, none of the payment methods included with Magento stores card data.

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Jul 19, 2016

OK, that's good to know. I hope no plugins are using this for that purpose.

paragonie-scott commented Jul 19, 2016

OK, that's good to know. I hope no plugins are using this for that purpose.

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Jul 19, 2016

(phpseclib? libsodium?)

Libsodium is available as a PHP extension, so if you can't require your users to install it from PECL, that's probably not an option.

For symmetric-key encryption, you have a few good choices:

Last I checked (which, admittedly, was ages ago, so it could have changed), phpseclib didn't offer a simple and easy-to-use authenticated encryption interface like defuse does. Unauthenticated encryption, though correctly implemented, doesn't fix the sensitivity to CPAs or CCAs.

If you need public-key encryption and/or digital signatures, give EasyRSA a gander. I wrote about its benefits in response to a Drupal thread about implementing automatic security updates.

https://www.drupal.org/node/2367319#comment-11415297

paragonie-scott commented Jul 19, 2016

(phpseclib? libsodium?)

Libsodium is available as a PHP extension, so if you can't require your users to install it from PECL, that's probably not an option.

For symmetric-key encryption, you have a few good choices:

Last I checked (which, admittedly, was ages ago, so it could have changed), phpseclib didn't offer a simple and easy-to-use authenticated encryption interface like defuse does. Unauthenticated encryption, though correctly implemented, doesn't fix the sensitivity to CPAs or CCAs.

If you need public-key encryption and/or digital signatures, give EasyRSA a gander. I wrote about its benefits in response to a Drupal thread about implementing automatic security updates.

https://www.drupal.org/node/2367319#comment-11415297

@piotrekkaminski

This comment has been minimized.

Show comment
Hide comment
@piotrekkaminski

piotrekkaminski Jul 19, 2016

Contributor

Internal ticket ID MAGETWO-39838

Contributor

piotrekkaminski commented Jul 19, 2016

Internal ticket ID MAGETWO-39838

@joshspivey

This comment has been minimized.

Show comment
Hide comment
@joshspivey

joshspivey Sep 15, 2016

I just ran into this issue on production :/ when do you plan to fix this?

joshspivey commented Sep 15, 2016

I just ran into this issue on production :/ when do you plan to fix this?

@unfunco

This comment has been minimized.

Show comment
Hide comment
@unfunco

unfunco Oct 26, 2016

Member

Since ext-openssl is already a requirement of Magento, would it not make more sense to use that instead? PHP 7.1 will be throwing E_DEPRECATED for the use of ext-mcrypt, and it will either be moved to PECL in 7.2 or 8.0. Usage (ignoring vendors) is contained within Framework/Encryption and it shouldn't be too complicated to migrate (code) – migrating existing data might be more involved.

On my PHP 7.0.12 installation, the OpenSSL extension has 190 cipher methods available, it should be suitable without requiring an additional library or extension.

Member

unfunco commented Oct 26, 2016

Since ext-openssl is already a requirement of Magento, would it not make more sense to use that instead? PHP 7.1 will be throwing E_DEPRECATED for the use of ext-mcrypt, and it will either be moved to PECL in 7.2 or 8.0. Usage (ignoring vendors) is contained within Framework/Encryption and it shouldn't be too complicated to migrate (code) – migrating existing data might be more involved.

On my PHP 7.0.12 installation, the OpenSSL extension has 190 cipher methods available, it should be suitable without requiring an additional library or extension.

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Oct 27, 2016

defuse/php-encryption uses OpenSSL, but more importantly it offers simple and easy-to-use authenticated encryption.

You want authenticated encryption.

paragonie-scott commented Oct 27, 2016

defuse/php-encryption uses OpenSSL, but more importantly it offers simple and easy-to-use authenticated encryption.

You want authenticated encryption.

@unfunco

This comment has been minimized.

Show comment
Hide comment
@unfunco

unfunco Oct 27, 2016

Member

An easy-to-use abstraction over OpenSSL is fine. For simplicity's sake, I'd rather not have to compile an additional extension or use PECL, which could also be curtains shortly... hence my recommendation of OpenSSL since it's already a requirement, an additional entry in the requires object of my composer.json is a 👍 from me.

Member

unfunco commented Oct 27, 2016

An easy-to-use abstraction over OpenSSL is fine. For simplicity's sake, I'd rather not have to compile an additional extension or use PECL, which could also be curtains shortly... hence my recommendation of OpenSSL since it's already a requirement, an additional entry in the requires object of my composer.json is a 👍 from me.

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Oct 27, 2016

Understood.

We are in agreement that OpenSSL is preferable to mcrypt. I just wanted to emphasize the use of a secure abstraction instead of lower-level primitives. :)

paragonie-scott commented Oct 27, 2016

Understood.

We are in agreement that OpenSSL is preferable to mcrypt. I just wanted to emphasize the use of a secure abstraction instead of lower-level primitives. :)

@maxbucknell

This comment has been minimized.

Show comment
Hide comment
@maxbucknell

maxbucknell Dec 2, 2016

Contributor

I just checked my installation of 2.1.2, and the develop branch, and we can see that use of the mcrypt_* is still alive and well:

See Magento\Framework\Encryption\Crypt

<?php

// ....
public function __construct($key, $cipher = MCRYPT_BLOWFISH, $mode = MCRYPT_MODE_ECB, $initVector = false)
{
    $this->_cipher = $cipher;
    $this->_mode = $mode;
    $this->_handle = mcrypt_module_open($cipher, '', $mode, '');
    try {
        $maxKeySize = mcrypt_enc_get_key_size($this->_handle);
    
    // ...

Since it's not reasonably possible to say that PHP 7.1 is supported without this, and given that PHP 7.1 is now out, can we have a status update on this ticket MAGETWO-39838?

Contributor

maxbucknell commented Dec 2, 2016

I just checked my installation of 2.1.2, and the develop branch, and we can see that use of the mcrypt_* is still alive and well:

See Magento\Framework\Encryption\Crypt

<?php

// ....
public function __construct($key, $cipher = MCRYPT_BLOWFISH, $mode = MCRYPT_MODE_ECB, $initVector = false)
{
    $this->_cipher = $cipher;
    $this->_mode = $mode;
    $this->_handle = mcrypt_module_open($cipher, '', $mode, '');
    try {
        $maxKeySize = mcrypt_enc_get_key_size($this->_handle);
    
    // ...

Since it's not reasonably possible to say that PHP 7.1 is supported without this, and given that PHP 7.1 is now out, can we have a status update on this ticket MAGETWO-39838?

@ktomk

This comment has been minimized.

Show comment
Hide comment
@ktomk

ktomk Dec 4, 2016

Contributor

For cross-reference: In Magerun 2 we've added PHP 7.1 support now and do see the various reported issues with Magento 2 now as well as they became blatantly obvious. We're tracking this downstream at netz98/n98-magerun2#256.

Issues I could spot so far within the Magento 2 Github Tracker are:

  • #3881 - PHP mcrypt extension to be removed in PHP 7.1
  • #5701 - Several Cryptography Flaws in Magento 2
  • #5880 - Magento 2.1 php7.1 will not be supported due to mcrypt deprecation
  • #7663 - Magento 2.1.x No support for PHP7.1

I suggest to add 7.1 to the Travis build and allow it to fail. That should make it easier to progress and keep track of things.

Contributor

ktomk commented Dec 4, 2016

For cross-reference: In Magerun 2 we've added PHP 7.1 support now and do see the various reported issues with Magento 2 now as well as they became blatantly obvious. We're tracking this downstream at netz98/n98-magerun2#256.

Issues I could spot so far within the Magento 2 Github Tracker are:

  • #3881 - PHP mcrypt extension to be removed in PHP 7.1
  • #5701 - Several Cryptography Flaws in Magento 2
  • #5880 - Magento 2.1 php7.1 will not be supported due to mcrypt deprecation
  • #7663 - Magento 2.1.x No support for PHP7.1

I suggest to add 7.1 to the Travis build and allow it to fail. That should make it easier to progress and keep track of things.

ktomk added a commit to ktomk/magento2 that referenced this issue Dec 6, 2016

Add PHP 7.1 to Travis build
This change is for forward compatibility with PHP 7.1.

Add PHP 7.1 (currently PHP 7.1-RC6 on Travis) to the Travis build. Allow
failures with it as failures are expected.

To get it to work, for PHP 7.1 composer ignores platform requirements.

Refs:

- magento#5701
@ktomk

This comment has been minimized.

Show comment
Hide comment
@ktomk

ktomk Jan 23, 2017

Contributor

In the light of the recent security report with the remote code execution issue on sending mails (#6146, ZF2016-04), I would suggest Magento to run checks with RIPS which is able to find injection flaws of such kind (e.g. in Roundcube, published Dec 6 2016). Just commenting for reference /cc @piotrekkaminski

Contributor

ktomk commented Jan 23, 2017

In the light of the recent security report with the remote code execution issue on sending mails (#6146, ZF2016-04), I would suggest Magento to run checks with RIPS which is able to find injection flaws of such kind (e.g. in Roundcube, published Dec 6 2016). Just commenting for reference /cc @piotrekkaminski

@ktomk

This comment has been minimized.

Show comment
Hide comment
@ktomk

ktomk May 3, 2017

Contributor

X-Ref:

This can be now closed as PHP 7.1 became a part of Travis CI builds officially: https://github.com/magento/magento2/blob/develop/.travis.yml#L13

via: #7688 (comment)

Contributor

ktomk commented May 3, 2017

X-Ref:

This can be now closed as PHP 7.1 became a part of Travis CI builds officially: https://github.com/magento/magento2/blob/develop/.travis.yml#L13

via: #7688 (comment)

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott May 3, 2017

One possible solution to the cryptography flaws laid out here is sodium_compat, but it's not yet known if it's safe to use yet. Only a cryptography audit will tell.

paragonie-scott commented May 3, 2017

One possible solution to the cryptography flaws laid out here is sodium_compat, but it's not yet known if it's safe to use yet. Only a cryptography audit will tell.

@markoshust

This comment has been minimized.

Show comment
Hide comment
@markoshust

markoshust May 17, 2017

Contributor

What's the TAT on a release for PHP 7.1 compatibility?

Contributor

markoshust commented May 17, 2017

What's the TAT on a release for PHP 7.1 compatibility?

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Jun 14, 2017

sodium_compat v1.0 is out if the Magento team wants to switch to more secure cryptography

paragonie-scott commented Jun 14, 2017

sodium_compat v1.0 is out if the Magento team wants to switch to more secure cryptography

@SharkWipf

This comment has been minimized.

Show comment
Hide comment
@SharkWipf

SharkWipf Jul 11, 2017

Libsodium has just been merged into PHP 7.2 so, from 7.2 onwards, it's no longer dependent on an extension.

SharkWipf commented Jul 11, 2017

Libsodium has just been merged into PHP 7.2 so, from 7.2 onwards, it's no longer dependent on an extension.

@magento-team

This comment has been minimized.

Show comment
Hide comment
@magento-team

magento-team Jul 31, 2017

Contributor

Internal ticket to track issue progress: MAGETWO-66161

Contributor

magento-team commented Jul 31, 2017

Internal ticket to track issue progress: MAGETWO-66161

@barryvdh

This comment has been minimized.

Show comment
Hide comment
@barryvdh

barryvdh Sep 13, 2017

Contributor

So given the state of M2.2, it will never work on PHP7.2? If M2.3 will use Sodium, does that mean it will only work with PHP7.2 and above, or will the compat module be included (or suggested for lower versions)?

Contributor

barryvdh commented Sep 13, 2017

So given the state of M2.2, it will never work on PHP7.2? If M2.3 will use Sodium, does that mean it will only work with PHP7.2 and above, or will the compat module be included (or suggested for lower versions)?

@magento-engcom-team

This comment has been minimized.

Show comment
Hide comment
@magento-engcom-team

magento-engcom-team Dec 7, 2017

Contributor

@paragonie-scott, thank you for your report.
We've created internal ticket(s) MAGETWO-39838 to track progress on the issue.

Contributor

magento-engcom-team commented Dec 7, 2017

@paragonie-scott, thank you for your report.
We've created internal ticket(s) MAGETWO-39838 to track progress on the issue.

@ishakhsuvarov

This comment has been minimized.

Show comment
Hide comment
@ishakhsuvarov

ishakhsuvarov Apr 13, 2018

Contributor

Implementation for 2.3 is currently in progress in the scope of magento-engcom/php-7.2-support project

Contributor

ishakhsuvarov commented Apr 13, 2018

Implementation for 2.3 is currently in progress in the scope of magento-engcom/php-7.2-support project

@peterjaap

This comment has been minimized.

Show comment
Hide comment
@peterjaap

peterjaap Aug 23, 2018

Collaborator

@ishakhsuvarov judging from the composer.json in that branch, M2.3.1 will still be using a polyfill for mcrypt. Can you confirm there is an initiative underway to actually implement libsodium along with sodium_compat instead of mcrypt, phpseclib and mcrypt_compat?

Collaborator

peterjaap commented Aug 23, 2018

@ishakhsuvarov judging from the composer.json in that branch, M2.3.1 will still be using a polyfill for mcrypt. Can you confirm there is an initiative underway to actually implement libsodium along with sodium_compat instead of mcrypt, phpseclib and mcrypt_compat?

@paragonie-scott

This comment has been minimized.

Show comment
Hide comment
@paragonie-scott

paragonie-scott Aug 23, 2018

I can answer this one, as I've been involved in the migration strategy and protocol review.

The gameplan is to switch to libsodium (with sodium_compat as a fallback when ext/sodium isn't available).

However, for the sake of decrypting existing ciphertexts after the M2 upgrade, mcrypt_compat was included as well. Its purpose is to enable seamless migrations to better encryption, not to keep Magento in the dark ages of cryptography.

paragonie-scott commented Aug 23, 2018

I can answer this one, as I've been involved in the migration strategy and protocol review.

The gameplan is to switch to libsodium (with sodium_compat as a fallback when ext/sodium isn't available).

However, for the sake of decrypting existing ciphertexts after the M2 upgrade, mcrypt_compat was included as well. Its purpose is to enable seamless migrations to better encryption, not to keep Magento in the dark ages of cryptography.

@peterjaap

This comment has been minimized.

Show comment
Hide comment
@peterjaap

peterjaap Aug 23, 2018

Collaborator

@paragonie-scott thanks for the explanation! Just listened to you on the Roundtable ep 71 (and 73) yesterday, great work on libsodium w/ PHP7.2 👍

Collaborator

peterjaap commented Aug 23, 2018

@paragonie-scott thanks for the explanation! Just listened to you on the Roundtable ep 71 (and 73) yesterday, great work on libsodium w/ PHP7.2 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment