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

If You're Typing the Word MCRYPT Into Your PHP Code, You're Doing It Wrong #129

Closed
colinmollenhour opened this issue Oct 25, 2016 · 8 comments

Comments

@colinmollenhour
Copy link
Member

colinmollenhour commented Oct 25, 2016

https://paragonie.com/blog/2015/05/if-you-re-typing-word-mcrypt-into-your-code-you-re-doing-it-wrong

Mage_Core_Model_Encryption uses Varien_Crypt which implements mcrypt by default.
I propose the following:

  • Add Varien_Crypt_Openssl to implement encryption via Openssl using whatever the best practices are for secure encryption.
  • Read a config key global/crypt/method to get a specific method if specified. Default to mcrypt if not specified for backwards compatibility.
  • Add global/crypt/method -> openssl to the local.xml.template so that for new installations the default is openssl. I don't really care to support platforms without openssl so I don't even know if one exists or if they are common but I think it is perfectly reasonable to require a supported encryption library for an ecommerce platform.. It can easily be added to list of modules required during install.
  • Write a script to migrate encrypted data from old method/key to new method/key. (find all encrypted config values, decrypt, re-encrypt with new key/method and save over old local.xml)
    • Note, Magento CE had a major bug up until 1.8/1.9 whereby a VERY poor encryption key was generated at installation so without a migration script if you are using one of these keys you basically might as well not even be using any encryption it is so bad. I tried to raise this with Magento on a magento2 issue but my warnings were completely ignored since they already have such a migration script for EE apparently.

EDIT:
Oh yeah, mcrypt will be deprecated in PHP 7.1 and removed in the version after. More info: https://wiki.php.net/rfc/mcrypt-viking-funeral

@Flyingmana
Copy link
Contributor

👍
And yes, there are platforms without openSSL out there, but we can sure add alternates then.

@sreichel
Copy link
Contributor

Reference ... Inchoo/Inchoo_PHP7#98

@inluxc
Copy link

inluxc commented Nov 6, 2017

This is a nice read about this problem.
https://www.zimuel.it/slides/phpce2017#/

@colinmollenhour
Copy link
Member Author

Just pushed PR for this issue. See #506

@kkrieger85
Copy link
Contributor

Close due to #506

@seansan
Copy link
Contributor

seansan commented Sep 28, 2020

@colinmollenhour
Copy link
Member Author

Should this also be changed? https://github.com/OpenMage/magento-lts/blob/1.9.4.x/lib/Zend/Filter/Encrypt.php#L85

Is that code used anywhere? I'd rather remove unused code than try to fix it.

@seansan
Copy link
Contributor

seansan commented Oct 1, 2020 via email

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

No branches or pull requests

6 participants