MaxSem has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/405374 )
Change subject: DO NOT MERGE: clean up CryptRand for PHP7
......................................................................
DO NOT MERGE: clean up CryptRand for PHP7
This kills various shims and b/c hacks and makes this class a
thin wrapper around random_bytes() - so thin that one might question
if it's even needed.
Change-Id: Ic30376a90e66d8f00dab86e7e6466fb3a750b87d
---
M includes/ServiceWiring.php
M includes/installer/Installer.php
M includes/installer/i18n/en.json
M includes/installer/i18n/qqq.json
M includes/libs/CryptRand.php
M includes/utils/MWCryptRand.php
6 files changed, 30 insertions(+), 334 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/74/405374/1
diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index 79e5b84..3a04514 100644
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -178,27 +178,8 @@
return new WatchedItemQueryService(
$services->getDBLoadBalancer() );
},
- 'CryptRand' => function ( MediaWikiServices $services ) {
- $secretKey = $services->getMainConfig()->get( 'SecretKey' );
+ 'CryptRand' => function () {
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' )
);
},
diff --git a/includes/installer/Installer.php b/includes/installer/Installer.php
index 5ea9bfe..dd44b51 100644
--- a/includes/installer/Installer.php
+++ b/includes/installer/Installer.php
@@ -1517,21 +1517,9 @@
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 6d4c485..dd93f18 100644
--- a/includes/installer/i18n/en.json
+++ b/includes/installer/i18n/en.json
@@ -292,7 +292,6 @@
"config-install-interwiki-exists": "<strong>Warning:</strong> 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": "<strong>Warning:</strong> {{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": "<strong>Error:</strong> 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 2fc95ce..f287eee 100644
--- a/includes/installer/i18n/qqq.json
+++ b/includes/installer/i18n/qqq.json
@@ -312,7 +312,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/CryptRand.php b/includes/libs/CryptRand.php
index 859d58b..0b716ba 100644
--- a/includes/libs/CryptRand.php
+++ b/includes/libs/CryptRand.php
@@ -27,183 +27,58 @@
class CryptRand {
/**
- * Minimum number of iterations we want to make in our drift
calculations.
+ * @deprecated since 1.31, 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.31, 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
- *
- * @var callable[]
- */
- protected $randomFuncs = [];
-
- /**
- * List of files to generate some random state from
- *
- * @var string[]
- */
- protected $randomFiles = [];
/**
* @var LoggerInterface
*/
protected $logger;
- public function __construct( array $randomFuncs, array $randomFiles,
LoggerInterface $logger ) {
- $this->randomFuncs = $randomFuncs;
- $this->randomFiles = $randomFiles;
+ public function __construct( LoggerInterface $logger ) {
$this->logger = $logger;
}
/**
* Initialize an initial random state based off of whatever we can find
+ *
+ * @deprecated since 1.31, unused and does nothing
+ *
* @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 ) {
- MediaWiki\suppressWarnings();
- $stat = stat( $file );
- MediaWiki\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.31, 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.31, 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;
+ throw new Exception( __CLASS__ . ' now returns guaranteed
cryptographically secure random, no weak cryptography is needed' );
}
/**
@@ -211,14 +86,12 @@
* random bytes generation in the previously run generate* call
* was cryptographically strong.
*
+ * @deprecated since 1.31, always returns true
+ *
* @return bool Returns true if the source was strong, false if not.
*/
public function wasStrong() {
- if ( is_null( $this->strong ) ) {
- throw new RuntimeException( __METHOD__ . ' called
before generation of random data' );
- }
-
- return $this->strong;
+ return true;
}
/**
@@ -228,148 +101,11 @@
* 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 ) {
- // 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.
- if ( function_exists( 'mcrypt_create_iv' ) ) {
- $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 ) {
- if ( function_exists( 'openssl_random_pseudo_bytes' ) )
{
- $rem = $bytes - strlen( $buffer );
- $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.
- MediaWiki\suppressWarnings();
- $urandom = fopen( "/dev/urandom", "rb" );
- MediaWiki\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 );
}
/**
@@ -379,18 +115,15 @@
* 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 ) {
+ public 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( $this->generate( $bytes, $forceStrong ) );
+ $hex = bin2hex( $this->generate( $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
diff --git a/includes/utils/MWCryptRand.php b/includes/utils/MWCryptRand.php
index 5818958..697b5bf 100644
--- a/includes/utils/MWCryptRand.php
+++ b/includes/utils/MWCryptRand.php
@@ -39,10 +39,12 @@
* random bytes generation in the previously run generate* call
* was cryptographically strong.
*
+ * @deprecated since 1.31, always returns true
+ *
* @return bool Returns true if the source was strong, false if not.
*/
public static function wasStrong() {
- return self::singleton()->wasStrong();
+ return true;
}
/**
@@ -52,13 +54,10 @@
* 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 static function generate( $bytes, $forceStrong = false ) {
- return self::singleton()->generate( $bytes, $forceStrong );
+ public static function generate( $bytes ) {
+ return self::singleton()->generate( $bytes );
}
/**
@@ -68,12 +67,9 @@
* 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 ) {
+ return self::singleton()->generateHex( $chars );
}
}
--
To view, visit https://gerrit.wikimedia.org/r/405374
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic30376a90e66d8f00dab86e7e6466fb3a750b87d
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: MaxSem <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
