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

mcrypt library is long ago abandoned #7215

Closed
tom-- opened this issue Feb 9, 2015 · 14 comments
Closed

mcrypt library is long ago abandoned #7215

tom-- opened this issue Feb 9, 2015 · 14 comments
Assignees
Labels
severity:security status:ready for adoption Feel free to implement this issue.
Milestone

Comments

@tom--
Copy link
Contributor

tom-- commented Feb 9, 2015

The upstream we all derive from hasn't been touched by its authors in 8 years. The last update in Debian was 5 or 6 years ago. RHEL has dropped it.

I think it's fairly a straight forward task to replace everything we use from mcrypt in yii\base\Security. The API will be backwards compatible and I think encrypted data will be compatible too, if we do it right.

So I think we can remove mcrypt from Security without it being a breaking change. The headache for users would be the openssl extension requirement. But I think that is widely deployed already because PHP relies on it for all the SSL io in built-ins.

There may be small gains too, e.g. better random numbers on Windows.

I will start work this week if there's consensus to do this.

@samdark
Copy link
Member

samdark commented Feb 9, 2015

I confirm that everything said about mcrypt isn't maintained is true. It's good to replace it and openssl looks like a good alternative.

@samdark
Copy link
Member

samdark commented Feb 9, 2015

Not sure about milestone though since while API could be compatible, requirements won't be.

@samdark samdark added severity:security status:ready for adoption Feel free to implement this issue. labels Feb 9, 2015
@tom--
Copy link
Contributor Author

tom-- commented Feb 9, 2015

In terms of requirements, on Linux, BSD or most Unix platforms, OpenSSL will be required only for encrypt and decrypt. On Windows, OpenSSL will be required also for randomness.

So requirements will change but OpenSSL will, on *nux/nix platforms, be not a requirement for Yii but a requirement for encrypt/decrypt, which is much weaker.

@samdark
Copy link
Member

samdark commented Feb 9, 2015

OpenSSL extension is required for Composer to download from secure locations so it's likely installed.

@tom--
Copy link
Contributor Author

tom-- commented Feb 9, 2015

completely untested first draft

only look at if you're really interested, it's not ready for real review

@tom--
Copy link
Contributor Author

tom-- commented Feb 10, 2015

@dynasource
Copy link
Member

its definitely time to upgrade to OpenSSL or better. Requirements are less important in my opinion.

@Faryshta
Copy link
Contributor

reading the rfc mail list is so painful. PHP is not great because of their custodians but despite of them.

The scalar typehinting voting right now is a huge example.

@samdark
Copy link
Member

samdark commented Feb 10, 2015

@Faryshta yup, but let's keep on topic.

@tom--
Copy link
Contributor Author

tom-- commented Feb 10, 2015

In my yii2 repo these two classes are passing the tests:

https://github.com/tom--/yii2/blob/7215-replace-mcrypt/framework/base/Security.php
https://github.com/tom--/yii2/blob/7215-replace-mcrypt/tests/unit/framework/base/SecurityTest.php

I'm not happy about the base64 bullshit on lines 179 and 219. But I couldn't get openssl_en/decrypt to work with raw binary data. Any help greatly appreciated.

I added unit tests and test vectors to test:

  • Forwards compat: encrypt data with Yii 2.0.2 using Mcrypt and decrypt with my new version using OpenSSL
  • Backwards compat: encrypt data with my new version using OpenSSL and decrypt with Yii 2.0.2 using Mcrypt

The whole thing is confusing and I don't know how to organize this so we have regression tests for forwards and backwards compat in future releases of Yii. Again, any help greatly appreciated.

The SecurityTest class also has the test vector generator I needed but that isn't actually a test. It should be removed and the test vectors set in stone once we have this figured out.

@cebe
Copy link
Member

cebe commented Feb 11, 2015

@samdark
Copy link
Member

samdark commented Feb 11, 2015

Not sure what to do with base64encode. All PHP implementations I'm aware of are using it.

I guess we need to move old implementation into tests in order to check backwards compatibility.

@yurii-github
Copy link
Contributor

i may be wrong but according to php.net it was updated at 5.6 version at least once
http://php.net/manual/en/function.mcrypt-encrypt.php
and is native cipher support. why to make it harder?

also

openssl
https://bugs.php.net/search.php?cmd=display&search_for=openssl+related

mcrypt
https://bugs.php.net/search.php?cmd=display&package_name[]=mcrypt+related

mature doesn't need to be changed often

@samdark
Copy link
Member

samdark commented Feb 12, 2015

@yurii-github PHP's library is a wrapper around C mcrypt that was abandoned in 2003. There are about 10 bugs most probably related to the algorithm itself that aren't going to be fixed since some of these are 12 years old.

@samdark samdark added this to the 2.0.3 milestone Feb 24, 2015
@samdark samdark self-assigned this Feb 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity:security status:ready for adoption Feel free to implement this issue.
Projects
None yet
Development

No branches or pull requests

6 participants