mt_rand() should not be used for any cryptographic purposes which is also documented on the PHP docs of mt_rand(). Drupal 8 core only uses mt_rand() as a last resort for generating pseudo random numbers in Crypt::randomBytes(). Therefore it does not make sense to seed the random number generator on every single page request. We found out in #2550519: Crypt::randomBytes()/drupal_random_bytes() doesn't actually return cryptographically secure random bytes that the seeding with random bytes from /dev/urandom can cost up to 1 ms of performance on a page request.
The mt_srand() call was orginally introduced in #2140433: Port SA-CORE-2013-003 to Drupal 8.
Remove the mt_srand() call from DrupalKernel and move it to the last resirt mt_rand() call in Crypt.php.
Patch + review.
none.
none.
none.
Comments
Comment #2
cweagansNote that Crypt::randomBytes() currently calls mt_rand() for generating "randomness". This will go away if #2550519: Crypt::randomBytes()/drupal_random_bytes() doesn't actually return cryptographically secure random bytes lands.
Comment #3
heddnDoes this need a backport too? Tagging for now.
Comment #4
klausi@cweagans: good point, so we should move the seeding there.
Comment #5
cweagans@klausi, We shouldn't be using mt_rand() for crypto. Fixing that alleviates the need for seeding it the way we do if I'm not mistaken. IMO, this can be fixed in #2550519: Crypt::randomBytes()/drupal_random_bytes() doesn't actually return cryptographically secure random bytes since the cryptographic use of mt_rand() is also being removed. The PHP docs for mt_rand() say that there's no need to seed, as it's done automatically, and the only reason that we were seeding it was because we were using it for crypto. We use mt_rand() elsewhere in core, but I think that in those instances, it's okay to just call mt_rand() without doing anything to seed it.
Comment #8
klausiDoes not apply anymore to Drupal 8 because the call was removed already.
Comment #9
heine commentedThe explicit seeding of mt_rand was added because the default seed was relatively easy to discover using a seed cracker and core and many modules used mt_rand at that time. The intent of the seeding was a blanket protection against the seed cracker, without having to go through all of contrib to fix.
At this point in time -after #2550519: Crypt::randomBytes()/drupal_random_bytes() doesn't actually return cryptographically secure random bytes - mt_rand is only used in core 7.x for file upload progress, replica selection and tests and could be replaced.
Some contributed modules such as legal however still use mt_rand for signature purposes. Others, such as uuid and ctools use mt_rand for uuid generation. These would be immediately exposed to the seed cracker when seeding is removed, and have to switch to drupal_random_bytes.