Page MenuHomePhabricator

Deprecation warnings about mcrypt_* functions in PHP 7.1
Closed, ResolvedPublic

Description

Using MW master and PHP 7.1 beta 3.

See warnings starting at line 742: https://travis-ci.org/JeroenDeDauw/Maps/jobs/154749309

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 24 2016, 1:46 PM
Aklapper renamed this task from Deprecation warning mcrypt_create_iv to Deprecation warnings about mcrypt_* functions in PHP 7.1.Aug 24 2016, 2:03 PM
Aklapper moved this task from Unsorted to PHP 7 on the [DO NOT USE] NewPHP board.
TheDJ added a comment.Aug 24 2016, 3:12 PM

The CSPRNG openssl offers is a userspace PRNG based on hash functions, which goes against the advice of Thomas Ptacek to use /dev/urandom. The only one-liner alternative is mcrypt_create_iv(), as demonstrated above, but this function is only exposed if you enable the mcrypt extension. Fortunately, PHP 7 will offer a core random_bytes() function that leverages the kernel's CSPRNG.

https://github.com/wikimedia/mediawiki/blob/8d590fac31d5b37b892ea76852da029bb01cdbed/includes/utils/MWCryptRand.php#L233
reads:

If available make use of mcrypt_create_iv URANDOM source to generate randomness

So. seems obvious enough.

Paladox moved this task from PHP 7 to PHP 7.1 on the [DO NOT USE] NewPHP board.Aug 31 2016, 9:21 AM
Paladox added a subscriber: Paladox.
Paladox added a comment.EditedAug 31 2016, 9:30 AM

I guess we should start moving towards removing those functions from mw, since it will be removed in php 7.2.

I'm wondering can mw work without using those functions?

Are there any alternatives to mycrypt?

@Paladox

"PHP 7 will offer a core random_bytes() function that leverages the kernel's CSPRNG"

Oh thanks, I guess we should replace it with that.

Aklapper moved this task from PHP 7.1 to PHP 7 on the [DO NOT USE] NewPHP board.Sep 2 2016, 6:57 AM
Paladox added a comment.EditedDec 2 2016, 8:31 AM

Php 7.1 was released yesterday.

https://wiki.php.net/rfc/mcrypt-viking-funeral

Change 324897 had a related patch set uploaded (by TheDJ):
Add support for PHP7 random_bytes in favor of mcrypt_create_iv

https://gerrit.wikimedia.org/r/324897

Reedy moved this task from PHP 7 to PHP 7.1 on the [DO NOT USE] NewPHP board.Dec 2 2016, 6:40 PM

@Legoktm It would be nice if someone could do a follow-up on this as our tests fail [0, 1] with:

PHP Deprecated:  Function mcrypt_create_iv() is deprecated in /home/travis/build/SemanticMediaWiki/mw/includes/libs/CryptRand.php on line 254

[0] https://github.com/SemanticMediaWiki/SemanticMediaWiki/pull/1733
[1] https://travis-ci.org/SemanticMediaWiki/SemanticMediaWiki/builds/203149177

php 7.1 is now generally available. We should start fixing this now. I am having the same deprecated warnings filling my log. It also gets displayed on mw-config.

We should replace mcrypt_create_iv() with random_bytes() per docs http://php.net/manual/en/function.mcrypt-create-iv.php

Change 343133 had a related patch set (by Paladox) published:
Update to support php 7.1

https://gerrit.wikimedia.org/r/343133

Change 343133 abandoned by Paladox:
Update to support php 7.1

Reason:
https://gerrit.wikimedia.org/r/#/c/324897/1

https://gerrit.wikimedia.org/r/343133

Change 324897 merged by jenkins-bot:
[mediawiki/core] Add support for PHP7 random_bytes in favor of mcrypt_create_iv

https://gerrit.wikimedia.org/r/324897

JeroenDeDauw added a comment.EditedApr 20 2017, 7:58 AM

The changes are cool and all, but they do not fix the mcrypt_create_iv deprecation warning, since the old code is still getting executed, even when random_bytes is available.

The CI of various extensions is still failing on PHP 7.1 due to this issue, even with latest master: https://travis-ci.org/SemanticMediaWiki/SemanticMediaWiki/jobs/203149187

TheDJ added a comment.May 9 2017, 3:37 PM

@JeroenDeDauw Is this still a problem ? I cannot find corresponding info in the link you posted.

There are two places in core that reference mcrypt, CryptRand (which DJ patched for using random_bytes) and MediaWiki\Session\Session (which checks for openssl first).

The CryptRand code basically looks like:

		if ( strlen( $buffer ) < $bytes ) {
			if ( function_exists( 'random_bytes' ) ) {
				$rem = $bytes - strlen( $buffer );
				$buffer .= random_bytes( $rem );
			}
			if ( strlen( $buffer ) >= $bytes ) { // <-- if random_bytes is called, this should always be true
				$this->strong = true;
			}
		}

		if ( strlen( $buffer ) < $bytes ) { // <-- if random_bytes is called, this should always be false
			// use mcrypt
		}

@JeroenDeDauw / @mwjames is this still occurring on latest master in your tests? The travis-ci log linked in T143788#3196665 doesn't have any mcrypt related deprecation functions AFAIS.

TheDJ added a subscriber: Tgr.EditedMay 10 2017, 9:46 AM

I do note that @Tgr left two more comments after merge.
1: maybe do a version check on random_bytes to protect against user land polyfills being dragged in.

I'll make a patch for this.

2: random_bytes can throw

I considered the latter 'good'. As in: if you are running php > 7.0 on a platform that doesn't support strong cryptography, then a fatal error might be a good idea.
One thing I didn't consider, that maybe in some platforms/containers, you might not be allowed to use strong cryptography ? Is that a real thing ? not sure.
Guidance appreciated.

@Legoktm regarding MediaWiki\Session\Session, we should probably fix that to use random bytes as well right ?
:Ah in Session, we only use it for encryption, not for randomness. So I guess that is just fine being secondary to openssl.

Change 353026 had a related patch set uploaded (by TheDJ; owner: TheDJ):
[mediawiki/core@master] CryptRand: only use random_bytes on php 7 and later

https://gerrit.wikimedia.org/r/353026

Change 353026 merged by jenkins-bot:
[mediawiki/core@master] CryptRand: only use random_bytes on php 7 and HHVM

https://gerrit.wikimedia.org/r/353026

Change 358302 had a related patch set uploaded (by Paladox; owner: TheDJ):
[mediawiki/core@REL1_29] CryptRand: only use random_bytes on php 7 and HHVM

https://gerrit.wikimedia.org/r/358302

Change 358302 merged by jenkins-bot:
[mediawiki/core@REL1_29] CryptRand: only use random_bytes on php 7 and HHVM

https://gerrit.wikimedia.org/r/358302

wasn't there also security alerts about the excessive use of urandom in the same system user account (used by the PHP instance) because it can easily exceed the available entropy, and so it is also easily attackable ?
Are there ways to improve it by using a mix of "urandom" entropy for stateless generation and some limited use of a stateful PRNG (with some constraints, notably if they are used from online external website accesses: is there a way then to delay the speed of random number generation for clients that exceed the threshold (i.e. reducing their experimented performance) to limit attacks and force them to use an unpredictable urandom number once the urandom entropy has been "refilled" with reasonnably unpredictable entropy sources ?

How does PHP7's random_bytes() mitigate these possible attacks ?

Is there several distinct urandom sources we could use for different things ? (e.g. separating the generation sequences for sessions tokens, or edit tokens, or custom code in extensions and parser functions ? I think the most critical random number generation is to protect wikis against attacks on sessions.

Another one would be for generation of OAuth tokens (from third party sites which may also have been attacked or could generate their own very weak random generators when they initiate sessions with our wikis and register new credentials for some wiki users: this could allow third party access to local private data of wiki users on the wiki, including impersonation of a user). We should pace the fequency of new OAuth accrediation requests from the same OAuth provider, assigning them their own random generator with enough initialization from an urandom source and then some local cache to avoid breaking it by creating many OAuth requests from different OAuth providers.

There's also the problem of time attacks which could inform attacks when a true new urandom is used or when the basic PRNG sequence is reused from the cache. Another one would be to protect from attacks for SSL initialization in HTTPS by some HTTPS client.

I think all this research scope goes largely above Mediawiki technical capabilities: this should be discussed and evaluated with PHP technical designers (there are a lot of companies working on securing it for lot of websites, we should benefit from this work). Its difficult for us to assess the security of PHP, even with the new PHP7 urandom_bytes() feature (and anyway MediaWiki is not made only for PHP7: we need to study alternatives when PHP7 is not used but a former version, or another PHP engine or compatible faster engine such as Zend which could have better alternatives; the sameis true for other serverside scripting/templating engines or application servers, including those implemented on top of .Net, Java, Python VMs or integrated in IIS, or with Apache plugins, or in Oracle servers).

Verdy_p added a comment.EditedJul 10 2017, 9:13 PM

Just a basic concept (not a proof of concept): can we create a pool of preinitialized random generators, with unpredictable max lifetime, fed possibly by network services querying multiple external pools (possibly from various local or remote servers), then cached and selected randomly by one of them, and switched from one to another at unpredictable time, in order to increase the total entropy available ? And can we protect this pool from time attacks to avoid someone knowing when (and then which one) a PRNG sequence is used, or when it was initialized from its entropy source ?

The concept would be that one of the initialized PRNG would be selected randomly, but would then randomly die (get out of the pool until it is fed again from an external entropy source) or otherwise would remain cached in its current state even if we don't select that pool immediately.

Such pool could store the various PRNG state in a database (with an in-memory cache for performance).

This could improve servers that have poor local urandom entropy. Then to feed the pool would just require an external background service (possibly a cron querying external entropy servers by a simple web service whose implementation would be not more than just a basic Telnet socket emitting random bytes from its entropy source), or it could be provided by custom scripts using different implementations.

We could also not depend only on PHP7 or specific hardware capabilities or devices for specific OSes: each source could be able to limit themselves the number of random bytes they would emit to initialize a new PRNG sequence in the pool and set its own lifetime parameters (max duration, max number of numbers generated).

(Looking for PRNG pool, I found an Wikipedia article about "Fortuna (PRNG)", a flexible system allowing the setup of such pools with unpredible lifetime which is easily customizable to connect variosu entropy sources and create pools with arbitrary number of cached PRNG sequences, that can be (re)initialized asynchronously, but unpredictably, and independantly of the time it will be used by applications)

MaxSem added a subscriber: MaxSem.Aug 22 2017, 11:35 PM

Besides "loss of entropy" being a misnomer (see e.g. here), we use CSPRNG mostly for session encryption and the typical number is under 2 logins per second. Which means that overall CSPRNG usage across the cluster is negligibly low and we're way too far from "exhaustion" of any kind on this front.

Change 374098 had a related patch set uploaded (by Reedy; owner: TheDJ):
[mediawiki/core@REL1_27] Add support for PHP7 random_bytes in favor of mcrypt_create_iv

https://gerrit.wikimedia.org/r/374098

Change 374099 had a related patch set uploaded (by Reedy; owner: TheDJ):
[mediawiki/core@REL1_27] CryptRand: only use random_bytes on php 7 and HHVM

https://gerrit.wikimedia.org/r/374099

Change 374098 merged by jenkins-bot:
[mediawiki/core@REL1_27] Add support for PHP7 random_bytes in favor of mcrypt_create_iv

https://gerrit.wikimedia.org/r/374098

Change 374099 merged by jenkins-bot:
[mediawiki/core@REL1_27] CryptRand: only use random_bytes on php 7 and HHVM

https://gerrit.wikimedia.org/r/374099

Change 374108 had a related patch set uploaded (by Reedy; owner: TheDJ):
[mediawiki/core@REL1_28] Add support for PHP7 random_bytes in favor of mcrypt_create_iv

https://gerrit.wikimedia.org/r/374108

Change 374108 merged by jenkins-bot:
[mediawiki/core@REL1_28] Add support for PHP7 random_bytes in favor of mcrypt_create_iv

https://gerrit.wikimedia.org/r/374108

Change 374109 had a related patch set uploaded (by Reedy; owner: TheDJ):
[mediawiki/core@REL1_28] CryptRand: only use random_bytes on php 7 and HHVM

https://gerrit.wikimedia.org/r/374109

Reedy added a subscriber: Reedy.Aug 28 2017, 12:34 AM

Once fixed it needs backport to 1.28 where this also happens: An error: Deprecated: Function mcrypt_create_iv() is deprecated

Done for CryptRand.

For sessions, like other versions (including 1.27), it'll use openssl first

Change 374109 merged by jenkins-bot:
[mediawiki/core@REL1_28] CryptRand: only use random_bytes on php 7 and HHVM

https://gerrit.wikimedia.org/r/374109

Do we need to do anything else for this?

MaxSem closed this task as Resolved.Oct 14 2017, 12:29 AM
MaxSem claimed this task.

The project in question now passes tests assuming fixed.