Remove mcrypt as a dependency #101

skymeyer · 2016-04-13T00:39:17Z

I'd be a big fan of doing a restructure on the code as currently adding/extending the library is a bit of a pain as per #72. I have stuck to the current code paradigms to remove mcrypt in this PR.

Fixes #55, #87 and #2.

skymeyer · 2016-05-07T00:48:36Z

@robrichards can this be reviewed ? As per https://wiki.php.net/rfc/mcrypt-viking-funeral mcrypt (as known for a long time) is abandon ware. A new major version of xmlseclib is probably required as PHP 5.4 is the absolute minimum to get rid of mcrypt (see note about padding above - unless there is an alternative).

If need you can always maintain 2.x for 5.3 with mcrypt and 3.x for 5.4+ without mcrypt if that makes sense.

lt · 2016-09-02T16:48:20Z

@robrichards Please can you re-visit both this and #100

ext/mcrypt is deprecated as of PHP 7.1, and in all likelihood will be removed in 7.2

I understand your reservations about introducing openssl as a dependency, but given that there is now a plan for the removal of mcrypt I think it makes sense to make this change sooner rather than later, with an appropriate version bump to go with it.

restena-sw · 2016-10-20T06:36:04Z

What is the issue of going up to 5.4 as a minimum requirement anyway? 5.4 (and even 5.5 for that matter) are already out of support and do not receive any security fixes by the PHP team any more!
http://php.net/supported-versions.php

lt · 2016-10-31T10:34:43Z

Change my previous comment. ext/mcrypt will definitely be removed from core in 7.2

robrichards · 2016-10-31T12:20:02Z

I need to go through this commit and make the changes to remove the crypt dependancy. The PR includes additional changes on top of the mcrypt removal. Will take care of it this week

jaimeperez · 2016-12-05T14:39:36Z

@robrichards any news on this?

lt · 2016-12-12T18:10:54Z

I have now removed mcrypt from php-src/master

I'd really prefer not to have to fork this library :(

codecov-io · 2016-12-14T05:20:15Z

Current coverage is 28.40% (diff: 77.65%)

No coverage report found for master at 1369dd1.

Powered by Codecov. Last update 1369dd1...4d85872

skymeyer · 2016-12-14T05:25:39Z

Thanks for the headsup @lt, that was expected 👍

@robrichards is there anything you want me to do on this PR to move forward with this ? I recommend first looking at #100 as this PR is an increment of it.

Is there somebody else who can help you reviewing the proposed changes ? Maybe @paragonie-scott or @ircmaxell ?

robrichards · 2016-12-14T15:03:43Z

I thought about it a bit as I really didn't want to add additional dependency unless absolutely required. With PHP 5.6 going into security only support I am thinking about making Master branch PHP 7+ only which means we can drop the paragonie/random_compat requirement. Can you make that change as well as well as update the composer file to drop all PHP 5 support?

jaimeperez · 2016-12-14T15:17:28Z

Uhm, that's a bit drastic, isn't it?

"Security support only" is quite different than "no support at all", and considering how some distros keep supporting old versions backporting security fixes for them (i.e. PHP 5.3 still supported by Red Hat), I think dropping support for PHP 5 completely is a bit too much.

I fully agree with the principle of minimizing the dependencies, but not at any expense.

Besides, even if we drop support for PHP 5 in master, that's still development. Stable releases will still depend on mcrypt, and everybody will, of course, depend on stable releases. What we really need here is to drop the dependency on mcrypt in the stable releases (at least 2.0), and then we simply can't afford dropping support for PHP 5 in a stable release.

robrichards · 2016-12-14T15:21:59Z

I was planning on keeping 2.0 as is and creating a 3.0 release for the PHP 7+.

jaimeperez · 2016-12-14T15:36:30Z

But having 2.0 as is (meaning, requiring mcrypt) and a 3.0 dropping the dependency on mcrypt and support for PHP 5 doesn't change much, right? People who can't get rid of PHP 5 won't be able to get rid of mcrypt either...

robrichards · 2016-12-14T16:43:38Z

It looks like there is an alternative which would allow mcrypt to be dropped using PHP 5.6.24+. The only place random_bytes would have been needed is in when generating the session key as the guid generation did not need to be cryptographically strong. Now that openssl_random_pseudo_bytes has been fixed under forked conditions in June, and was fixed a while back with respect to the cryptographic strength, that can be used exclusively in the generateSessionKey method. I think this may be a decent compromise.

jaimeperez · 2016-12-14T17:26:07Z

That sounds like an improvement indeed, thanks!

If I understand correctly, openssl_random_pseudo_bytes() is not cryptographically secure before PHP 5.6.24. Is that correct? Is there any other alternative that we could use as a CSPRNG, even as an added dependency?

My concern is that installing mcrypt is hard nowadays (and I think that's how it should be, given that it has been unmaintained for a decade or more), so removing that dependency should have top priority. If we can use another CSPRNG that runs even in older versions of PHP (ok, maybe not 5.3, but definitely at least 5.4), then that's a path worth following, even if that requires an extra dependency (given that such dependency is easier to install than mcrypt, of course).

paragonie/random_compat seems also like a compromise solution, given that you can install it with composer and it doesn't have any requisites on non-php dependencies. I don't have experience using it, but after taking a brief look, they look like competent people to me.

At the very least, I'd say it's preferable to remove the mcrypt dependency on all the branches in xmlseclibs, even if that implies depending on paragonie/random_compat. Then we can also have a new branch (3.0 as you were suggesting) that relies only on openssl_random_pseudo_bytes() and requires PHP 5.6.24 at least, and in the future, when PHP 7 is common ground, we can start using random_bytes().

skymeyer · 2016-12-14T20:59:33Z

@robrichards I can definitely change this PR targeting PHP 7+ only in master without the need of the paragonie/random_compat polyfil to prepare for a 3.0 release and move forward removing the mcrypt dependency.

Regarding the 2.0 release train: I agree that PHP 5.6.24+ solves the issue of openssl_random_pseudo_bytes of not being cryptographically secure. However as this is a security library, we should help dependents of this library to have a secure environment and not assume that they understand its internals and having a false sense of security.

These are the options I see for the 2.0 release train:

Bumping the required PHP version to 5.6.24 in the 2.0 release - this is a BC change so I'm not in favor of that
Duplicating the logic from paragonie/random_compat into xmlseclibs directly - this avoids introducing an additional dependency, but requires some copy/pasta trying to solve an issue which is already properly addressed elsewhere
Compromising having paragonie/random_compat in the 2.0 release - this is still the best course of action to me

Let me know what you think on the above. I'll proceed meanwhile getting this PR ready for PHP7+ only as requested. We can see what we can backport into 2.0 later on regarding test restructure and coverage.

* Use openssl for symmetric ciphers * Adding ext-openssl as a dependency instead of suggestion * Adding mbstring safe strlen and substr * Adding plumbing unit tests using autoloader exclusively * Bump minimum PHP version to 7 * Remove deprecated xmlseclibs.php

skymeyer · 2016-12-15T03:30:52Z

@robrichards this PR has been updated targeting PHP 7+ only without the introduction of random_compat. I have updated the test matrix using the two recent LTS releases of HHVM as well.

skymeyer · 2016-12-15T03:49:20Z

src/XMLSecEnc.php

@@ -315,7 +315,7 @@ public function encryptKey($srcKey, $rawKey, $append=true)
            $this->encKey = $encKey;
        }
        $encMethod = $encKey->appendChild($this->encdoc->createElementNS(self::XMLENCNS, 'xenc:EncryptionMethod'));
-        $encMethod->setAttribute('Algorithm', $srcKey->getAlgorith());
+        $encMethod->setAttribute('Algorithm', $srcKey->getAlgorithm());


If master will used to create a new 3.0 release train as is on the table, I suggest we remove the deprecated methods XMLSecurityDSig::generate_GUID and XMLSecurityKey::getAlgorith as well. This is not part of this PR yet.

robrichards · 2017-02-13T19:09:21Z

@skymeyer @jaimeperez Sorry its taken so long. My local env got pooched so have been rebuilding it. I paired down the pull request to only changes dealing with removing mcrypt - I do agree with some of the other changes as they do fix some things but will add them as separate commits once we get through this one. For this request I went with the change that will work for 5.6.24 only. I would look at making this a 3.0 branch. Would you all please take a look at the changes in my remove_mcrypt branch? I had to commit blind as I couldn't run the tests but appears they passed in Travis. Let me know what you guys think?

Hikariii · 2017-02-14T09:34:15Z

👍 for all the work being done here! Looking forward to using this in https://github.com/onelogin/php-saml

robrichards · 2017-03-09T16:25:39Z

@skymeyer @jaimeperez Just realized I hadn't pushed my changes. They are there now. I still have a broken env (problem with my openssl so haven't been able to test locally yet) so this was committed blind. Will try to resolve that with a different env today as it looks like all the tests failed so I need to go through it.

skymeyer · 2017-03-09T22:08:49Z

Thanks @robrichards - will take a look tomorrow and run some tests.

skymeyer · 2017-03-29T01:45:51Z

@robrichards I have reviewed the commit in the remove_mcrypt branch. There were a few things missing with respect to test fixtures and the logic to call encryptSymmetric and decryptSymmetric respectively. This new PR should correct this: #123

robrichards · 2017-05-25T15:18:46Z

dupe of #123

skymeyer force-pushed the remove_mcrypt branch from ce9fce0 to 0a11166 Compare April 13, 2016 00:40

skymeyer changed the title ~~Remove mcrypt as a dependency~~ Remove mcrypt as a dependency (WIP) Apr 13, 2016

skymeyer force-pushed the remove_mcrypt branch 6 times, most recently from 3b5dd0c to b6cbb97 Compare April 13, 2016 03:04

skymeyer changed the title ~~Remove mcrypt as a dependency (WIP)~~ Remove mcrypt as a dependency Apr 13, 2016

jaimeperez mentioned this pull request Dec 5, 2016

mcrypt deprecated in PHP 7.1 simplesamlphp/simplesamlphp#526

Closed

skymeyer force-pushed the remove_mcrypt branch from b6cbb97 to 8944eb6 Compare December 14, 2016 05:18

skymeyer mentioned this pull request Dec 14, 2016

Use composer version of xmlseclibs SAML-Toolkits/php-saml#81

Closed

skymeyer force-pushed the remove_mcrypt branch 2 times, most recently from dfcbc67 to 9a50ca4 Compare December 14, 2016 23:55

skymeyer force-pushed the remove_mcrypt branch 6 times, most recently from 5fb4f27 to 1b7cb4a Compare December 15, 2016 01:00

skymeyer mentioned this pull request Dec 15, 2016

Avoid insecure openssl_random_pseudo_bytes #100

Closed

skymeyer force-pushed the remove_mcrypt branch from 1b7cb4a to 18231ef Compare December 15, 2016 03:00

skymeyer force-pushed the remove_mcrypt branch from 18231ef to 4d85872 Compare December 15, 2016 03:13

skymeyer mentioned this pull request Dec 15, 2016

HHVM seg fault #87

Closed

skymeyer commented Dec 15, 2016

View reviewed changes

amochohan mentioned this pull request Feb 8, 2017

Replace mcrypt functionality with openssl SAML-Toolkits/php-saml#79

Closed

thijskh mentioned this pull request Feb 14, 2017

Migration from mcrypt PHP extension to OpenSSL #120

Closed

skymeyer mentioned this pull request Mar 29, 2017

Remove mcrypt dependency #123

Closed

6 tasks

robrichards closed this May 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove mcrypt as a dependency #101

Remove mcrypt as a dependency #101

skymeyer commented Apr 13, 2016 •

edited

Loading

skymeyer commented May 7, 2016

lt commented Sep 2, 2016

restena-sw commented Oct 20, 2016

lt commented Oct 31, 2016

robrichards commented Oct 31, 2016

jaimeperez commented Dec 5, 2016

lt commented Dec 12, 2016 •

edited

Loading

codecov-io commented Dec 14, 2016 •

edited

Loading

skymeyer commented Dec 14, 2016

robrichards commented Dec 14, 2016

jaimeperez commented Dec 14, 2016

robrichards commented Dec 14, 2016

jaimeperez commented Dec 14, 2016

robrichards commented Dec 14, 2016

jaimeperez commented Dec 14, 2016 •

edited

Loading

skymeyer commented Dec 14, 2016

skymeyer commented Dec 15, 2016

skymeyer Dec 15, 2016 •

edited

Loading

robrichards commented Feb 13, 2017

Hikariii commented Feb 14, 2017

robrichards commented Mar 9, 2017

skymeyer commented Mar 9, 2017

skymeyer commented Mar 29, 2017

robrichards commented May 25, 2017

Remove mcrypt as a dependency #101

Remove mcrypt as a dependency #101

Conversation

skymeyer commented Apr 13, 2016 • edited Loading

skymeyer commented May 7, 2016

lt commented Sep 2, 2016

restena-sw commented Oct 20, 2016

lt commented Oct 31, 2016

robrichards commented Oct 31, 2016

jaimeperez commented Dec 5, 2016

lt commented Dec 12, 2016 • edited Loading

codecov-io commented Dec 14, 2016 • edited Loading

Current coverage is 28.40% (diff: 77.65%)

skymeyer commented Dec 14, 2016

robrichards commented Dec 14, 2016

jaimeperez commented Dec 14, 2016

robrichards commented Dec 14, 2016

jaimeperez commented Dec 14, 2016

robrichards commented Dec 14, 2016

jaimeperez commented Dec 14, 2016 • edited Loading

skymeyer commented Dec 14, 2016

skymeyer commented Dec 15, 2016

skymeyer Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

robrichards commented Feb 13, 2017

Hikariii commented Feb 14, 2017

robrichards commented Mar 9, 2017

skymeyer commented Mar 9, 2017

skymeyer commented Mar 29, 2017

robrichards commented May 25, 2017

skymeyer commented Apr 13, 2016 •

edited

Loading

lt commented Dec 12, 2016 •

edited

Loading

codecov-io commented Dec 14, 2016 •

edited

Loading

jaimeperez commented Dec 14, 2016 •

edited

Loading

skymeyer Dec 15, 2016 •

edited

Loading