openssl_random_pseudo_bytes() does not appear to cryptographically secure

[MasonM]

Although the PHP docs claim that openssl_random_pseudo_bytes() returns a "cryptographically strong" result as long as the $crypto_strong parameter is true, I checked the source and that does not appear to be the case. The openssl_random_pseudo_bytes() PHP function calls the RAND_psuedo_bytes() OpenSSL function, which the OpenSSL docs say should only be used for non-cryptographic purposes:

RAND_pseudo_bytes() has been deprecated. Users should use RAND_bytes() instead. RAND_pseudo_bytes() puts num pseudo-random bytes into buf. Pseudo-random byte sequences generated by RAND_pseudo_bytes() will be unique if they are of sufficient length, but are not necessarily unpredictable. They can be used for non-cryptographic purposes and for certain purposes in cryptographic protocols, but usually not for key generation etc.

It's quite possible I'm missing something here. If so, I apologize for wasting your time.

[paragonie-scott]

Although the PHP docs claim that openssl_random_pseudo_bytes() returns a "cryptographically strong" result as long as you pass true for the $crypto_strong parameter,

Where does it make that claim? You pass a variable (by reference) to the second argument of openssl_random_pseudo_bytes() and it will set it to false if it can't return strong random data.

---

This does sound like a bug that the PHP team should be made aware of.

So far, random_bytes() will try (in order):

1. mcrypt_create_iv with MCRYPT_DEV_URANDOM
2. openssl_random_pseudo_bytes
3. reading from /dev/urandom
4. (Windows) CAPICOM.Utilities.1's GetRandom() method

If all 4 fail, it will throw an exception which should hopefully kill the script that invoked it.

Would you be more comfortable if openssl_random_pseudo_bytes were demoted to the very bottom of the list? This shouldn't negatively affect security in any way.

[MasonM]

Where does it make that claim? You pass a variable (by reference) to the second argument of openssl_random_pseudo_bytes() and it will set it to false if it can't return strong random data.

You're right, I misread the docs there. I edited my comment.

I'm no security researcher, but it seems to me that openssl_random_pseudo_bytes() shouldn't be used to all if the goal is to always return a cryptographically secure result. Simply moving it to the bottom of the list would mean it could still be used in some cases, which would violate that goal.

Anyway, I entered a bug with the PHP team: https://bugs.php.net/bug.php?id=70014

We'll see what they have to say. Maybe they know something about RAND_pseudo_bytes() that I don't.

[paragonie-scott]

This bug report is marked as private.

It must be marked as a security bug, so the public cannot see its contents. Please let us know what they say.

[paragonie-scott]

Simply moving it to the bottom of the list would mean it could still be used in some cases, which would violate that goal.

Demoting it means, if run on a Linux system where /dev/urandom exists, it will read that first. And on a Windows system, it will rely on CAPICOM first. In an ideal world, the openssl call will never be encountered. If it is, it will only succeed if the $crypto_strong parameter is true.

If you don't have mcrypt (which invokes Windows' CryptGenRandom on a non-Unix environment), /dev/urandom, or CAPICOM on your system, and you're running PHP 5.x, this is the best we can hope to provide.

[SamMousa]

Shouldn't a version check be added before using openssl_random_bytes? The PHP bug is now closed: https://bugs.php.net/bug.php?id=70014

And it has been fixed in 5.6.10, but any other version might still be insecure and thus the argument by @MasonM still seems to apply.

[paragonie-scott]

More importantly: openssl_random_pseudo_bytes() might still be dangerous. Best to use it as a last resort. @ircmaxell

[SamMousa]

Doesn't it make more sense to just completely remove it then?

-- edit: or at the very least reopen this issue?

[paragonie-scott]

https://docs.google.com/spreadsheets/d/1Y74jasbZgLtgYcNhQG63yf5sam4eEaXMYDPgy11HFoo/edit#gid=0

Imagine this matrix without the openssl column.

[SamMousa]

I did, it is one of the reason I self-host ;-)

It would be possible to require some constant to be set (not ideal), before using openSSL, that way users of this library can choose whether they want possibly insecure behavior.

This project is supposed to be a "secure backport", not a "secure in almost all cases but maybe not in some backport" right?

[paragonie-scott]

https://github.com/paragonie/random_compat/commit/4629665f403924470bfe51027f4e6f80ea62db9e

Part of this patch only lets you use openssl on versions of PHP that fix PHP bug 70014.

https://github.com/paragonie/random_compat/commit/0143ad75621b08dba5cd00133f3662100f2942bf

If we comment this out it will implement what you suggest.

[tom--]

openssl_random_pseudo_bytes() makes me very uncomfortable. I outlined my concerns I the yii2 issue referenced ⤴︎

While libmcrypt is very scary, mcrypt_create_iv() is not because, iiuc, it goes straight to CryptGenRandom/kernel without touching libmcrypt. This makes it the best way, imo, to get random bytes on Windows in PHP < 7.

I tried using CAPICOM.Utilities GetRandom() years ago and gave up. I can't remember clearly why but my unclear memory is

• it's really old
• it's not generally available, requiring a framework that might not be installed
• it's really slow
• did I even get it working? :-/

[tom--]

What worries me most is those run-time environments in which PHP is so tightly sandboxed/jailed that nothing can access the proper system RNG. In this case OpenSSL is the only thing that will return a value but, at the same time, it won't be properly seeded because it too cannot read from the system RNG.

In this situation, OpenSSL might be adequate for some purposes (e.g. some uses of salts, email confirmation lookup keys, maybe even Diceware) but I wouldn't trust it for keys or IVs or stuff like that.

A lib that provides OpenSSL as a last resort in a situation like this needs somehow to educate the user.

Imagine PHP apps out there in the cloud with access to PHP's OpenSSL API but no proper randomness. This is what we enable if we aren't careful.

[paragonie-scott]

While libmcrypt is very scary, mcrypt_create_iv() is not

Right. That's part of ext/mcrypt not libmcrypt.

Imagine PHP apps out there in the cloud with access to PHP's OpenSSL API but no proper randomness. This is what we enable if we aren't careful.

This is probably why people are getting colliding 128-bit UUIDs.

The obvious solution is, "Just use PHP 7, it has a sane cross-platform and simple CSPRNG." But that defeats the purpose of a polyfill.

Ultimately, there is very little we can do if someone falls back to OpenSSL. It's the absolute last possible option, and unlike mt_rand(), might actually not always be bad (i.e. The machine uses LibreSSL instead of OpenSSL).

[tom--]

Hypothetically, imagine mcrypt_create_iv() was first choice in the search and that everyone has the mycrypt PHP ext. There are still the sandboxed cases where you end up with nothing left to choose except OpenSSL. In this case, OpenSSL is simply not safe for cryptography. In such sandboxed cases random_bytes() will throw \Exception. It doesn't attempt to resort to OpenSSL (quite right). A random_bytes() polyfill should therefore also not resort to OpenSSL.

Dropping the hypothetical, i.e. assuming mcrypt_create_iv() is not available, the only remaining argument for OpenSSL is as an alternative route to CryptGenRandom. This is the only gap the polyfill isn't filling. But I honestly don't believe it is safe. In such a case it's perhaps best to throw an exception with a message recommending to installing mcrypt.

One last point, why can't the random_bytes/int() patch in PHP 7 be backported to future point releases of PHP 5?

[tom--]

Concerning the worry of breaking BC by removing OpenSSL from random_compat, just think of it as rolling out a security patch. We are all familiar with the inconvenience of having to patch all our servers when a vulnerability is discovered. It's not necessarily a bad thing to force people into either fixing their runtime environment or else catch an Exception or silence an error.

[paragonie-scott]

Re: BC -- we just have to increment the minor version and call it 1.1.0; I'm not opposed to doing it.

[tom--]

https://www.mail-archive.com/internals@lists.php.net/msg81070.html

[paragonie-scott]

I'm going to mark this as closed again.

The only systems that will use OpenSSL meet all of these conditions:

• PHP >= 5.3.0 && PHP < 7.0.0
• No ext/mcrypt or ext/libsodium
• Non-Windows OS without access to /dev/urandom due to open_basedir and/or safe_mode restrictions

In this setup, should OpenSSL fail us, there's nothing we can do from PHP to secure it.

https://twitter.com/ircmaxell/status/653950793830297603

[tom--]

I agree completely with your analysis but not the conclusion. I think putting OpenSSL behind the random_bytes() API, which provides randomness "for use within cryptographic contexts", is ethically dubious and a wasted opportunity to educate users about their crippled PHP and what to do about it.

I say it is ethically dubious for two reasons. First there is the general feeling of mistrust towards OpenSSL I get from the various footguns its API provides and the history of injuries to feet that have been documented, at least one of which is referenced above. Evidently I'm not alone in having this concern.

But more importantly, under the conditions you set out in those three bullets, the only conditions in which random_compat uses OpenSSL, there is cause to ask if its RNG is properly seeded. But random_compat has no way to answer that at runtime. The history of randomness failures in cryptography demonstrates the importance and sensitivity of RNG seeding. Hence I believe that if there's any doubt over an RNG's seeding then is not OK to say it is suitable for use within cryptographic contexts.

The wasted opportunity is simply that random_compat could alert the user whose PHP system is not suitable for general cryptography when she tries out the package. The exception's documentation would educate the user on the nature of the problem and the options to solve it. One option might be a way to quiet the exception and use OpenSSL, a decision an educated user might reasonably make but not one random_compat can make in good conscience a priori.

[paragonie-scott]

The problem is that users on insecure platforms are more likely to go, "Oh, random_compat doesn't work. I'll just use openssl_random_pseudo_bytes()".

[narfbg]

To be honest, you could end that sentence with "... I'll just use mt_rand()" just as well, and there's no way around that. But, on a system that doesn't have crypto-safe PRNG, openssl_random_pseudo_bytes() is the closest thing you can get.

Maybe there is some middle-ground however ... I like the suggestion to optionally plug-in OpenSSL, it being the user's choice. That's tricky to do because it's a polyfill and not our own API design, but not impossible.

[SamMousa]

Well, can we really protect against outright stupidity?

It would be viable to have the user define a global constant as I suggested. the naming of that constant would then give the developer some hints about what he's doing.

const I_DO_NOT_CARE_ABOUT_SECURITY = true;

Would serve nicely, and seems unlikely to collide with any other global constant.

Other options include: const RATHER_LAZY_THAN_TIRED = true; const BAD_DEVELOPER = true;

I'm sure we can come up with some more ;-)

[GrahamCampbell]

Well, can we really protect against outright stupidity?

No. :P

[paragonie-scott]

Something like this?

if (!defined('RANDOM_COMPAT_USE_OPENSSL')) {
    define('RANDOM_COMPAT_USE_OPENSSL', false);
}
// ...
} elseif (RANDOM_COMPAT_USE_OPENSSL && extension_loaded('openssl') && PHP_VERSION_ID >= 50300) {

[tom--]

The problem is that users on insecure platforms are more likely to go, "Oh, random_compat doesn't work. I'll just use openssl_random_pseudo_bytes()".

Even if that were true, it's a better outcome, in my opinion, than providing them a random_bytes() interface to the same thing and calling it a polyfill for what PHP 7.0 provides.

[tom--]

But, on a system that doesn't have crypto-safe PRNG, openssl_random_pseudo_bytes() is the closest thing you can get.

Yes. But neither we nor random_compat can decide if it is good enough for the application.

[SamMousa]

While my previous suggestions where obviously (partially) jokes, I would prefer at least the word UNSECURE in the constant; that way it is actually obvious that it is not secure at all.

[tom--]

Well, can we really protect against outright stupidity?

No. :P

I disagree. Educating the user is what I proposed. If I didn't think education had any practical value, I wouldn't have proposed it.

[paragonie-scott]

While my previous suggestions where obviously (partially) jokes, I would prefer at least the word UNSECURE in the constant; that way it is actually obvious that it is not secure at all.

Problem: We don't specifically know that it's "not secure at all". Even before bug 70014 was fixed, it's indeterminable if the RNG was seeded or not from PHP.

Is its reputation dodgey? Yes.

Is it a userspace CSPRNG which is generally a bad idea? Yes.

But does it provide adequate randomness for PHP developers? I don't know. But I can't say "no" for certain.

[SamMousa]

@tom-- I was referring to the people that refuse to read the error message you propose and are in this category:

The problem is that users on insecure platforms are more likely to go, "Oh, random_compat doesn't work. I'll just use openssl_random_pseudo_bytes()".

I am in favor of exception if the constant is not set (education) and otherwise accepting that if they refuse to be educated and set the appropriate constant using insecure openSSL.

[narfbg]

But, on a system that doesn't have crypto-safe PRNG, openssl_random_pseudo_bytes() is the closest thing you can get.

Yes. But neither we nor random_compat can decide if it is good enough for the application.

I wasn't suggesting otherwise.

In fact, if we go for using a constant to enable OpenSSL, I'd prefer to keep it undocumented. ;)

[paragonie-scott]

Just so we're clear: If we do disable or remove OpenSSL, it won't be in 1.0.6. Ping @ircmaxell

[SamMousa]

@paragonie-scott We can go for POSSIBLY_INSECURE?

I mean it is not about being right as much as it is about being safe in my opinion.

[tom--]

@paragonie-scott

Something like this?

That looks fine. There are various possibilities.

For me the more important thing is the exception and its documentation, which could say something to the effect that if you don't need a trustworthy CSPRNG, openssl_random_pseudo_bytes() might be worth considering.

I volunteer to author a docblock for the exception if you like.

[paragonie-scott]

I've reopened #51 if you want to discuss it there, and reference it in any PRs.

[tom--]

OK. I'll make a proposal via PR.

[tom--]

@paragonie-scott

Just so we're clear: If we do disable or remove OpenSSL, it won't be in 1.0.6.

That's fine.