[wp-trac] [WordPress Trac] #28633: Generate better random numbers

[WordPress Trac]

#28633: Generate better random numbers
-------------------------------------+------------------------------
 Reporter:  sarciszewski             |       Owner:
     Type:  enhancement              |      Status:  new
 Priority:  normal                   |   Milestone:  Awaiting Review
Component:  Security                 |     Version:  trunk
 Severity:  major                    |  Resolution:
 Keywords:  needs-testing has-patch  |     Focuses:
-------------------------------------+------------------------------

Comment (by dd32):

 I've spent some time looking into this, and as a hardening-only patch,
 this seems like it's going in a good direction. I'm approaching this
 explicitly as a hardening-only patch as I believe `wp_rand()` we have in
 WordPress is already safe from prediction (and related) attacks, but that
 it was also written when the latest version of PHP available was 5.2.8, so
 did not have most of these available to it.

 Here's a rundown of the currently available sources that we can use:
 * Mcrypt
  - If the extension is available
  - PHP 5.3.7+ only for Windows (uses native Crypto Library)
 * /dev/urandom
  - If *nix and readable
  - may eat entropy in PHP < 5.3.3 (as it'll use read-ahead buffering)
  - not available on Windows
 * OpenSSL
  - If the extension available
  - PHP 5.3.4+ only for Windows, as it's interactive-display random seed
 uses lots of processing time (30-60s+) on a non-interactive screen
  - Uses Windows native Crypto API in PHP 5.4+ (might be a better minimum
 for windows)
  - May use a non-strong crypto source on some "rare" systems (see
 `$crypto_strong` param)
  - Also worth noting OpenSSL has a myriad of random generators, I believe
 at least one of these leverages `/dev/random` (or urandom/srandom) on
 supported platforms, but I'm unable to determine what platforms/OpenSSL
 versions at a glance
 * Windows specific: CAPICOM
  - Deprecated; For Windows 2008 and lower (XP, Visa, 7, 2003, 2008)
  - Available when the optional package is installed
  - Haven't tested this, seems like it can have permissions issues
 * Windows specific: DOTNET API
  - Supported in Windows 2008 R2 and greater with .NET 1.0 or greater
 bindings enabled
  - Haven't tested this, seems like it can have permissions issues
  - Reports of it not working reliably at all in PHP due to incompatible
 types

 It seems that randomness support on Windows servers is not-great in older
 versions of PHP, and we should potentially ditch windows-specific external
 API's and leverage OpenSSL/Mcrypt instead as they work reliably with the
 native Windows crypto API's in PHP 5.3.7/5.4+.

 The simple approach we can take here, assuming that our existing
 `wp_rand()` is not insecure, is to simply beef up `wp_rand()` to leverage
 these alternative random sources when available.
 This also leads me to questioning if we should support `/dev/urandom`
 which although is available on many hosting environments, is also less and
 less useful as OpenSSL or Mcrypt are available on more installs. Although
 i'm not sure of the penetration of Mcrypt, OpenSSL is very commonly
 supported. The code to use `urandom` is lightweight and straight-forward
 however, so I don't see a significant reason not to include it.

 So, I've attached two patches
 * [attachment:28633.8.diff 28633.8.diff] This is my WIP patch against the
 ones suggested here and was based off of [attachment:28633_with_md5.patch
 28633_with_md5.patch]
  - PHP syntax issues corrected
  - Windows version checks added
  - uses our `mbstring_binary_safe_encoding()` function to avoid the need
 for `wp_binsafe_strlen()`
  - rewrites the urandom handling in `wp_random_bytes()` to be more
 explicit and cleaner
  - fixes `wp_secure_rand()` to return inclusive values between `$min` and
 `$max` while making the code easier to read (It can also now handle
 `wp_secure_rand(1, 2)` which it couldn't previously)
  - a bunch of other formatting and code cleanliness issues
 * [attachment:28633.9.diff 28633.9.diff] however is an initial rewrite
 into what I think we should aim for
  - it renames the functions (although I'm really not sold on the names
 I've chosen at all, just wanted to remove the secure keyword)
  - moves towards ditching `wp_secure_rand()` and simply having `wp_rand()`
  - Drops the Windows-specific branches but leaves urandom in play.
  - In this scenario these functions would live elsewhere, i'm just
 including them in this file for ease of patch comparison

 Thoughts? Suggestions?

--
Ticket URL: <https://core.trac.wordpress.org/ticket/28633#comment:35>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform