Edit report at https://bugs.php.net/bug.php?id=71915&edit=1
ID: 71915
User updated by: mathieuk at gmail dot com
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:
> 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.
Previous Comments:
------------------------------------------------------------------------
[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.
------------------------------------------------------------------------
[2016-03-29 20:42:22] bukka@php.net
Ah I see what's the issue - finally read the links... :). Reading and thinking about it a bit
more, the RAND_poll is probably reasonable. It would be good to know if there is any visible slow
down though. Not that it would be a blocker but rather to see if we should have also consider some
other solutions like addressing that in the SAPI's possibly (which might not be a great idea
though). Anyway I will have to think about it a bit more so will leave this for a couple of days.
------------------------------------------------------------------------
[2016-03-29 19:33:24] mathieuk at gmail dot com
I've moved it to https://github.com/php/php-src/pull/1843
@bukka Seeding with PID and time already happens after each call to RAND_bytes() and is what caused
this behaviour in the first place. Apache's mod_ssl calls RAND_seed() with a few bytes from
/dev/urandom (or egd) for each request - we could do that instead, but it seems like that's
pretty much what RAND_poll() does. What are you seeing as the heavy part of RAND_poll?
@krakjoe Right. This would work fine in mod_php, but maybe not FPM. I could force the bool to false
in PHP_RINIT_FUNCTION(openssl). Think that'd work better?
------------------------------------------------------------------------
[2016-03-29 19:09:22] bukka@php.net
Yeah there seems to be missing global init which should reset the value for each request
(PHP_GINIT_FUNCTION). Then it should be fine and the entropy would be added for the first call of
the openssl_random_pseudo_bytes in the request. It means it can be added multiple time if the same
process is used for more requests (fpm) which I'm not sure is ideal but might not be an issue.
Again I might be wrong.
------------------------------------------------------------------------
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