Crypt::randomBytes()/drupal_random_bytes() doesn't actually return cryptographically secure random bytes

I wrote a patch which should ensure that stronger random bytes are generated.

This patch doesn't regenerate the secret key. I am unsure if doing so would cause problems with existing sites.

Is it worth checking if the system is running 5.4 if Drupal 8 minimum version is 5.5?

It occurred to me that we may want to check to see if random_bytes() is available. If it is, then we don't need to use openssl_random_pseudo_bytes(). It is a PHP 7 feature, though I assume is a better and more efficient source of random bytes.

http://php.net/manual/en/function.random-bytes.php

I have made the following changes:

• Don't check for PHP 5.4. Anyone trying to run this code has more pressing issues than the FALSE return value.
• Use the PHP 7 random_bytes() function if available

I am attaching another patch and an interdiff.

Disclaimer: I do not have PHP 7 installed on the computer I am using. The QA test bot is running PHP 5.5 and won't be able to test the PHP 7 code. I believe the code is correct and welcome anyone running PHP 7 to confirm that this is working as expected.

Not sure whether we need the PHP 7 part in the issue. #2540694: Deprecate Crypt::randomBytes() in favor of PHP 7 polyfll is an issue about some form of replacement for it.

In my previous comment I accidentally used a .txt file extention for the patch. Reuploading for the QA bot to test.

Edit: I inadvertently got the filename wrong again (comment number) due to the comment from dawehner at the same time. dawehner, I believe my patch implements one of the proposed solutions of that issue. If the random_bytes() function is available, Crypt::randomBytes() will essentially be a wrapper for it.

Edit: I inadvertently got the filename wrong again (comment number) due to the comment from dawehner at the same time. dawehner, I believe my patch implements one of the proposed solutions of that issue. If the random_bytes() function is available, Crypt::randomBytes() will essentially be a wrapper for it.

Well, its still not needed to fix the issue, keep things as reviewable as possible. This additional call doesn't help with that.

Someone argued that the entire fix is kinda pointless, because in case attackers care about that level of security, you have to be on the latest release (or at least the patched one from your distribution) as otherwise you have much bigger issues.

Overall, I like the proposed solution in the issue you linked. I looked over some of the library code but not all of it. At this time I cannot vouch for its security. It seems to do a lot to ensure good random bytes. However I think it is arguably not more efficient than the "additional call" and more difficult to review.

If it is decided to add that library as a dependency, my patch will utilise it. Whether the library is accepted as a core dependency or not, the random_bytes() function should be used if PHP, contrib modules, or website developers make the function available to Drupal.

If you or anyone else want to submit a patch (or possibly overhaul the code), I'd be interested to take a look. I think that the code I proposed is a step in the right direction.

Yeah, well its security hardening, even if we optimize for a case, which is not that likely, see previous comment.

Doesn't matter if it is ready, but AFAIK, security hardening issues aren't critical and there's a separate tag for them?

Good point.

Regenerate the drupal secret key

Also isn't adressed yet.

+++ b/core/lib/Drupal/Component/Utility/Crypt.php
@@ -140,4 +157,39 @@ public static function randomBytesBase64($count = 32) {
+    // PHP Versions that were fixed: 5.6.12, 5.5.28, 5.4.44.

This means that we won't use openssl_random_pseudo_bytes() on distribution versions of PHP that are patched but without a version number increment. I think Debian now follows the minor PHP version numbering normally, Ubuntu LTS still patches (hence current 5.5.9 requirement), but not sure if this would affect anything else, or if it's a problem if so.

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=757342 and http://www.debian.org/security/2014/dsa-3064 is what I was thinking of.

I wrote an update function. I ran "drush state-get system.private_key" before and after the update, and the key had changed. I'm not sure what consequences (if any) there are to doing this. I attached a patch and interdiff.

/**
 * Regenerate system.private_key in case it was generated using a weak algorithm.
 */
function system_update_8004() {
  $state = \Drupal::state();
  $state->delete('system.private_key');
  $key = new \Drupal\Core\PrivateKey($state);
  $key->get();
}

Edit: removed irrelevant remark about backport version numbering.

I'm not sure why those two failures happened, or if my code even caused them. Any ideas?

As far as eliminating OpenSSL, I'm on the fence. With Windows, the code would wind up using the final fallback if random_bytes() isn't available. Ripping it out entirely may be throwing out the baby with the bathwater. I do agree that we should prefer /dev/urandom over openssl. However Windows doesn't have /dev/urandom available to provide entropy.

I think that openssl_pseudo_random_bytes() can provide Windows (and any server using the fallback) a decent source of entropy. The bytes can be thrown into the hashing function along with mt_rand(), $_SERVER, and microtime(). If a Linux distro backports the fix to an earlier version number, openssl bytes would be hashed instead of directly used.

Edit: Removed reference to getmypid().

I made changes based on my previous comment. I think this may get us close to resolving this issue.

In summary:

• /dev/urandom (if available) is preferred over OpenSSL.
• OpenSSL is used if its function is defined.
• If the bytes from openssl_random_pseudo_bytes() aren't considered cryptographically strong, they are recycled in the fallback case's hashing function.
• static $bytes is also now hashed as part of the fallback case. (at worst it is NULL and nothing changes)
• checkOpenssl() has been renamed to checkOpensslStrength() and simplified.

I haven't changed the code to regenerate the private key. Perhaps it will pass the QA test cases this time around?

> If a Linux distro backports the fix to an earlier version number, openssl bytes would be hashed instead of directly used.

that's very unlikely. As I linked above, Debian (which did such porting previously) is now more up to date with the releases instead of backporting, they are now up to 5.4.41 and I presume as security dictates they will update further. This OpenSSL bug was not marked as a security bug and if there's any other distro left that backports (which?) they most probably won't do this one -- in the past such backporting was done for security only.

Response to #28:

> /dev/urandom (if available) is preferred over OpenSSL.

This makes drupal_random_bytes insecure on Windows, when another process writes a /dev/urandom file with known content.

> If the bytes from openssl_random_pseudo_bytes() aren't considered cryptographically strong, they are recycled in the fallback case's hashing function.

Is the fallback cryptographically strong? If not, is it better, equal or worse than the result from openssl_random_pseudo_bytes? If equal or worse, do we need to change anything here?

As to #31: We should look at the php7 shiv code which sets the buffered read of /dev/urandom to 8 bytes [instead] of reading a large block.

What is the rationale for changing this and the number of bytes requested? Crypt::randomBytesBase64 defaults to 32 bytes.

@pwolanin Can we use random_bytes() on PHP7? And if so, can we make Drupal forwards-compatible?

Given there was no CVE for the PHP release, I don't think this is critical. Looks like straight hardening at most.

We should just pull in https://packagist.org/packages/paragonie/random_compat, and always call random_bytes().

Opened https://github.com/paragonie/random_compat/issues/49 to discuss. The maintainers of random_compat are very responsive. I've landed a couple PRs in the last day or two.

This may be a stupid question, but if you try to file_put_contents('/dev/urandom', 'something'); on Windows, does it actually work? Seems to me that if PHP doesn't complain loudly about that, it's a PHP bug.

Okay, looks like random_compat had already mitigated that problem, but there's now an explicit condition for Windows hosts. See https://github.com/paragonie/random_compat/issues/49#issuecomment-147579107

Let's give this a try. This is a straight conversion to random_compat, now that Heine's concern is resolved upstream (the library won't even try to read from /dev/urandom if it's running on Windows).

+++ b/vendor/paragonie/random_compat/lib/random_int.php
--- /dev/null
+++ b/vendor/paragonie/random_compat/tests/full/DieHardTest.php

We have to exclude tests folder after #2585165: Don't include vendor test code (especially mink) in the Drupal webroot.

Ah, right. Okay, here's a new one.

Reroll for changes in head.

@chx was asking about why I had the \ in front of random_bytes(). I seem to remember it being something about differences in behavior between php 5.5 and php 7.0 (given that random_bytes() is a built in in 7.0 and a user defined function in 5.5), but I'm not 100% sure, so uploading a patch without the \ to see what happens on 5.5 and 7.0.

Apparently, I was imagining the difference :)

Reviews for #47, plz!

Changing title, bumping to critical, adding information about why that's the case to the IS.

@cweagans pinged me and asked me to take a look at this. Based on the information in the summary, my feeling is that the issue is major, but I'd defer to the security team on the priority.

quoting Damien Tournoud: "Note: RAND_pseudo_bytes and RAND_bytes are *exactly the same thing* on *all systems except the most arcane ones*. Which is why the "fix" in PHP was not marked as a security issue, because except on some weird systems that do not have either /dev/random nor /dev/urandom, it is not, actually, a security issue."

Here's a reroll and the tests directory properly excluded by the script.

@klausi, the openssl docs disagree https://www.openssl.org/docs/manmaster/crypto/RAND_bytes.html (see second paragraph under "Description"), and so does the code (https://github.com/openssl/openssl/blob/master/crypto/rand/rand_lib.c#L158 vs https://github.com/openssl/openssl/blob/master/crypto/rand/rand_lib.c#L167 + https://github.com/openssl/openssl/blob/master/crypto/rand/md_rand.c#L610). I may be misinterpreting that, though, because my C is very rusty.

For the sake of argument, however, let's assume that rand_pseudo_bytes() and rand_bytes() do the same thing except for a small percentage of systems. Are we really okay with putting sites running on those systems in harms way? It'd be one thing if this were some weird unicode thing or something, but for this? These things that use random data from this class with the expectation that the output is going to be unpredictable:

* Form tokens
* CSRF tokens
* Config directories
* Password salts

If we cannot guarantee that this class will output cryptographically secure random data in all cases, we shouldn't make that claim, and if we can't make that claim, then we can't rely on it for the things that we're using it for.

Even if the openssl issue turns out to be mitigated in part or in full, there are still two other broken paths in Crypt::randomBytes() for PHP installations without ext_openssl: /dev/urandom without any checking that it's actually the right thing, and the userspace "random" data generator (which literally uses nothing suitably random).

Attacking a site via creating a file called c:\dev\urandom is just devious and exceptionally elegant... even if the feasibility of it is questionable.

Patch does not apply anymore.

I agree that the patch is a good improvement, I just don't agree on the critical priority.

For the sake of argument, however, let's assume that rand_pseudo_bytes() and rand_bytes() do the same thing except for a small percentage of systems. Are we really okay with putting sites running on those systems in harms way?

No, which is why this is a major bug that we'll fix as soon as it's ready. However it's security hardening for a theoretical vulnerability, not an actual exploitable vulnerability - which is why it's a major bug and public, rather than critical and in the private tracker.

So downgrading back to major, but tagging RC target, could also be fixed in 8.0.1 if it doesn't get into 8.0.0.

Rerolled

@klausi, Can you please elaborate on why you don't agree with it being critical?

@catch, the ext_openssl issue is not the only thing here - are you saying that all of the reasons given in the "Issue priority" section in the RC phase evaluation in the issue summary are not valid reasons for making this Critical?

@alexpott, thanks for rerolling this :)

@catch - also, for the record, I did open a security issue about this, but since the public issue predated the security issue, I was told to work on it here. This vulnerability *is* exploitable on Windows if an attacker can also write a file to C:\dev\urandom.

@cweagans: this issue is not critical because there is no direct security vulnerability, no data loss, no unusable system.

I reviewed the patch and everything makes sense to me. The additional random.php file used for auto loading on every single page request only does a couple of file inclusions and function_exists() calls, so this should only have a tiny performance impact.

hiding obsolete patch files.

Ha, spoken too soon. Executing ab -n 500 -c 4 http://drupal-8.localhost/node/3 (with page_cache module enabled) multiple times, consistently getting the same results:

Core 8.0.x (without patch): 19ms time per request (median)
Patch: 21ms time per request (median)

That would indicate a 10% performance regression for cached pages. Can anyone reproduce those results?

EDIT: PHP version 5.6.4-4ubuntu6.4

Is there any reason why this external library cannot use classes?

With Xdebug disabled, again ab -n 500 -c 4 http://drupal-8.localhost/node/3, page_cache enabled:

Drupal core: 16 ms time per request (median)
Patch: 17 ms time per request (median)

The random_compat library uses /dev/urandom on my system while core uses openssl_random_pseudo_bytes(), which seems to be faster. Still a difference of 6% in performance for this average page request to /node/3.

Testing autoload performance: reverting the hunks in Crypt.php so that only the new autoload code of the patch runs I get the same 16 ms time per request (median) as without the patch.

So it seems the performance regression in the patch is the usage of /dev/urandom now. If I use core and modify Crypt::randomBytes() so that openssl_random_pseudo_bytes() is not used, then I see the same 17 ms time per request (median) because it falls back to /dev/urandom.

Are we comfortable with sacrificing 1 ms on every Drupal page request just because openssl_random_pseudo_bytes() can somtimes be insecure on old PHP versions?

I think we could potentially not do the Crypt::randomBytes() call in Kernel::boot(), and instead re-seed it on demand (and document you shouldn't use mt_rand() directly) to skip the 1ms regression. Need to find the issue that was added.

The mt_srand() seeding was introduced in #2140433: Port SA-CORE-2013-003 to Drupal 8.

The mt_srand() call does not make sense to me, filed #2617208: Remove mt_srand() seeding because of performance.

@dawehner: I don't know that it's actually been discussed, but I'm not sure how much sense it makes for random_compat because it's just a polyfill for two functions.

As I said in https://www.drupal.org/node/2617208#comment-10577338, I think we can just get rid of mt_srand() here since this patch is also removing cryptographic use of mt_rand().

OK, here is a patch that removes the mt_srand() call from DrupalKernel.

The page cache performance tests are now equal to 8.0.x, so the removal from the Kernel bootstrapping definitely helped.

I disabled page_cache and dynamic_page_cache to measure the performance impact on an uncached page, same test with ab -n 500 -c 4 http://drupal-8.localhost/node/3:

Executed 5 times, median time per request:
core: 160,160,159,160,160 ms => median of medians: 160ms
patch: 163,165,161,162,162 ms => median of medians: 162ms

Which means we have a performance regression of about 2ms = 1% on an uncached page.

Random test fail, passes now.

I did some more benchmarking on a more powerful computer with the same ab -n 500 -c 4 http://drupal-8.localhost/node/3 test (page_cache and dynamic page cache disabled):

PHP 5.6.11-1ubuntu3.1, medians of 5 test runs:
core: 51,51,51,51,51 ms
patch: 51,52,51,51,51 ms

PHP 7.x master as of 2015-11-20, medians of 5 test runs:
core: 32,32,32,32,32 ms
patch: 31,32,32,32,32 ms

As we can see there is no significant performance difference on this typical Drupal request to an uncached node page.

Taking a closer look at Crypt::randomBytes() in isolation with this benchmark script which calls the static method 1 million times:

require_once 'autoload.php';

for ($i = 0; $i < 1000000; $i++) {
  \Drupal\Component\Utility\Crypt::randomBytes(55);
}

Testing with time php benchmark.php:

PHP 5.6.11-1ubuntu3.1, 5 test runs:
core: 2287,2272,2192,2291,2368 ms
patch: 4624,4600,4730,4612,4603 ms

PHP 7.x master as of 2015-11-20, 5 test runs:
core: 1590,1573,1549,1707,1545 ms
patch: 4016,3980,3971,4020,4004 ms

Quite disappointing that random_bytes() in PHP 7 performs so much worse than openssl_random_pseudo_bytes(), 2.5 times slower.

Of course this is a very artificial benchmark since the typical Drupal use case will never invoke Crypt::randomBytes() one million times. As the first ab benchmark shows a performance regression of 1% is only noticeable on older hardware (and shared hosting probably).

There is FUD flying around that openssl_random_pseudo_bytes() has a weakness https://twitter.com/ircmaxell/status/666667962959532032 , but I have not found hard evidence yet. I'm torn between the superior performance of openssl_random_pseudo_bytes() over random_bytes() and the advantage of getting rid of the code so that we don't have to maintain it anymore.

Hm, this does not have to be an all or nothing issue. We can pick the random_compat library, remove the scary parts we all agree that should be removed and keep openssl_random_pseudo_bytes() for performance reasons. We can see this issue as a first step in the right direction until we know more and rather have small improvements instead of all at once.

We have here:
1) adding random_compat library, no measurable performance impact of adding it to the autoloader.
2) keeping openssl_random_pseudo_bytes() for performance reasons
3) using random_bytes() as a secure fallback, thereby removing our own custom PRNG fallback.

As we can see there is no significant performance difference on this typical Drupal request to an uncached node page.

There's no performance hit, we get rid of code that we have to maintain, and the code we replace it with uses other/better sources of randomness before openssl. Why would we want to use openssl_random_pseudo_bytes() before that?

As you say, that's a very artificial benchmark, and is not indicative of real-world use.

IMO, we should not do anything except call random_bytes() here.

There is a performance hit for slower hardware (as demonstrated) and as soon as any contrib module gets random bytes somewhere in the critical performance path. We want to use openssl_random_pseudo_bytes() because it is faster.

So we don't agree whether using openssl_random_pseudo_bytes() is worth it or not.

In the meantime I'm offering a patch that removes the other crap where we absolutely agree that it should go. Can we do the latest patch as a compromise until we know more?

There is a performance hit for slower hardware (as demonstrated) and as soon as any contrib module gets random bytes somewhere in the critical performance path. We want to use openssl_random_pseudo_bytes() because it is faster.

That's what I'm saying though: There isn't a real-world performance hit, and contrib modules that are getting random bytes in a critical path are doing it wrong.

The performance test that you did was a contrived example. Nobody is ever going to be getting 55 random bytes 1,000,000 times in a row. It's extremely likely that they'll be getting < 128 bytes a small handful of times, and for that, there's no observable difference, and you said that yourself:

PHP 5.6.11-1ubuntu3.1, medians of 5 test runs:
core: 51,51,51,51,51 ms
patch: 51,52,51,51,51 ms

PHP 7.x master as of 2015-11-20, medians of 5 test runs:
core: 32,32,32,32,32 ms
patch: 31,32,32,32,32 ms

As we can see there is no significant performance difference on this typical Drupal request to an uncached node page.

This method needs to return cryptographically secure pseudorandom bytes in all circumstances - no matter what OS, version of PHP, or nonstandard configuration is in use. If doing that is too slow for whatever reason, then we need to evaluate whether or not we can actually provide this functionality, and since this functionality backs a lot of our other security components, that's not a good situation to be in.

We should not sacrifice security for performance. In this case, there's only a negligible performance gain by adding back openssl_random_pseudo_bytes(), and it's just not worth it in my opinion.

If it's absolutely necessary, I'd be okay with adding openssl_random_pseudo_bytes() back in a way that a user has to specifically opt-in to using it before random_bytes() (i.e. it should be off by default). However, I think we should use random_bytes() and then open a followup to determine if openssl_random_pseudo_bytes() is really worth the performance gain in all scenarios (and there are a *lot* of them - Windows wasn't considered here at all, nor were systems that are using ext_libsodium). I suspect that it's not, but I'd want to verify that in a test that reflects real-world use (and not just the raw speed of execution of each method 1,000,000 times).

We don't know the use cases of contrib modules and using random bytes in the critical performance path might be something they want to do - so this is not necessarily wrong.

I'm still missing evidence here that openssl_random_pseudo_bytes() is insecure (even in the older PHP versions). I agree with Damien Tournoud that we just have FUD about it right now. If we cannot come up with an attack scenario that describes on what system and what configuration openssl_random_pseudo_bytes() is exploitable we should stick with it.

Talked to alexpott about this and he is in favor of getting as much custom crypto code removed here as we can, off-loading this to random_compat and PHP 7 core. The performance loss for that is small enough.

We have eliminated the worst performance offender in DrupalKernel in this patch, so I guess I can live with random_bytes() only for the sake of making progress here.

Patch attached that uses random_bytes() only and is the same as in #76.

@klausi, Sorry I missed you in IRC over the weekend. There are details about the problem here: https://github.com/paragonie/random_compat/issues/6#issuecomment-119564973

+1 to #85

OK, me + cweagans + alexpott agree, no objections, enough for RTBC.

I suspected that the extra requires random_compat does was causing the slowdown on the "slow" test. Klausi confirmed there was no opcache on that instance which is good enough for me to write off my concerns.

+1 RTBC.

+            "name": "paragonie/random_compat",
+            "version": "1.1.0",

There's a newer version of the library available (1.1.1) - should the patch use that instead?

yes, will do a reroll.

Rerolled with composer update paragonie/random_compat.

+1 to RTBC on this. Thanks for moving this forward Klausi. I've been stupidly busy lately. I went ahead and opened the followup I mentioned earlier: https://www.drupal.org/node/2629926

While we're waiting on this to land in 8.0.x and/or 8.1.x, we should figure out how we might backport this to D7 and/or D6 (per the tags on this issue). Pulling in a Composer lib in D8 is reasonably straightforward, but I don't know that we have a reasonable way to do that in D7 or D6. Any ideas here?

klausi, looks like someone else saw the performance issue you were seeing (I could not reproduce). https://github.com/paragonie/random_compat/issues/78 The fix for that is included in 1.1.1. Does that change resolve the issue satisfactorily?

@cweagans so we should update the patch - no?

@alexpott, klausi already rerolled the patch with 1.1.1 in #91. Just wanted to check to see if the new release resolved the issue he was seeing. Still RTBC from my perspective.

phantomjs collision. Conflicts in composer.lock and installed.json.

Rebased, reset files to HEAD version and rebuilt with composer.phar update --lock

LGTM. Back to RTBC.

Committed 5106176 and pushed to 8.0.x and 8.1.x. Thanks!

I committed this to 8.0.x because afacis this patch does not add any new functionality to Drupal 8 and therefore is only a bug fix and patch eligible. I think in most cases adding a new library would be a minor version activity.

Commits in #102 & #103 doesn't looks correct.
Function random_bytes() available only in PHP 7.
But according to this page https://www.drupal.org/requirements PHP 5.5.9 is enough for Drupal 8.

I use PHP 5.5.9 and I have Fatal error :(

Fatal error: Call to undefined function Drupal\Component\Utility\random_bytes() in /../test8/core/lib/Drupal/Component/Utility/Crypt.php on line 36

@eugene.ilyin any chance you can provide a full backtrace? The patch adds a polyfil library for older versions of PHP and the patches are tested on PHP5.5

@alexpott, I've added xdebug backtrace in the attached archive. Is it enough?
Also for information, I've installed composer manager and I've updated my libraries via "composer drupal-update"
Maybe something went wrong...

@eugene.ilyin yeah you seem to be missing vendor/paragonie/random_compat - if you run composer install --dry-run in the drupal root what happens? And please ensure that composer is up-to-date.

@alexpott and @bojanz helped me in IRC. Trouble was in my personal state of directory "vendor".

How I've fixed it:
1. I've executed "composer install --dry-run" in root directory
2. After that I've tried to execute "composer drupal-update" but I had an error
3. I've turned back the directory "vendor" to clean state (as in drupal core)
4. I've executed "composer drupal-update" again and my trouble has gone

After discussing with @catch we've decided to revert this patch because adding dependencies is proving to be tricky. For example if you following the instructions to update only the /core directory and then run composer install in the drupal root directory nothing happens - it does not get the new paragonie/random_compat. Therefore, it seems sensible to give ourselves some more time to work out how to give the best advice on updating core dependencies.

Just to clarify this is reverted against 8.0.x, the 8.1.x patch is still in as is.

@alexpott @catch So should this work be cleaned up for 8.0.x? Or should we just have users wait to update to 8.1.0 to get this improvement? I'm not really sure why this was rolled back though. If you're using Composer + a subtree split of /core, you'll have to wait for the subtree repo to be updated + wait for Composer to generate new package metadata that includes random_compat as a dependency (which would happen when 8.0.2 is tagged). I'm fairly certain that running on head + composer_manager won't pick up new dependencies until that happens.

@cweagans here is why... https://github.com/wikimedia/composer-merge-plugin/issues/96 - basically we're breaking the promise of update ./core and you're good to go. And there is no composer command to help either - apart from composer update and that is going to do way more than just install the dependency.

@cweagans for me I'd just have this wait until 8.1.x - especially given the security improvement is as far as we know theoretical.

#116 is interesting though, if tagging the release made it all work again, but not sure it's worth waiting to find out. We'd probably need an 8.1.0-beta1 to test that successuflly without impacting real sites.

Eh. Let's just wait for now. People that really want the change can apply the patch to 8.0.x, and they'll likely know how to deal with the composer issues too.

So how should we go about getting this into D7? Just download random_compat, shove it into includes, and require() it from bootstrap.inc? The other changes are pretty straightforward - just want to know what's going to be acceptable in terms of getting the lib loaded.

Up to David in the end, but #119 is about what I'd expect.

So how should we go about getting this into D7? Just download random_compat, shove it into includes, and require() it from bootstrap.inc?

I guess, though it does seem a bit wasteful to load all that code when it might not be used.

Another way (which I think may have made sense for D8 too) is to load it from within drupal_random_bytes() the first time that function is called?

A couple other questions I had just now skimming through the D8 patch (I guess they apply to both D7 and D8):

1. +        if (!function_exists('random_bytes')) {
   +            /**
   +             * We don't have any more options, so let's throw an exception right now
   +             * and hope the developer won't let it fail silently.
   +             */
   +            function random_bytes()
   +            {
   +                throw new Exception(
   +                    'There is no suitable CSPRNG installed on your system'
   +                );
   +            }
   +        }
   

   Do we know that this condition won't ever be reached in Drupal? Because we aren't doing anything to handle it...

   Currently in drupal_random_bytes() we always return something and never throw an exception.

2. This is a new library so should we be listing it in Drupal's COPYRIGHT.txt file?

The argument made on PHP Internals was that if you ask for cryptographically secure random bytes, and those cannot be provided, then the only secure thing to do is bail out. Proceeding with an insecure string (or worse, false aka empty string) is a security hole. That is, if you hit that exception your site's environment is not safe to use in the first place, and a fatal uncaught exception is appropriate. (Or, alternatively, we could see about translating it to a nicer-looking fatal error message.)

For Drupal 7, we could wrap the include statement in a version_compare() check as it's only needed < PHP 7.0.0. But really, that's what the if (function_exists()) stuff is doing already so I wouldn't worry about it too much in D7 or D8.

Edited: Corrected the direction of the carrot

I guess, though it does seem a bit wasteful to load all that code when it might not be used.

Another way (which I think may have made sense for D8 too) is to load it from within drupal_random_bytes() the first time that function is called?

It's not a lot of code all things considered - likely not much more than what drupal_random_bytes() already contains.

I *think* this code is going to get loaded on most page requests anyway, but I have no objection to loading it on the first drupal_random_bytes() call.

Do we know that this condition won't ever be reached in Drupal? Because we aren't doing anything to handle it...

It's very rare that a system would be so thin on sources of randomness. IIRC, when I did some digging on phpinfo for a bunch of shared hosts a couple years ago for something else, openssl was enabled 100% of the time, even on the most obscure of hosts, so that would be the most likely worst case scenario.

I think this is a missunderstanding. @eugene.ilyin could you provide the steps you did to upgrade? My guess from Alex's upstream report is that composer.lock was maybe not updated? http://cgit.drupalcode.org/drupal/tree/core/UPGRADE.txt#n67 requires all files be updated. If you don't follow that process things will probably go wrong.

Additionally from the discussion people might be looking for composer.phar update --lock which will pull in things from composer.json that are missing from composer.lock. This is a pretty important command for people using composer_manager or similar contrib solutions for managing /vendor.

It's not a lot of code all things considered - likely not much more than what drupal_random_bytes() already contains.

I *think* this code is going to get loaded on most page requests anyway

Yeah, I think that part is probably fine as is.

It's very rare that a system would be so thin on sources of randomness. IIRC, when I did some digging on phpinfo for a bunch of shared hosts a couple years ago for something else, openssl was enabled 100% of the time, even on the most obscure of hosts, so that would be the most likely worst case scenario.

I guess what I want to avoid is throwing a runtime exception on live sites... The library does appear to throw exceptions in a couple different places under a couple different scenarios.

So if we're adding some new requirements to Drupal here that didn't exist before, it should at least be in hook_requirements() somewhere. And we should think a bit how it should be handled on upgrade.

One possible solution would be to keep our old code around, but only use it as a fallback in the case where the library throws an exception. And when a site is in that situation, throw a REQUIREMENT_ERROR in hook_requirements().

This is based on the idea that our current drupal_random_bytes() is not a complete security disaster, even if it's not perfect either (i.e. that a site can still operate with a reasonable level of security in that case, even though the security could be improved).

I had intended to leave a related issue with the above comment, but it got eaten.

One possible solution would be to keep our old code around, but only use it as a fallback in the case where the library throws an exception.

I strongly object to this. The polyfill only throws an exception when random bytes cannot be generated for whatever reason (usually, it's a lack of a trustworthy source of randomness). If we're saying that drupal_random_bytes() is going to return cryptographically secure random bytes, then it should either do that or throw an exception when it can't. An exception is (in this case) the least undesirable thing, and the most undesirable thing is that we return more predictable random bytes silently.

All of that said, it's very rare that an environment won't have at least one of the polyfill methods available. I think every shared hosting company that I've looked at has at least two options (usually mcrypt and openssl, but some windows hosts have the COM extension enabled too). I don't think this is something to worry about.

A hook_requirements() entry, however, might be a good idea. Just ensure that the host that Drupal is running on has at least ONE of the handfull of polyfill methods available.

Just bumping this as this would be good to get into 7.x

Here is a first D7 patch. I did not include a hook_requirements() check because drupal_random_bytes() is used so early. I agree with @cweagans that a site should fail fast if there is no source of secure random bytes.

Thanks!

Removed some build scripts in random_compat that we really should not ship with Drupal.

Why are we going to use version 2 of paragonie's random_compat library for D7 and version 1 for D8. Should we upgrade the library for D8?

We should use version 2 in both, the issue to upgrade it in D8 is #2763787: Upgrade random_compat to latest version.

Right, like I said above, I really don't like the idea of whitescreening someone's production site just because our random number generator doesn't reach a level of security that we never claimed it had in the first place...

I think allowing the fallback to be used, but putting something about it in hook_requirements() (perhaps a REQUIREMENT_ERROR on runtime and a REQUIREMENT_WARNING on install?) would really be good enough.

Oops, cross-post (had the issue open in a browser tab for a while). Not entirely sure what "affecting production sites" means in this context though...?

- CNW for the try {} catch

Just got a report that on some site this generated the same Session ID twice in a day ...

Right, like I said above, I really don't like the idea of whitescreening someone's production site just because our random number generator doesn't reach a level of security that we never claimed it had in the first place...

By naming it Crypt::randomBytes, you're implicitly claiming it will be cryptographically secure. Using random_compat v1 will guarantee that you have a CSPRNG in most installations, but its fallback to OpenSSL will cause problems like https://github.com/ramsey/uuid/issues/80.

Using v2 guarantees a level of security comparable to what PHP 7 offers (and is, therefore, a true polyfill).

My recommendation is to set the version requirement to ^1|^2 so people can choose whichever one works best for their plugins/extensions.

(In case it wasn't clear: I work for Paragon Initiative Enterprises, whose namespace random_compat is published under.)

By naming it Crypt::randomBytes, you're implicitly claiming it will be cryptographically secure.

The name of the function in Drupal 7 is drupal_random_bytes(), not Crypt::randomBytes(), and the documentation at https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drup... only claims that it is "highly randomized" and "uses the best available pseudo-random source", nothing more than that.

But yes, it is confusing that the issue title here dates from the Drupal 8 version of the patch :)

Just got a report that on some site this generated the same Session ID twice in a day

fallback to OpenSSL will cause problems like https://github.com/ramsey/uuid/issues/80.

So we'd probably be better off never using OpenSSL at all, to be safe?

In any case, there is definitely no need to keep the OpenSSL code from the current version of drupal_random_bytes() - I assume the only thing we'd need to keep from that is the final fallback, i.e. the part from here onward:

  // If we couldn't get enough entropy, this simple hash-based PRNG will
  // generate a good set of pseudo-random bytes on any system.

Since our goal is to hand this off to random_bytes() whenever possible, but just keep a single guaranteed backup method in place to prevent whitescreening the site if all else fails.

Ah, we forgot to remove the performance critical mt_srand() seeding during Drupal boostrap as we did for Drupal 8.

Still no exception handling or hook_requirements(), feel free to pick that up.

Forgot interdiff.

Yes, getting random bytes is slow on php5, see analysis in #71.

1. +++ b/includes/bootstrap.inc
   @@ -2242,7 +2242,42 @@ function drupal_base64_encode($string) {
   +  catch (\Exception $e) {
   

   This will fail on PHP 5.2 because it does not understand the "\" namespace operator.

2. +++ b/includes/bootstrap.inc
   @@ -2242,7 +2242,42 @@ function drupal_base64_encode($string) {
   +      // Initialize on the first call. The contents of $_SERVER includes a mix of
   +      // user-specific and system information that varies a little with each page.
   

   line length now over 80 characters.

Looks good! Now we also need that try/catch block in system_requirements(). It should show an error on runtime and install time when random_bytes() throws an exception, right?

BTW I'm still in favor of not catching the exception, but I guess any progress is better than nothing?

We are running patch #144 on our production sites successfully now.

This will fail on PHP 5.2 because it does not understand the "\" namespace operator.

https://github.com/joomla/joomla-cms/blob/91749f845dcf20e097801ecd75da8e...

@sarciszewski: we are talking about the old stable version of Drupal here, which is Drupal 7 and was released in 2011. It's minimum requirement is PHP 5.2 and we are not breaking that compatibility just for fun.

Drupal 8 of course requires PHP 5.5 :)

@pwolanin

The reason why your patch #156 failed:
+ require_once DRUPAL_ROOT . '/includes/random_compat/lib/random.php';

random_compat and this library was not included in the patch, therefore travis ci will fail during testing.. Try making a new patch that adds this random_compat/lib/random.php

To do this you will git add this and commit it in your local with the other changes , then to generate the patch you will do a git diff of the resulting commit hash tag vs the previous hashtag.

19:43:52 error: patch failed: scripts/run-tests.sh:142
19:43:52 error: scripts/run-tests.sh: patch does not apply

This is pretty easy to generate a new patch, what version of this random.php library are you wanting to include?

@joseph.olstad: the latest patch already contains the library in version 2.0.2, so there is no need to roll a new patch.

Queuing the patch for testing again.

Testbot says its good after a kick. So yes the patch was fine after all. Blame it on the testbot.

+    'title' => $t('Random byte generation'),
...
+    $requirements['php_random_bytes']['description'] = $t('Drupal could not generate secure random bytes and is using a slow, less-secure fallback generator. %exception_message', array('%exception_message' => $e->getMessage()));
+    $requirements['php_random_bytes']['value'] = $t('Failed to fallback random generator');
+    $requirements['php_random_bytes']['severity'] = REQUIREMENT_ERROR;

This could use some kind of help text (or, probably better, a link) explaining how to fix the problem.

Wording could be a bit less technical also, maybe like:
Title: "Random number generation"
Summary: "Less secure"
Description: "The server is unable to generate highly randomized numbers, which means certain security features like password reset URLs are not as secure as they should be. This site is using a slow, less-secure fallback generator instead..." ... something like that.

Do we really want this to be a REQUIREMENT_ERROR even on install/update? Won't that prevent Drupal from being installed at all?

I think REQUIREMENT_ERROR is fine. You should really fix your server setup first when there is no source of secure random numbers at all. We only want to permit usage of our weak random number generator for existing sites so that they don't white screen on an update.

Which means we should not run this check in the "update" phase to always allow sites being updated.

I'll work on the wording changes asked for in #163

wasn't sure if the "better" to use a link meant making a handbook page, or linking to an existing page. maybe this is text change for now, and the link can be a separate issue?

there is also some whitespace at the end of lines in the patch that we can clean up. not sure if we want that in the 7.x patch, or if it should keep it to match 8.x.

I looked to see if these text changes would mean a change also in a test, and didn't see any.

1.

I asked @pwolanin about only giving the message if not during an update. The requirement will still show up on the status page, it just wont completely die when trying to run update. Seems ok to me.

--

2.

I also asked about no test fail with the change of the exception wording. Seems we could write a test that redefines random_bytes() to force an exception.

See, in bootstrap.inc

function drupal_random_bytes($count)  {
  require_once DRUPAL_ROOT . '/includes/random_compat/lib/random.php';
  try {
    return random_bytes($count);
  }
  catch (Exception $e) {

But, since random_bytes is in php 7, it might cause a fatal in 7, or we would need a condition in the test to only do the test logic if php is less than 7.

if (PHP_VERSION_ID < 70000) {

So adding a test might not be worth it.

Not sure if the CI problem with d7 php7 tests is still an issue. related: #2656548: Fully support PHP 7.0 in Drupal 7

I read the entire (non library part) of the patch.

1.

the white space warning when I did the git apply is from the library we are including, so nothing to change there. see my #166.

2.

+++ b/includes/bootstrap.inc
@@ -2271,8 +2257,8 @@ function drupal_random_bytes($count)  {
-      // Initialize on the first call. The contents of $_SERVER includes a mix of
-      // user-specific and system information that varies a little with each page.
+      // Initialize on the first call. The $_SERVER variable includes user and
+      // system-specific information that varies a little with each page.

There looks like there might be some unrelated nit fixing. (changing comment that is over 80 chars, but not changing the meaning). This hunk should be reverted. https://www.drupal.org/core/scope#creep

I think this happened because of #149 which was looking at an interdiff, which made it looked like those lines were part of a larger change, but when looking at the patch, it is more clear that that section is not being changed.

3.

ha

+++ b/includes/bootstrap.inc
@@ -2230,39 +2230,25 @@ function drupal_base64_encode($string) {
+ * In PHP 7 and up, this uses the built-in PHP function random_bytes().
+ * In older PHP versions, this uses the random_bytes() function provided by
+ * the random_compat library.

nit. wrap to 80.

===

I think the only core gate blocking thing might be 2. the unrelated change. @pwolanin is gonna address both though really soon. so not fixing the nit myself.

thanks! that looks great. expecting the bot to return green. setting to rtbc.

1. +      $requirements['php_random_bytes']['description'] = $t('The server is unable to generate highly randomized numbers, which means certain security features like password reset URLs are not as secure as they should be. Instead, this site is using a slow, less-secure fallback generator. %exception_message', array('%exception_message' => $e->getMessage()));
   

   This still needs help text or a link which explains how to fix the problem, especially if we're going to go ahead and make this a hard requirement for new installs. Right now the exception message (from the library) just says "There is no suitable CSPRNG installed on your system", which is not useful. We need to give people some information about what they need to install on their server to be able to use this. It could be a link to some outside documentation (if that exists) or a page on drupal.org, etc.

2. I wonder if anyone has tested this on a typical Windows setup. The reason I ask is that I just confirmed that on my standard Linux setup, /dev/urandom is the only place that this library can actually get random numbers from... if it didn't do that, it would have failed on my machine. And /dev/urandom is not available on Windows.

3. -  // Ensure mt_rand is reseeded, to prevent random values from one page load
   -  // being exploited to predict random values in subsequent page loads.
   -  $seed = unpack("L", drupal_random_bytes(4));
   -  mt_srand($seed[1]);
   

   Isn't this still needed for the fallback behavior of drupal_random_bytes()? (Perhaps it doesn't need to be in the bootstrap anymore, but seems like it's still necessary somewhere in the case where the fallback is being used.)

4. +      $requirements['php_random_bytes']['value'] = $t('Successful');
   +    } catch (Exception $e) {
   

   Maybe it should be "Secure" rather than "Successful"?

   Also (minor), catch (Exception $e) { should be on its own line.

Also the help text doesn't look quite right for the case where an installation is actually being blocked... it says "this site is using a slow, less-secure fallback generator" but in fact the site isn't using anything because Drupal won't let you install it.

I've removed the exception from the status message and added a link to https://www.drupal.org/docs/7/system-requirements/php#csprng which we can add.

In addition, I discovered that many of our sites will default to the fallback, because they use openbase_dir without /dev/urandom and do not have any other random source such as mcrypt or libsodium. This change probably needs a big note in the release notes.

I've added the suggestions in #179, #180. Not certain about the wording.

I'm looking into adding the mt_rand seed back. Doing some research.

I did not run a full install on Windows, just random_compat on Windows 10 with php-5.6.26-nts-Win32-VC11-x86.zip . random_compat was able to get random bytes.

The 5.6 binaries from windows.php.net all have mcrypt statically linked; this is also what random_compat uses. When I disabled the mcrypt option in random_compat, it threw an exception.

Please don't fall back to mt_rand(), ever.

http://www.openwall.com/php_mt_seed/