not fork-safe if pids wrap (was Re: DLL hell)
On 8/15/13 11:51 PM, Patrick Pelletier wrote: On Aug 15, 2013, at 10:38 PM, Nico Williams wrote: Hmm, I've only read the article linked from there: http://android-developers.blogspot.com/2013/08/some-securerandom-thoughts.html Yeah, that's the only place I've seen it, and then the Google+ thread I linked to is essentially the comment area for that post. We (meaning those of us commenting in the thread) haven't gotten any official answer from Google, but Nikolay Elenkov has been very helpful in reconstructing what seems to be happening. We've exchanged a few more posts this evening, and it appears that what's happening is that OpenSSL is correctly self-seeding when system_server starts, but then system_server forks (without execing) to start multiple processes, and these processes are producing the same random sequence. It's not yet entirely clear why, since the OpenSSL source code looks like it's trying to be fork-safe, but it appears that somehow in practice it's not succeeding. s/system_server/zygote/ So, it appears that the problem is that since OpenSSL merely mixes in the pid to the existing random state, once the pids wrap, you will have had two processes that have generated the exact same random sequence, since they started with the same state (before the fork) and mixed in the same thing (the pid) after the fork, resulting in the same output. (This is in contrast to the approach of comparing the old and new pids, and doing a full reseed from /dev/urandom if they differ, which is what is done by Nick Mathewson's preliminary but already excellent-looking libottery.) Nikolay Elenkov wrote a proof-of-concept that shows the pid-wrapping bug on Android, and then I took it one step further and wrote a proof-of-concept using OpenSSL in C, demonstrating that this is an underlying OpenSSL bug: https://gist.github.com/ppelleti/6290984 An easy way to work around this, if you don't mind linking against pthreads, is to do this at the start of your application, after initializing OpenSSL: typedef void (*voidfunc) (void); if (ENGINE_get_default_RAND () == NULL) pthread_atfork (NULL, (voidfunc) RAND_poll, (voidfunc) RAND_poll); But, of course, this ought to eventually be fixed in OpenSSL itself. (By using the pid-comparison trick that libottery uses, rather than just mixing in the pid.) I'm happy to submit a patch, if we think there's a good chance it would be considered? --Patrick __ OpenSSL Project http://www.openssl.org User Support Mailing Listopenssl-users@openssl.org Automated List Manager majord...@openssl.org
Re: not fork-safe if pids wrap (was Re: DLL hell)
On 8/21/13 12:19 AM, Patrick Pelletier wrote: Nikolay Elenkov wrote a proof-of-concept that shows the pid-wrapping bug on Android, and then I took it one step further and wrote a proof-of-concept using OpenSSL in C, demonstrating that this is an underlying OpenSSL bug: https://gist.github.com/ppelleti/6290984 Hmmm... so I'm able to reproduce the bug with my little program when using the version of OpenSSL that ships with Ubuntu 12.04 (OpenSSL 1.0.1 14 Mar 2012). But I just built an OpenSSL off the tip of master in github (OpenSSL 1.1.0-dev xx XXX ), and my test program doesn't produce any duplicate random numbers when linked against that OpenSSL. So, this would suggest the bug has already been fixed, but I'm not sure how, since md_rand.c hasn't been changed since 2011, and it's still doing the same pid-mixing trick. Anybody else have any observations or thoughts on this? --Patrick __ OpenSSL Project http://www.openssl.org User Support Mailing Listopenssl-users@openssl.org Automated List Manager majord...@openssl.org
Re: not fork-safe if pids wrap (was Re: DLL hell)
On 21 August 2013 03:19, Patrick Pelletier c...@funwithsoftware.org wrote: On 8/15/13 11:51 PM, Patrick Pelletier wrote: On Aug 15, 2013, at 10:38 PM, Nico Williams wrote: Hmm, I've only read the article linked from there: http://android-developers.**blogspot.com/2013/08/some-** securerandom-thoughts.htmlhttp://android-developers.blogspot.com/2013/08/some-securerandom-thoughts.html Yeah, that's the only place I've seen it, and then the Google+ thread I linked to is essentially the comment area for that post. We (meaning those of us commenting in the thread) haven't gotten any official answer from Google, but Nikolay Elenkov has been very helpful in reconstructing what seems to be happening. We've exchanged a few more posts this evening, and it appears that what's happening is that OpenSSL is correctly self-seeding when system_server starts, but then system_server forks (without execing) to start multiple processes, and these processes are producing the same random sequence. It's not yet entirely clear why, since the OpenSSL source code looks like it's trying to be fork-safe, but it appears that somehow in practice it's not succeeding. s/system_server/zygote/ So, it appears that the problem is that since OpenSSL merely mixes in the pid to the existing random state, once the pids wrap, you will have had two processes that have generated the exact same random sequence, since they started with the same state (before the fork) and mixed in the same thing (the pid) after the fork, resulting in the same output. (This is in contrast to the approach of comparing the old and new pids, and doing a full reseed from /dev/urandom if they differ, which is what is done by Nick Mathewson's preliminary but already excellent-looking libottery.) Nikolay Elenkov wrote a proof-of-concept that shows the pid-wrapping bug on Android, and then I took it one step further and wrote a proof-of-concept using OpenSSL in C, demonstrating that this is an underlying OpenSSL bug: https://gist.github.com/**ppelleti/6290984https://gist.github.com/ppelleti/6290984 An easy way to work around this, if you don't mind linking against pthreads, is to do this at the start of your application, after initializing OpenSSL: typedef void (*voidfunc) (void); if (ENGINE_get_default_RAND () == NULL) pthread_atfork (NULL, (voidfunc) RAND_poll, (voidfunc) RAND_poll); But, of course, this ought to eventually be fixed in OpenSSL itself. (By using the pid-comparison trick that libottery uses, rather than just mixing in the pid.) I'm happy to submit a patch, if we think there's a good chance it would be considered? Something needs to be done, but won't this re-introduce the problem of /dev/random starvation, leading to more use of /dev/urandom (on platforms where this is a problem)? Mixing in the time seems like a safer solution that should also fix the problem. Possibly only when the PID changes. --Patrick __**__**__ OpenSSL Project http://www.openssl.org User Support Mailing Listopenssl-users@openssl.org Automated List Manager majord...@openssl.org
Re: not fork-safe if pids wrap (was Re: DLL hell)
On Wed, Aug 21, 2013 at 2:19 AM, Patrick Pelletier c...@funwithsoftware.org wrote: An easy way to work around this, if you don't mind linking against pthreads, is to do this at the start of your application, after initializing OpenSSL: typedef void (*voidfunc) (void); if (ENGINE_get_default_RAND () == NULL) pthread_atfork (NULL, (voidfunc) RAND_poll, (voidfunc) RAND_poll); This is a pretty standard thing to do, and Solaris' libpkcs11 does it (not to add entropy but to re-initialize, since PKCS#11 requires all session and object handles to no longer be usable on the child-side of fork()). But, of course, this ought to eventually be fixed in OpenSSL itself. (By using the pid-comparison trick that libottery uses, rather than just mixing in the pid.) I'm happy to submit a patch, if we think there's a good chance it would be considered? OpenSSL should use pthread_atfork() and mix in more /dev/urandom into its pool in the child-side of the fork(), Only a child-side handler is needed, FYI, unless there's locks to acquire and release, in which case you also need a pre-fork and parent-side handlers, or unless fork() is just a good excuse to add entropy to the pool on the parent side anyways :) Nico -- __ OpenSSL Project http://www.openssl.org User Support Mailing Listopenssl-users@openssl.org Automated List Manager majord...@openssl.org
Re: not fork-safe if pids wrap (was Re: DLL hell)
On Wed, Aug 21, 2013 at 5:41 AM, Ben Laurie b...@links.org wrote: Something needs to be done, but won't this re-introduce the problem of /dev/random starvation, leading to more use of /dev/urandom (on platforms where this is a problem)? Mixing in the time seems like a safer solution that should also fix the problem. Possibly only when the PID changes. Stirring in time and PID seems like just a fail-safe. Some bytes from /dev/urandom should also be added -- it won't hang once seeded (or ever on Linux, but hopefully a simple service can be added by users to seed urandom from random). Provided one read from /dev/random has been done I think perturbing the pool with time + PID + urandom should suffice. Nico -- __ OpenSSL Project http://www.openssl.org User Support Mailing Listopenssl-users@openssl.org Automated List Manager majord...@openssl.org
