Download & Extend
Issues

Improve random number generation

Posted by Damien Tournoud on June 27, 2010 at 3:39pm
Project:Drupal core
Version:7.x-dev
Component:base system
Category:task
Priority:normal
Assigned:Unassigned
Status:closed (fixed)
Issue tags:needs backport to D7, needs security review, Novice

Issue Summary

I asked Pierre Joye, from the PHP team, to review our random number generation function (drupal_random_bytes()).

He suggested to use openssl_random_pseudo_bytes() if available (that's available since PHP 5.3), as it seems to be the only reliable source of random numbers of Windows for now.

Login or register to post comments

Comments

#1

Posted by Damien Tournoud on June 27, 2010 at 3:48pm
Status:active» needs review

Something like that should do it.

AttachmentSizeStatusTest resultOperations
838800.patch959 bytesIdlePASSED: [[SimpleTest]]: [MySQL] 21,125 pass(es).View details

#2

Posted by akamustang on December 8, 2011 at 5:24pm
Version:7.x-dev» 8.x-dev

Can't see a reason not to include openssl_random_pseudo_bytes() for a better system agnostic solution

#3

Posted by Dries on December 9, 2011 at 3:43pm
Status:needs review» reviewed & tested by the community

This looks RTBC to me.

It could be good to extend the code comment so it mentions that it is the only reliable source of random numbers on Microsoft Windows.

#4

Posted by xjm on December 9, 2011 at 5:05pm
Issue tags:+needs backport to D7

Note that the patch needs a reroll for core/. If we're doing function_exists(), we can also backport this.

#5

Posted by droplet on December 9, 2011 at 5:40pm

anyone do a real test, I read this from PHP.net

FYI, openssl_random_pseudo_bytes() can be incredibly slow under Windows, to the point of being unusable. It frequently times out (>30 seconds execution time) on several Windows machines of mine.

Apparently, it's a known problem with OpenSSL (not PHP specifically).

See: http://www.google.com/search?q=openssl_random_pseudo_bytes+slow

and seems fixed on latest PHP version..
http://stackoverflow.com/questions/1940168/openssl-random-pseudo-bytes-i...

#6

Posted by catch on December 13, 2011 at 3:46am
Status:reviewed & tested by the community» needs review

Since we only require 5.3.2, we might want to do a version check here. Moving back to needs review.

#7

Posted by xjm on December 13, 2011 at 4:08am
Status:needs review» needs work
Issue tags:+Novice

Tagging as novice for

  1. Rerolling the patch for the new D8 directory structure.
  2. Adding a version check. You can use version_compare() to check the PHP version, and the result can be cached statically because it will not change within a request. Add the version to the condition in elseif (function_exists('openssl_random_pseudo_bytes')). It should be at least 5.3.2 5.3.4.

#8

Posted by kotnik on December 13, 2011 at 10:05am
Status:needs work» needs review

Here's a patch that does what's been requested in #7.

But, shouldn't it check for PHP 5.3.4 since that version fixed openssl_random_pseudo_bytes locking on Windows?

AttachmentSizeStatusTest resultOperations
drupal-838800-2.patch1.74 KBIdlePASSED: [[SimpleTest]]: [MySQL] 34,172 pass(es).View details

#9

Posted by xjm on December 13, 2011 at 4:24pm
Status:needs review» needs work

#8: You're correct, 5.3.4, not 5.3.2. Thanks!

#10

Posted by kotnik on December 13, 2011 at 4:43pm
Status:needs work» needs review

Ok, here's the re-roll.

AttachmentSizeStatusTest resultOperations
drupal-838800-3.patch2.07 KBIdlePASSED: [[SimpleTest]]: [MySQL] 34,166 pass(es).View details

#11

Posted by marvil07 on January 28, 2012 at 9:17am
Status:needs review» reviewed & tested by the community

Patch is doing what was agreed on comment 7 and 9.

#12

Posted by droplet on January 28, 2012 at 11:22am

Manually tested ??

Maybe bump up PHP version again? PHP 5.3.8 and below has hash collision vulnerability. I think all hosting will bump up their PHP version.

#13

Posted by Damien Tournoud on January 28, 2012 at 12:06pm

The hash collision is unrelated to this patch.

#14

Posted by droplet on January 28, 2012 at 12:07pm

@#13,
yes, unrelated :)

#15

Posted by marvil07 on January 28, 2012 at 5:11pm

Manually tested ??

Yes, from a PHP 5.3.9
Since I do not have a non *nix platform to test the exact case, I have just commented the /dev/urandom if and only left the elseif as if; and it works fine.

#16

Posted by Dries on January 29, 2012 at 9:38pm
Version:8.x-dev» 7.x-dev
Status:reviewed & tested by the community» patch (to be ported)

Committed to 8.x. We may be able to remove the version check before we release 8.x. We'll see.

#17

Posted by jromine on February 2, 2012 at 4:59am
Status:patch (to be ported)» needs review

Re-rolled patch for Drupal 7.x.

AttachmentSizeStatusTest resultOperations
drupal-838800-4.patch1.7 KBIdlePASSED: [[SimpleTest]]: [MySQL] 37,708 pass(es).View details

#18

Posted by philosurfer on February 2, 2012 at 5:08am
Status:needs review» reviewed & tested by the community

#17 Works!

Drupal v7.12
PHP v5.3.5
Ubuntu v11.10

#19

Posted by webchick on February 14, 2012 at 6:48pm
Status:reviewed & tested by the community» fixed

Yay for more randomness. :)

Committed and pushed to 7.x. Thanks!

#20

Posted by System Message on February 28, 2012 at 6:50pm
Status:fixed» closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

nobody click here