[SR] Switch to using random_bytes() where available (built into php7)

[klonos]

This issue is a spin-off of a discussion that started in #5812, and a task to crossport https://www.drupal.org/project/drupal/issues/2550519 that's planned for D7.

Problem/Motivation

Drupal prefers openssl_random_pseudo_bytes() if available in Crypt::randomBytes() in 8.0.x or drupal_random_bytes() in 7.x and 6.x.

PHP used the wrong method in the openssl library now fixed in 5.6.12, 5.5.28, 5.4.44 see: https://bugs.php.net/bug.php?id=70014 but it is NOT classified as a security hole so backports are to older (ie distro) versions are less likely. Just because PHP didn't declare this a security hole doesn't mean it is not. But also read on for other scenarios where this class creates a security hole.

The returned pseudo-random bytes were NOT necessarily cryptographically secure.

Proposed resolution

Add random_compat v2.0.2 to core + use it for Crypt::randomBytes()
Adding v2 should be ok here because in drupal 7.x this includes a fallback.
(so not a concern like in #2763787: Upgrade random_compat to latest version which reverted the upgrade to v2.0.2 in drupal 8.2.x and 8.3.x)

[kiamlaluno]

There are two "issues" I could see in using paragonie/random_compat.

• They only accepts issues for supported PHP versions, which means that a PHP-5.6-only security issue won't be fixed
• They avoid using openssl_random_pseudo_bytes(), but they use mcrypt_create_iv() which seems more problematic, IMO

I would change the backdrop_random_bytes() code, for example adding the code to call PHP 7 random_bytes() when available and putting the code that verifies /dev/urandom is readable before the code that verifies openssl_random_pseudo_bytes() is available.

[indigoxela]

... adding the code to call PHP 7 random_bytes() when available and putting the code that verifies /dev/urandom is readable before the code that verifies openssl_random_pseudo_bytes() is available

Both these suggestions seem reasonable to me. 👍

• On newer PHP (function random_bytes() exists) we should really use that. Why re-inventing the wheel or use extra libraries, when not necessary?
• On older PHP it might be that openssl is also outdated

Looking at our (still imperfect) telemetry shows that the vast majority would benefit. And for the minority of php 5 users, nothing would change.

[indigoxela]

A PR is available for review. Testing... hmmm... I wouldn't know how to test that.

[kiamlaluno]

The PR changes the code as expected: First it tries to use random_bytes(), then it tries reading from /dev/urandom; finally, it tries to use openssl_random_pseudo_bytes().

[klonos]

Looking at our (still imperfect) telemetry shows that the vast majority would benefit. And for the minority of php 5 users, nothing would change.

Agreed 👍🏼 ...that's why we've introduced Telemetry for: to allow us to make the hard decisions while at the same time backing them with the appropriate data. At the time of writing, only 3% of sites report using php5.

[klonos]

...code looks good indeed 👍🏼

Thanks @indigoxela 🙏🏼

[quicksketch]

I don't see this as having any backwards-compatibility impact, since backdrop_random_bytes() is always going to return supposedly random bytes. I've merged backdrop/backdrop#4237 into 1.x and 1.23.x. Thanks @indigoxela, @kiamlaluno, and @klonos!