Silverstripe-framework: Insecure random number generators in RandomGenerator should require explicit enabling

[madmatt]

Ideally we can add a Config variable, that defaults to false, that prevents the use of the last two random number generators (Capicom for Windows and mt_rand()) unless the config var is changed to true, so it is opt-in to produce non-psuedo-random numbers.

Probably a v4 change given it would change the output of the method (might throw an Exception where none was thrown before).

Reference: https://github.com/silverstripe/silverstripe-framework/blob/d38097ec146d5509fb397d930f069a313093ac13/src/Security/RandomGenerator.php#L56

[dhensby]

Are there any tangible risks to how it is at the moment?

[tractorcow]

The method comment states * Note: Returned values are not guaranteed to be crypto-safe,. This method should not be used in any place where it could potentially raise a security risk at all.

I would rather see this left in place, and we implemented a proper crypto-safe random number generator instead. php 5.5 (supported in 4.0) has better crypto-random generation.

[robbieaverill]

Sort of related - Function mcrypt_create_iv() is deprecated in framework/src/Security/RandomGenerator.php:30 during a module's Travis build on PHP 7.1. If we want to support 7.1 for SS 4 we'll probably need to replace it.

[robbieaverill]

Regarding dropping the deprecated (in 7.1) mcrypt library use, here is the RFC that drops it (for reference). It seems that the suitable replacement would be openssl_random_pseudo_bytes' which is already implemented in this method, just a lower priority thanmcrypt_create_iv`.

I propose we simply remove the mcrypt use, leave the rest and give a higher priority for Windows to the method that @tractorcow shared over the existing code. Personally, I don't see any reason why we shouldn't remove old Windows code now (4.x) in favour of more modern, cross platform methods.

I've modularised a bit and given openssl priority over mcrypt in this commit - some feedback would be appreciated in the new year. It's possibly not necessary to modularise, but I figured it would be worthwhile in the long run to reduce some function complexity.

[dhensby]

@robbieaverill from a quick look that commit looks good - but is there a thirdparty library we can use that does the same thing?

[dhensby]

https://github.com/ircmaxell/RandomLib this looks good, well maintained and regularly contributed to

[robbieaverill]

@dhensby yeah cool. As long as we are ok with replacing the functionality with a new backend essentially then I'll have a play with that instead

[robbieaverill]

Damn - @dhensby - looks like that module isn't 7.1 compatible yet (ref: ircmaxell/RandomLib#55). It's definitely worth implementing though.

[dhensby]

hmmm - it's not so much it doesn't work, just that it's relying on deprecated libs - that's a shame, but maybe not a show stopper?

[tractorcow]

I've removed my suggestion because it wasn't a correct secure random number generator.

PHP 7 has random_bytes, but of course we can't rely on this without bumping our minimum version to php7.

However, with combination of mcrypt_* being deprecated in php 7.1 onwards, and random_bytes being available from 7.0 onwards, we have at least a complete surface area that doesn't require us to add any new dependencies. :)

Note that random_bytes is documented as being cryptographically secure.

[kinglozzer]

@tractorcow How about random_bytes() + https://github.com/paragonie/random_compat?

[tractorcow]

This part concerns me a little:

If PHP cannot safely generate random data, this library will throw an Exception. It will never fall back to insecure random data. If this keeps happening, upgrade to a newer version of PHP immediately.

Can we ensure that we NEVER get an exception thrown with only the bare minimum of server dependencies installed? Otherwise we either need to bump our requirements or look for an alternative.

[kinglozzer]

IMO the best solution is to remove the RandomGenerator class entirely, and simply error on systems that don’t support random_bytes() or the polyfill. For the polyfill, the randomness sources are well documented so we could update our server requirements if we really want to. It doesn’t do anything drastically different to our currently implementation though (besides the removal of openssl_random_pseudo_bytes(), and _not_ falling back to mt_rand() - both of which are good things).

In the case of PHP 7, I’d like to think that it’s safe to assume that if it ships with PHP, it’ll be very well supported - but I can’t find anything to back that up! For reference, this is what the native function currently checks: https://github.com/php/php-src/blob/c8aa6f3a9a3d2c114d0c5e0c9fdd0a465dbb54a5/ext/standard/random.c#L84-L184.

[tractorcow]

Yeah but I think we need to do better for our users than "you'll get an error if you aren't configured properly." What they'll need to know is what to do to ensure that doesn't happen at all. Otherwise they could end up with errors in a live environment they didn't expect. I'd factor what that is before choosing to adopt this library.

PHP 7 should be fine, I'm mostly concerned about 5.x

[kinglozzer]

So to summarise, the requirements/order sources are checked are:

• Current RandomGenerator class:
  
  
  
  1. random_bytes() (PHP 7 only, no polyfill)
  
  
  2. mcrypt_create_iv()
  
  
  3. openssl_random_pseudo_bytes()
  
  
  4. /dev/urandom (Linux / BSD only)
  
  
  5. CAPICOM.Utilities.1 (Windows only)
  
  
  6. mt_rand()
  
  
• Polyfilled random_bytes():
  
  
  
  1. libsodium
  
  
  2. /dev/urandom (Linux / BSD only)
  
  
  3. mcrypt_create_iv() (which on Windows will try to read from CryptGenRandom)
  
  
  4. CAPICOM.Utilities.1 (Windows only)
  
  
  5. Exception thrown. There are some terse troubleshooting docs.
  
  
• PHP 7 random_bytes():
  
  
  
  1. CryptGenRandom (Windows only)
  
  
  2. arc4random_buf (OpenBSD/NetBSD only)
  
  
  3. getrandom(2) (Linux only)
  
  
  4. /dev/urandom
  
  
  5. Exceptions are thrown at different points (e.g. if getrandom(2) is available but fails, it will throw an exception immediately rather than fall back to /dev/urandom)
  
  

Would that info in the server requirements doc suffice? We could even catch the exceptions that random_bytes() throws and provide our own message to point people in the right direction.

[robbieaverill]

@kinglozzer worth noting that my recent PR adds priority for random bytes before mcrypt

[tractorcow]

@kinglozzer I think that's fine. ;)

[tractorcow]

Fixed with https://github.com/silverstripe/silverstripe-framework/pull/6940#pullrequestreview-39367377