From 9bab7de5f8a249239de1466ec9c1d7a5d404ba00 Mon Sep 17 00:00:00 2001 From: Max Semenik Date: Fri, 19 Jan 2018 14:42:56 -0800 Subject: [PATCH] Clean up CSPRNG support for PHP7 Replace it all with random_bytes(), leave only MWCryptRand::generateHex() as a convenience helper. Change-Id: Ic30376a90e66d8f00dab86e7e6466fb3a750b87d --- RELEASE-NOTES-1.32 | 3 + includes/MediaWikiServices.php | 1 + includes/OutputPage.php | 4 +- includes/ServiceWiring.php | 29 +- includes/installer/Installer.php | 14 +- includes/installer/i18n/en.json | 1 - includes/installer/i18n/qqq.json | 1 - includes/libs/CryptHKDF.php | 15 +- includes/libs/CryptRand.php | 338 ++---------------- includes/password/BcryptPassword.php | 2 +- includes/password/EncryptedPassword.php | 4 +- includes/password/Pbkdf2Password.php | 2 +- includes/session/Session.php | 2 +- includes/utils/MWCryptRand.php | 45 ++- .../phpunit/includes/session/SessionTest.php | 2 +- 15 files changed, 71 insertions(+), 392 deletions(-) diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32 index 4e2bdfae24..cdc25827f3 100644 --- a/RELEASE-NOTES-1.32 +++ b/RELEASE-NOTES-1.32 @@ -186,6 +186,9 @@ because of Phabricator reports. * The ApiQueryContributions class has been renamed to ApiQueryUserContribs. * The XMPInfo, XMPReader, and XMPValidate classes have been deprecated in favor of the namespaced classes provided by the wikimedia/xmp-reader library. +* Class CryptRand, everything in MWCryptRand except generateHex() and function + MediaWikiServices::getCryptRand() are deprecated, use random_bytes() to + generate cryptographically secure random byte sequences. === Other changes in 1.32 === * … diff --git a/includes/MediaWikiServices.php b/includes/MediaWikiServices.php index ac98683535..dbb18a77fb 100644 --- a/includes/MediaWikiServices.php +++ b/includes/MediaWikiServices.php @@ -539,6 +539,7 @@ class MediaWikiServices extends ServiceContainer { /** * @since 1.28 + * @deprecated since 1.32, use random_bytes()/random_int() * @return CryptRand */ public function getCryptRand() { diff --git a/includes/OutputPage.php b/includes/OutputPage.php index f92f4f38da..6700cdbb49 100644 --- a/includes/OutputPage.php +++ b/includes/OutputPage.php @@ -4018,8 +4018,8 @@ class OutputPage extends ContextSource { } if ( $this->CSPNonce === null ) { // XXX It might be expensive to generate randomness - // on every request, on windows. - $rand = MWCryptRand::generate( 15 ); + // on every request, on Windows. + $rand = random_bytes( 15 ); $this->CSPNonce = base64_encode( $rand ); } return $this->CSPNonce; diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index ace64ab928..3f8ba185d5 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -188,29 +188,8 @@ return [ ); }, - 'CryptRand' => function ( MediaWikiServices $services ) { - $secretKey = $services->getMainConfig()->get( 'SecretKey' ); - return new CryptRand( - [ - // To try vary the system information of the state a bit more - // by including the system's hostname into the state - 'wfHostname', - // It's mostly worthless but throw the wiki's id into the data - // for a little more variance - 'wfWikiID', - // If we have a secret key set then throw it into the state as well - function () use ( $secretKey ) { - return $secretKey ?: ''; - } - ], - // The config file is likely the most often edited file we know should - // be around so include its stat info into the state. - // The constant with its location will almost always be defined, as - // WebStart.php defines MW_CONFIG_FILE to $IP/LocalSettings.php unless - // being configured with MW_CONFIG_CALLBACK (e.g. the installer). - defined( 'MW_CONFIG_FILE' ) ? [ MW_CONFIG_FILE ] : [], - LoggerFactory::getInstance( 'CryptRand' ) - ); + 'CryptRand' => function () { + return new CryptRand(); }, 'CryptHKDF' => function ( MediaWikiServices $services ) { @@ -231,9 +210,7 @@ return [ $cache = ObjectCache::getLocalClusterInstance(); } - return new CryptHKDF( $secret, $config->get( 'HKDFAlgorithm' ), - $cache, $context, $services->getCryptRand() - ); + return new CryptHKDF( $secret, $config->get( 'HKDFAlgorithm' ), $cache, $context ); }, 'MediaHandlerFactory' => function ( MediaWikiServices $services ) { diff --git a/includes/installer/Installer.php b/includes/installer/Installer.php index 05da0dbf49..0890bc4768 100644 --- a/includes/installer/Installer.php +++ b/includes/installer/Installer.php @@ -1597,23 +1597,11 @@ abstract class Installer { protected function doGenerateKeys( $keys ) { $status = Status::newGood(); - $strong = true; foreach ( $keys as $name => $length ) { - $secretKey = MWCryptRand::generateHex( $length, true ); - if ( !MWCryptRand::wasStrong() ) { - $strong = false; - } - + $secretKey = MWCryptRand::generateHex( $length ); $this->setVar( $name, $secretKey ); } - if ( !$strong ) { - $names = array_keys( $keys ); - $names = preg_replace( '/^(.*)$/', '\$$1', $names ); - global $wgLang; - $status->warning( 'config-insecure-keys', $wgLang->listToText( $names ), count( $names ) ); - } - return $status; } diff --git a/includes/installer/i18n/en.json b/includes/installer/i18n/en.json index 005fef6791..dbc18498e2 100644 --- a/includes/installer/i18n/en.json +++ b/includes/installer/i18n/en.json @@ -288,7 +288,6 @@ "config-install-interwiki-exists": "Warning: The interwiki table seems to already have entries.\nSkipping default list.", "config-install-stats": "Initializing statistics", "config-install-keys": "Generating secret keys", - "config-insecure-keys": "Warning: {{PLURAL:$2|A secure key|Secure keys}} ($1) generated during installation {{PLURAL:$2|is|are}} not completely safe. Consider changing {{PLURAL:$2|it|them}} manually.", "config-install-updates": "Prevent running unneeded updates", "config-install-updates-failed": "Error: Inserting update keys into tables failed with the following error: $1", "config-install-sysop": "Creating administrator user account", diff --git a/includes/installer/i18n/qqq.json b/includes/installer/i18n/qqq.json index 0779204613..d9edde5d13 100644 --- a/includes/installer/i18n/qqq.json +++ b/includes/installer/i18n/qqq.json @@ -308,7 +308,6 @@ "config-install-interwiki-exists": "Error notice during the installation saying that one of the database tables is already set up, so it's continuing without taking that step.", "config-install-stats": "*{{msg-mw|Config-install-database}}\n*{{msg-mw|Config-install-tables}}\n*{{msg-mw|Config-install-schema}}\n*{{msg-mw|Config-install-user}}\n*{{msg-mw|Config-install-interwiki}}\n*{{msg-mw|Config-install-stats}}\n*{{msg-mw|Config-install-keys}}\n*{{msg-mw|Config-install-sysop}}\n*{{msg-mw|Config-install-mainpage}}", "config-install-keys": "*{{msg-mw|Config-install-database}}\n*{{msg-mw|Config-install-tables}}\n*{{msg-mw|Config-install-schema}}\n*{{msg-mw|Config-install-user}}\n*{{msg-mw|Config-install-interwiki}}\n*{{msg-mw|Config-install-stats}}\n*{{msg-mw|Config-install-keys}}\n*{{msg-mw|Config-install-sysop}}\n*{{msg-mw|Config-install-mainpage}}", - "config-insecure-keys": "Parameters:\n* $1 - A list of names of the secret keys that were generated.\n* $2 - the number of items in the list $1, to be used with PLURAL.", "config-install-updates": "Message indicating that the updatelog table is filled with keys of updates that won't be run when running database updates.\n\nSee also:\n*{{msg-mw|Config-install-database}}\n*{{msg-mw|Config-install-tables}}\n*{{msg-mw|Config-install-interwiki}}\n*{{msg-mw|Config-install-stats}}\n*{{msg-mw|Config-install-keys}}\n*{{msg-mw|Config-install-updates}}\n*{{msg-mw|Config-install-schema}}\n*{{msg-mw|Config-install-user}}\n*{{msg-mw|Config-install-sysop}}\n*{{msg-mw|Config-install-mainpage}}", "config-install-updates-failed": "Used as error message. Parameters:\n* $1 - detailed error message", "config-install-sysop": "Message indicates that the administrator user account is being created\n\nSee also:\n*{{msg-mw|Config-install-database}}\n*{{msg-mw|Config-install-tables}}\n*{{msg-mw|Config-install-schema}}\n*{{msg-mw|Config-install-user}}\n*{{msg-mw|Config-install-interwiki}}\n*{{msg-mw|Config-install-stats}}\n*{{msg-mw|Config-install-keys}}\n*{{msg-mw|Config-install-sysop}}\n*{{msg-mw|Config-install-mainpage}}", diff --git a/includes/libs/CryptHKDF.php b/includes/libs/CryptHKDF.php index c41aab339c..0478a33700 100644 --- a/includes/libs/CryptHKDF.php +++ b/includes/libs/CryptHKDF.php @@ -99,22 +99,14 @@ class CryptHKDF { 'whirlpool' => 64, ]; - /** - * @var CryptRand - */ - private $cryptRand; - /** * @param string $secretKeyMaterial * @param string $algorithm Name of hashing algorithm * @param BagOStuff $cache * @param string|array $context Context to mix into HKDF context - * @param CryptRand $cryptRand * @throws InvalidArgumentException if secret key material is too short */ - public function __construct( $secretKeyMaterial, $algorithm, BagOStuff $cache, $context, - CryptRand $cryptRand - ) { + public function __construct( $secretKeyMaterial, $algorithm, BagOStuff $cache, $context ) { if ( strlen( $secretKeyMaterial ) < 16 ) { throw new InvalidArgumentException( "secret was too short." ); } @@ -122,7 +114,6 @@ class CryptHKDF { $this->algorithm = $algorithm; $this->cache = $cache; $this->context = is_array( $context ) ? $context : [ $context ]; - $this->cryptRand = $cryptRand; // To prevent every call from hitting the same memcache server, pick // from a set of keys to use. mt_rand is only use to pick a random @@ -150,12 +141,12 @@ class CryptHKDF { $lastSalt = $this->cache->get( $this->cacheKey ); if ( $lastSalt === false ) { // If we don't have a previous value to use as our salt, we use - // 16 bytes from CryptRand, which will use a small amount of + // 16 bytes from random_bytes(), which will use a small amount of // entropy from our pool. Note, "XTR may be deterministic or keyed // via an optional “salt value” (i.e., a non-secret random // value)..." - http://eprint.iacr.org/2010/264.pdf. However, we // use a strongly random value since we can. - $lastSalt = $this->cryptRand->generate( 16 ); + $lastSalt = random_bytes( 16 ); } // Get a binary string that is hashLen long $this->salt = hash( $this->algorithm, $lastSalt, true ); diff --git a/includes/libs/CryptRand.php b/includes/libs/CryptRand.php index f7702dd3ac..a1bbd099c4 100644 --- a/includes/libs/CryptRand.php +++ b/includes/libs/CryptRand.php @@ -23,187 +23,54 @@ * @author Daniel Friesen * @file */ -use Psr\Log\LoggerInterface; +/** + * @deprecated since 1.32, use random_bytes()/random_int() + */ class CryptRand { /** - * Minimum number of iterations we want to make in our drift calculations. + * @deprecated since 1.32, unused */ const MIN_ITERATIONS = 1000; /** - * Number of milliseconds we want to spend generating each separate byte - * of the final generated bytes. - * This is used in combination with the hash length to determine the duration - * we should spend doing drift calculations. + * @deprecated since 1.32, unused */ const MSEC_PER_BYTE = 0.5; /** - * A boolean indicating whether the previous random generation was done using - * cryptographically strong random number generator or not. - */ - protected $strong = null; - - /** - * List of functions to call to generate some random state + * Initialize an initial random state based off of whatever we can find * - * @var callable[] - */ - protected $randomFuncs = []; - - /** - * List of files to generate some random state from + * @deprecated since 1.32, unused and does nothing * - * @var string[] - */ - protected $randomFiles = []; - - /** - * @var LoggerInterface - */ - protected $logger; - - public function __construct( array $randomFuncs, array $randomFiles, LoggerInterface $logger ) { - $this->randomFuncs = $randomFuncs; - $this->randomFiles = $randomFiles; - $this->logger = $logger; - } - - /** - * Initialize an initial random state based off of whatever we can find * @return string */ protected function initialRandomState() { - // $_SERVER contains a variety of unstable user and system specific information - // It'll vary a little with each page, and vary even more with separate users - // It'll also vary slightly across different machines - $state = serialize( $_SERVER ); - - // Try to gather a little entropy from the different php rand sources - $state .= rand() . uniqid( mt_rand(), true ); - - // Include some information about the filesystem's current state in the random state - $files = $this->randomFiles; - - // We know this file is here so grab some info about ourselves - $files[] = __FILE__; - - // We must also have a parent folder, and with the usual file structure, a grandparent - $files[] = __DIR__; - $files[] = dirname( __DIR__ ); - - foreach ( $files as $file ) { - Wikimedia\suppressWarnings(); - $stat = stat( $file ); - Wikimedia\restoreWarnings(); - if ( $stat ) { - // stat() duplicates data into numeric and string keys so kill off all the numeric ones - foreach ( $stat as $k => $v ) { - if ( is_numeric( $k ) ) { - unset( $k ); - } - } - // The absolute filename itself will differ from install to install so don't leave it out - $path = realpath( $file ); - if ( $path !== false ) { - $state .= $path; - } else { - $state .= $file; - } - $state .= implode( '', $stat ); - } else { - // The fact that the file isn't there is worth at least a - // minuscule amount of entropy. - $state .= '0'; - } - } - - // Try and make this a little more unstable by including the varying process - // id of the php process we are running inside of if we are able to access it - if ( function_exists( 'getmypid' ) ) { - $state .= getmypid(); - } - - // If available try to increase the instability of the data by throwing in - // the precise amount of memory that we happen to be using at the moment. - if ( function_exists( 'memory_get_usage' ) ) { - $state .= memory_get_usage( true ); - } - - foreach ( $this->randomFuncs as $randomFunc ) { - $state .= call_user_func( $randomFunc ); - } - - return $state; + return ''; } /** * Randomly hash data while mixing in clock drift data for randomness * + * @deprecated since 1.32, unused and does nothing + * * @param string $data The data to randomly hash. * @return string The hashed bytes * @author Tim Starling */ protected function driftHash( $data ) { - // Minimum number of iterations (to avoid slow operations causing the - // loop to gather little entropy) - $minIterations = self::MIN_ITERATIONS; - // Duration of time to spend doing calculations (in seconds) - $duration = ( self::MSEC_PER_BYTE / 1000 ) * MWCryptHash::hashLength(); - // Create a buffer to use to trigger memory operations - $bufLength = 10000000; - $buffer = str_repeat( ' ', $bufLength ); - $bufPos = 0; - - // Iterate for $duration seconds or at least $minIterations number of iterations - $iterations = 0; - $startTime = microtime( true ); - $currentTime = $startTime; - while ( $iterations < $minIterations || $currentTime - $startTime < $duration ) { - // Trigger some memory writing to trigger some bus activity - // This may create variance in the time between iterations - $bufPos = ( $bufPos + 13 ) % $bufLength; - $buffer[$bufPos] = ' '; - // Add the drift between this iteration and the last in as entropy - $nextTime = microtime( true ); - $delta = (int)( ( $nextTime - $currentTime ) * 1000000 ); - $data .= $delta; - // Every 100 iterations hash the data and entropy - if ( $iterations % 100 === 0 ) { - $data = sha1( $data ); - } - $currentTime = $nextTime; - $iterations++; - } - $timeTaken = $currentTime - $startTime; - $data = MWCryptHash::hash( $data ); - - $this->logger->debug( "Clock drift calculation " . - "(time-taken=" . ( $timeTaken * 1000 ) . "ms, " . - "iterations=$iterations, " . - "time-per-iteration=" . ( $timeTaken / $iterations * 1e6 ) . "us)" ); - - return $data; + return ''; } /** * Return a rolling random state initially build using data from unstable sources + * + * @deprecated since 1.32, unused and does nothing + * * @return string A new weak random state */ protected function randomState() { - static $state = null; - if ( is_null( $state ) ) { - // Initialize the state with whatever unstable data we can find - // It's important that this data is hashed right afterwards to prevent - // it from being leaked into the output stream - $state = MWCryptHash::hash( $this->initialRandomState() ); - } - // Generate a new random state based on the initial random state or previous - // random state by combining it with clock drift - $state = $this->driftHash( $state ); - - return $state; + return ''; } /** @@ -211,191 +78,36 @@ class CryptRand { * random bytes generation in the previously run generate* call * was cryptographically strong. * - * @return bool Returns true if the source was strong, false if not. + * @deprecated since 1.32, always returns true + * + * @return bool Always true */ public function wasStrong() { - if ( is_null( $this->strong ) ) { - throw new RuntimeException( __METHOD__ . ' called before generation of random data' ); - } - - return $this->strong; + return true; } /** - * Generate a run of (ideally) cryptographically random data and return + * Generate a run of cryptographically random data and return * it in raw binary form. * You can use CryptRand::wasStrong() if you wish to know if the source used * was cryptographically strong. * * @param int $bytes The number of bytes of random data to generate - * @param bool $forceStrong Pass true if you want generate to prefer cryptographically - * strong sources of entropy even if reading from them may steal - * more entropy from the system than optimal. * @return string Raw binary random data */ - public function generate( $bytes, $forceStrong = false ) { + public function generate( $bytes ) { $bytes = floor( $bytes ); - static $buffer = ''; - if ( is_null( $this->strong ) ) { - // Set strength to false initially until we know what source data is coming from - $this->strong = true; - } - - if ( strlen( $buffer ) < $bytes ) { - // If available make use of PHP 7's random_bytes - // On Linux, getrandom syscall will be used if available. - // On Windows CryptGenRandom will always be used - // On other platforms, /dev/urandom will be used. - // Avoids polyfills from before php 7.0 - // All error situations will throw Exceptions and or Errors - if ( PHP_VERSION_ID >= 70000 - || ( defined( 'HHVM_VERSION_ID' ) && HHVM_VERSION_ID >= 31101 ) - ) { - $rem = $bytes - strlen( $buffer ); - $buffer .= random_bytes( $rem ); - } - if ( strlen( $buffer ) >= $bytes ) { - $this->strong = true; - } - } - - if ( strlen( $buffer ) < $bytes && function_exists( 'mcrypt_create_iv' ) ) { - // If available make use of mcrypt_create_iv URANDOM source to generate randomness - // On unix-like systems this reads from /dev/urandom but does it without any buffering - // and bypasses openbasedir restrictions, so it's preferable to reading directly - // On Windows starting in PHP 5.3.0 Windows' native CryptGenRandom is used to generate - // entropy so this is also preferable to just trying to read urandom because it may work - // on Windows systems as well. - $rem = $bytes - strlen( $buffer ); - $iv = mcrypt_create_iv( $rem, MCRYPT_DEV_URANDOM ); - if ( $iv === false ) { - $this->logger->debug( "mcrypt_create_iv returned false." ); - } else { - $buffer .= $iv; - $this->logger->debug( "mcrypt_create_iv generated " . strlen( $iv ) . - " bytes of randomness." ); - } - } - - if ( strlen( $buffer ) < $bytes && function_exists( 'openssl_random_pseudo_bytes' ) ) { - $rem = $bytes - strlen( $buffer ); - $openssl_strong = false; - $openssl_bytes = openssl_random_pseudo_bytes( $rem, $openssl_strong ); - if ( $openssl_bytes === false ) { - $this->logger->debug( "openssl_random_pseudo_bytes returned false." ); - } else { - $buffer .= $openssl_bytes; - $this->logger->debug( "openssl_random_pseudo_bytes generated " . - strlen( $openssl_bytes ) . " bytes of " . - ( $openssl_strong ? "strong" : "weak" ) . " randomness." ); - } - if ( strlen( $buffer ) >= $bytes ) { - // openssl tells us if the random source was strong, if some of our data was generated - // using it use it's say on whether the randomness is strong - $this->strong = !!$openssl_strong; - } - } - - // Only read from urandom if we can control the buffer size or were passed forceStrong - if ( strlen( $buffer ) < $bytes && - ( function_exists( 'stream_set_read_buffer' ) || $forceStrong ) - ) { - $rem = $bytes - strlen( $buffer ); - if ( !function_exists( 'stream_set_read_buffer' ) && $forceStrong ) { - $this->logger->debug( "Was forced to read from /dev/urandom " . - "without control over the buffer size." ); - } - // /dev/urandom is generally considered the best possible commonly - // available random source, and is available on most *nix systems. - Wikimedia\suppressWarnings(); - $urandom = fopen( "/dev/urandom", "rb" ); - Wikimedia\restoreWarnings(); - - // Attempt to read all our random data from urandom - // php's fread always does buffered reads based on the stream's chunk_size - // so in reality it will usually read more than the amount of data we're - // asked for and not storing that risks depleting the system's random pool. - // If stream_set_read_buffer is available set the chunk_size to the amount - // of data we need. Otherwise read 8k, php's default chunk_size. - if ( $urandom ) { - // php's default chunk_size is 8k - $chunk_size = 1024 * 8; - if ( function_exists( 'stream_set_read_buffer' ) ) { - // If possible set the chunk_size to the amount of data we need - stream_set_read_buffer( $urandom, $rem ); - $chunk_size = $rem; - } - $random_bytes = fread( $urandom, max( $chunk_size, $rem ) ); - $buffer .= $random_bytes; - fclose( $urandom ); - $this->logger->debug( "/dev/urandom generated " . strlen( $random_bytes ) . - " bytes of randomness." ); - - if ( strlen( $buffer ) >= $bytes ) { - // urandom is always strong, set to true if all our data was generated using it - $this->strong = true; - } - } else { - $this->logger->debug( "/dev/urandom could not be opened." ); - } - } - - // If we cannot use or generate enough data from a secure source - // use this loop to generate a good set of pseudo random data. - // This works by initializing a random state using a pile of unstable data - // and continually shoving it through a hash along with a variable salt. - // We hash the random state with more salt to avoid the state from leaking - // out and being used to predict the /randomness/ that follows. - if ( strlen( $buffer ) < $bytes ) { - $this->logger->debug( __METHOD__ . - ": Falling back to using a pseudo random state to generate randomness." ); - } - while ( strlen( $buffer ) < $bytes ) { - $buffer .= MWCryptHash::hmac( $this->randomState(), strval( mt_rand() ) ); - // This code is never really cryptographically strong, if we use it - // at all, then set strong to false. - $this->strong = false; - } - - // Once the buffer has been filled up with enough random data to fulfill - // the request shift off enough data to handle the request and leave the - // unused portion left inside the buffer for the next request for random data - $generated = substr( $buffer, 0, $bytes ); - $buffer = substr( $buffer, $bytes ); - - $this->logger->debug( strlen( $buffer ) . - " bytes of randomness leftover in the buffer." ); - - return $generated; + return random_bytes( $bytes ); } /** - * Generate a run of (ideally) cryptographically random data and return + * Generate a run of cryptographically random data and return * it in hexadecimal string format. - * You can use CryptRand::wasStrong() if you wish to know if the source used - * was cryptographically strong. * * @param int $chars The number of hex chars of random data to generate - * @param bool $forceStrong Pass true if you want generate to prefer cryptographically - * strong sources of entropy even if reading from them may steal - * more entropy from the system than optimal. * @return string Hexadecimal random data */ - public function generateHex( $chars, $forceStrong = false ) { - // hex strings are 2x the length of raw binary so we divide the length in half - // odd numbers will result in a .5 that leads the generate() being 1 character - // short, so we use ceil() to ensure that we always have enough bytes - $bytes = ceil( $chars / 2 ); - // Generate the data and then convert it to a hex string - $hex = bin2hex( $this->generate( $bytes, $forceStrong ) ); - - // A bit of paranoia here, the caller asked for a specific length of string - // here, and it's possible (eg when given an odd number) that we may actually - // have at least 1 char more than they asked for. Just in case they made this - // call intending to insert it into a database that does truncation we don't - // want to give them too much and end up with their database and their live - // code having two different values because part of what we gave them is truncated - // hence, we strip out any run of characters longer than what we were asked for. - return substr( $hex, 0, $chars ); + public function generateHex( $chars ) { + return MWCryptRand::generateHex( $chars ); } } diff --git a/includes/password/BcryptPassword.php b/includes/password/BcryptPassword.php index f811e3f514..4ba34effe5 100644 --- a/includes/password/BcryptPassword.php +++ b/includes/password/BcryptPassword.php @@ -65,7 +65,7 @@ class BcryptPassword extends ParameterizedPassword { // Replace + with ., because bcrypt uses a non-MIME base64 format strtr( // Random base64 encoded string - base64_encode( MWCryptRand::generate( 16, true ) ), + base64_encode( random_bytes( 16 ) ), '+', '.' ), 0, 22 diff --git a/includes/password/EncryptedPassword.php b/includes/password/EncryptedPassword.php index 0ea3c63198..d1774ba8af 100644 --- a/includes/password/EncryptedPassword.php +++ b/includes/password/EncryptedPassword.php @@ -60,7 +60,7 @@ class EncryptedPassword extends ParameterizedPassword { if ( count( $this->args ) ) { $iv = base64_decode( $this->args[0] ); } else { - $iv = MWCryptRand::generate( openssl_cipher_iv_length( $this->params['cipher'] ), true ); + $iv = random_bytes( openssl_cipher_iv_length( $this->params['cipher'] ) ); } $this->hash = openssl_encrypt( @@ -102,7 +102,7 @@ class EncryptedPassword extends ParameterizedPassword { $this->params = $this->getDefaultParams(); // Check the key size with the new params - $iv = MWCryptRand::generate( openssl_cipher_iv_length( $this->params['cipher'] ), true ); + $iv = random_bytes( openssl_cipher_iv_length( $this->params['cipher'] ) ); $this->hash = openssl_encrypt( $underlyingHash, $this->params['cipher'], diff --git a/includes/password/Pbkdf2Password.php b/includes/password/Pbkdf2Password.php index 541fd0e1a2..60650452fc 100644 --- a/includes/password/Pbkdf2Password.php +++ b/includes/password/Pbkdf2Password.php @@ -47,7 +47,7 @@ class Pbkdf2Password extends ParameterizedPassword { public function crypt( $password ) { if ( count( $this->args ) == 0 ) { - $this->args[] = base64_encode( MWCryptRand::generate( 16, true ) ); + $this->args[] = base64_encode( random_bytes( 16 ) ); } if ( $this->shouldUseHashExtension() ) { diff --git a/includes/session/Session.php b/includes/session/Session.php index 024bf9a2b0..e9a03f20f4 100644 --- a/includes/session/Session.php +++ b/includes/session/Session.php @@ -481,7 +481,7 @@ final class Session implements \Countable, \Iterator, \ArrayAccess { // Encrypt // @todo: import a pure-PHP library for AES instead of doing $wgSessionInsecureSecrets - $iv = \MWCryptRand::generate( 16, true ); + $iv = random_bytes( 16 ); $algorithm = self::getEncryptionAlgorithm(); switch ( $algorithm[0] ) { case 'openssl': diff --git a/includes/utils/MWCryptRand.php b/includes/utils/MWCryptRand.php index 581895807c..0fc45f7e11 100644 --- a/includes/utils/MWCryptRand.php +++ b/includes/utils/MWCryptRand.php @@ -28,6 +28,7 @@ use MediaWiki\MediaWikiServices; class MWCryptRand { /** + * @deprecated since 1.32 * @return CryptRand */ protected static function singleton() { @@ -39,41 +40,49 @@ class MWCryptRand { * random bytes generation in the previously run generate* call * was cryptographically strong. * - * @return bool Returns true if the source was strong, false if not. + * @deprecated since 1.32, always returns true + * + * @return bool Always true */ public static function wasStrong() { - return self::singleton()->wasStrong(); + return true; } /** - * Generate a run of (ideally) cryptographically random data and return + * Generate a run of cryptographically random data and return * it in raw binary form. - * You can use MWCryptRand::wasStrong() if you wish to know if the source used - * was cryptographically strong. + * + * @deprecated since 1.32, use random_bytes() * * @param int $bytes The number of bytes of random data to generate - * @param bool $forceStrong Pass true if you want generate to prefer cryptographically - * strong sources of entropy even if reading from them may steal - * more entropy from the system than optimal. * @return string Raw binary random data */ - public static function generate( $bytes, $forceStrong = false ) { - return self::singleton()->generate( $bytes, $forceStrong ); + public static function generate( $bytes ) { + return random_bytes( floor( $bytes ) ); } /** - * Generate a run of (ideally) cryptographically random data and return + * Generate a run of cryptographically random data and return * it in hexadecimal string format. - * You can use MWCryptRand::wasStrong() if you wish to know if the source used - * was cryptographically strong. * * @param int $chars The number of hex chars of random data to generate - * @param bool $forceStrong Pass true if you want generate to prefer cryptographically - * strong sources of entropy even if reading from them may steal - * more entropy from the system than optimal. * @return string Hexadecimal random data */ - public static function generateHex( $chars, $forceStrong = false ) { - return self::singleton()->generateHex( $chars, $forceStrong ); + public static function generateHex( $chars ) { + // hex strings are 2x the length of raw binary so we divide the length in half + // odd numbers will result in a .5 that leads the generate() being 1 character + // short, so we use ceil() to ensure that we always have enough bytes + $bytes = ceil( $chars / 2 ); + // Generate the data and then convert it to a hex string + $hex = bin2hex( random_bytes( $bytes ) ); + + // A bit of paranoia here, the caller asked for a specific length of string + // here, and it's possible (eg when given an odd number) that we may actually + // have at least 1 char more than they asked for. Just in case they made this + // call intending to insert it into a database that does truncation we don't + // want to give them too much and end up with their database and their live + // code having two different values because part of what we gave them is truncated + // hence, we strip out any run of characters longer than what we were asked for. + return substr( $hex, 0, $chars ); } } diff --git a/tests/phpunit/includes/session/SessionTest.php b/tests/phpunit/includes/session/SessionTest.php index f84d435f77..a74056d0ca 100644 --- a/tests/phpunit/includes/session/SessionTest.php +++ b/tests/phpunit/includes/session/SessionTest.php @@ -358,7 +358,7 @@ class SessionTest extends MediaWikiTestCase { $logger->clearBuffer(); // Unserializable data - $iv = \MWCryptRand::generate( 16, true ); + $iv = random_bytes( 16 ); list( $encKey, $hmacKey ) = TestingAccessWrapper::newFromObject( $session )->getSecretKeys(); $ciphertext = openssl_encrypt( 'foobar', 'aes-256-ctr', $encKey, OPENSSL_RAW_DATA, $iv ); $sealed = base64_encode( $iv ) . '.' . base64_encode( $ciphertext ); -- 2.20.1