Bug #71915 [Asn]: openssl_random_pseudo_bytes is not "fork-safe"

Edit report at https://bugs.php.net/bug.php?id=71915&edit=1 ID: 71915 Updated by: bukka@php.net Reported by: mathieuk at gmail dot com Summary: openssl_random_pseudo_bytes is not "fork-safe" Status: Assigned Type: Bug Package: OpenSSL related Operating System: any PHP Version: 5.6.19 Assigned To: bukka Block user comment: N Private report: N New Comment: I have been thinking about it and just came up with a bit different PR ( https://github.com/php/php-src/pull/1857 ). It just adds time before each RAND_bytes which is sort of what OpenSSL 1.1 does. It means it adds a time to the entropy for OpenSSL 0.9.x and 1.0.x. Both of them adds already PID so there shouldn't be a need to add it again. I think that this is a more in line of what OpenSSL does. It also doesn't require any extra lib dependency or adding globals. Previous Comments: ------------------------------------------------------------------------ [2016-04-04 09:16:20] mathieuk at gmail dot com Sorry bout that RAND_bytes() source link, I thought I linked to the 1.0.1 branch version of md_rand.c but that link seems to go to master indeed. For me it boils down to this: * Using non-easily guessable seeds (/dev/urandom realistically being the best we have) seems better than ones that are (pid/time), even if this information doesn't seem easily (ab)usable at this time: ** PIDs are easily guessable ** Time is guessable, a machine's time may be reset, a website may even purposefully tell you the time it created a blob of bytes etc. * OpenSSL itself suggests using /dev/urandom over RAND_bytes() and only offers pid/time as mitigations if that's not an option. * RAND_poll() seems like an easy (cross-OS) way to mix in bytes from /dev/urandom or similar. So, sure, time would help but given that we have an easily accessible way to create better randomness than pid/time, it seems like the right way to go to help the people who use openssl_random_pseudo_bytes() today. That's about all I can give you on the matter :) ------------------------------------------------------------------------ [2016-04-03 18:27:27] bukka@php.net > RAND_poll(): > https://github.com/openssl/openssl/blob/OpenSSL_1_0_1-stable/crypto/rand/rand_unix.c#L411-L417 Of course I meant just RAND_bytes as RAND_poll is irrelevant here (it's not called after fork). > RAND_bytes(): > https://github.com/openssl/openssl/blob/bfd53c32cd840ed381ba557c4de8f21e3615655c/crypto/rand/md_rand.c#L538-L552…‚g > ž˨â]•Lh Yeah I saw this but it's just in master (meaning it won't be available before 1.1) so it's not used with PHP for sure. I just don't get why you say that mixing time and PID is not safe. Of course PID alone is an issue, but actually mixing it with time seems fine and it is also mentioned so in the last paragraph on OpenSSL wiki that you linked: https://wiki.openssl.org/index.php/Random_fork-safety . So I will ask once again. What issue do you see with adding pid + time to entropy pool? ------------------------------------------------------------------------ [2016-03-31 20:46:01] mathieuk at gmail dot com > The perf hit that you say is actually limited for fpm quite considerably because you have got a > pool of processes so there is not so much forking and one process is used for many requests. In case > of prefork, I'm not sure if people that use it actually care about perf :) That'd mean that for FPM you'd still get somewhat predictable bytes that way, especially if pid doesn't change much. This really should be done on a per request basis. Which brings us back to the perf hit :). > I'm wondering where you see that OpenSSL adds time to the rand pool. I checked code and > the only version that does so is current master (1.1 that is not supported by PHP yet). All others > add only PID. I might have missed it thought. Could you point me to the code where it is added From OpenSSL 1.0.1: RAND_poll(): https://github.com/openssl/openssl/blob/OpenSSL_1_0_1-stable/crypto/rand/rand_unix.c#L411-L417 RAND_bytes(): https://github.com/openssl/openssl/blob/bfd53c32cd840ed381ba557c4de8f21e3615655c/crypto/rand/md_rand.c#L538-L552 OpenSSL also adds the bytes in the uninitialised( but alloc'ed ) buffer to RAND_bytes() to its mix: https://github.com/openssl/openssl/blob/OpenSSL_1_0_1-stable/crypto/rand/md_rand.c#L490. However, in the case of Debian (Wheezy) that is commented out. Even if it weren't, memory contents may not be unguessable. > and could you point me to some article that explains why adding time doesn't help either. > I'm just curious... Time and pid are guessable or simply iterated through, leading to the possibility of duplicates or reproduced values, as shown by my sample scripts. It probably wouldn't be trivial to abuse in the real world, but there's a lot of potential use-cases. > The Rand is not used just in openssl_random_pseudo_bytes but also in private key generation (if > you add partial parameters) and sealing as well so there are few more places but that could be > addressed of course in your patch too. I imagined there would be other places, I'll take a look. > There is actually another thing on pthread_atfork that wasn't mentioned. OpenSSL would now > require linking of libpthread which should be probably checked with packagers and RM's as > it's a new lib dependency for openssl ext (especially if that should go to bug fixing release) I would recon that a bugfixing release would be preferable for a change like this. ------------------------------------------------------------------------ [2016-03-31 19:54:02] bukka@php.net I'm still thinking (read didn't have time to think about it much yet :) ) what would be the best solution and will try to play with both patches later when I have time. Here are just few thoughts. - The perf hit that you say is actually limited for fpm quite considerably because you have got a pool of processes so there is not so much forking and one process is used for many requests. In case of prefork, I'm not sure if people that use it actually care about perf :) - I'm wondering where you see that OpenSSL adds time to the rand pool. I checked code and the only version that does so is current master (1.1 that is not supported by PHP yet). All others add only PID. I might have missed it thought. Could you point me to the code where it is added and could you point me to some article that explains why adding time doesn't help either. I'm just curious... - The Rand is not used just in openssl_random_pseudo_bytes but also in private key generation (if you add partial parameters) and sealing as well so there are few more places but that could be addressed of course in your patch too. - There is actually another thing on pthread_atfork that wasn't mentioned. OpenSSL would now require linking of libpthread which should be probably checked with packagers and RM's as it's a new lib dependency for openssl ext (especially if that should go to bug fixing release) I will try to send an update when I get a bit more time to look into it. ------------------------------------------------------------------------ [2016-03-31 13:21:55] mathieuk at gmail dot com @bukka Any idea yet on what direction you'd like to go? To follow up on some things: People are asking whether just reseeding on fork() isn't the way to go. @krakjoe even implemented [a POC PR](https://github.com/php/php-src/pull/1844) - although it's with PID and time (which doesn't help). I have some doubts about whether we are able to register that pthread_atfork callback soon enough for all SAPIs for it to be usable. Even if we are, that means that all requests are now getting a perf-hit regardless of if you even use openssl. That's something I tried to prevent in my PR. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=71915 -- Edit this bug report at https://bugs.php.net/bug.php?id=71915&edit=1