Thread: OpenSSL randomness seeding
After forking we call RAND_cleanup in fork_process.c to force a re-seed to
ensure that two backends cannot share sequence. OpenSSL 1.1.0 deprecated
RAND_cleanup, and contrary to how they usually leave deprecated APIs working
until removed, they decided to silently make this call a noop like below:
# define RAND_cleanup() while(0) continue
This leaves our defence against pool sharing seemingly useless, and also
against the recommendations of OpenSSL for versions > 1.1.0 and < 1.1.1 where
the RNG was rewritten:
https://wiki.openssl.org/index.php/Random_fork-safety
The silver lining here is that while OpenSSL nooped RAND_cleanup, they also
changed what is mixed into seeding so we are still not sharing a sequence. To
fix this, changing the RAND_cleanup call to RAND_poll should be enough to
ensure re-seeding after forking across all supported OpenSSL versions. Patch
0001 implements this along with a comment referencing when it can be removed
(which most likely won't be for quite some time).
Another thing that stood out when reviewing this code is that we optimize for
RAND_poll failing in pg_strong_random, when we already have RAND_status
checking for a sufficiently seeded RNG for us. ISTM that we can simplify the
code by letting RAND_status do the work as per 0002, and also (while unlikely)
survive any transient failures in RAND_poll by allowing all the retries we've
defined for the loop.
Also, as a disclaimer, this was brought up with the PostgreSQL security team
first whom have given permission to discuss this in public.
Thoughts on these?
cheers ./daniel
--
VMware
Attachment
On 7/21/20 8:13 AM, Daniel Gustafsson wrote: > After forking we call RAND_cleanup in fork_process.c to force a re-seed to > ensure that two backends cannot share sequence. OpenSSL 1.1.0 deprecated > RAND_cleanup, and contrary to how they usually leave deprecated APIs working > until removed, they decided to silently make this call a noop like below: > > # define RAND_cleanup() while(0) continue > > This leaves our defence against pool sharing seemingly useless, and also > against the recommendations of OpenSSL for versions > 1.1.0 and < 1.1.1 where > the RNG was rewritten: > > https://wiki.openssl.org/index.php/Random_fork-safety > > The silver lining here is that while OpenSSL nooped RAND_cleanup, they also > changed what is mixed into seeding so we are still not sharing a sequence. To > fix this, changing the RAND_cleanup call to RAND_poll should be enough to > ensure re-seeding after forking across all supported OpenSSL versions. Patch > 0001 implements this along with a comment referencing when it can be removed > (which most likely won't be for quite some time). This looks reasonable to me based on your explanation and the OpenSSL wiki. > Another thing that stood out when reviewing this code is that we optimize for > RAND_poll failing in pg_strong_random, when we already have RAND_status > checking for a sufficiently seeded RNG for us. ISTM that we can simplify the > code by letting RAND_status do the work as per 0002, and also (while unlikely) > survive any transient failures in RAND_poll by allowing all the retries we've > defined for the loop. I wonder how effective the retries are going to be if they happen immediately. However, most of the code paths I followed ended in a hard error when pg_strong_random() failed so it may not hurt to try. I just worry that some caller is depending on a faster failure here. Regards, -- -David david@pgmasters.net
> On 21 Jul 2020, at 17:31, David Steele <david@pgmasters.net> wrote: > On 7/21/20 8:13 AM, Daniel Gustafsson wrote: >> Another thing that stood out when reviewing this code is that we optimize for >> RAND_poll failing in pg_strong_random, when we already have RAND_status >> checking for a sufficiently seeded RNG for us. ISTM that we can simplify the >> code by letting RAND_status do the work as per 0002, and also (while unlikely) >> survive any transient failures in RAND_poll by allowing all the retries we've >> defined for the loop. > > I wonder how effective the retries are going to be if they happen immediately. However, most of the code paths I followedended in a hard error when pg_strong_random() failed so it may not hurt to try. I just worry that some caller isdepending on a faster failure here. There is that, but I'm not convinced that relying on specific timing for anything RNG or similarly cryptographic-related is especially sane. cheers ./daniel
On 7/21/20 3:44 PM, Daniel Gustafsson wrote: >> On 21 Jul 2020, at 17:31, David Steele <david@pgmasters.net> wrote: >> On 7/21/20 8:13 AM, Daniel Gustafsson wrote: > >>> Another thing that stood out when reviewing this code is that we optimize for >>> RAND_poll failing in pg_strong_random, when we already have RAND_status >>> checking for a sufficiently seeded RNG for us. ISTM that we can simplify the >>> code by letting RAND_status do the work as per 0002, and also (while unlikely) >>> survive any transient failures in RAND_poll by allowing all the retries we've >>> defined for the loop. >> >> I wonder how effective the retries are going to be if they happen immediately. However, most of the code paths I followedended in a hard error when pg_strong_random() failed so it may not hurt to try. I just worry that some caller isdepending on a faster failure here. > > There is that, but I'm not convinced that relying on specific timing for > anything RNG or similarly cryptographic-related is especially sane. I wasn't thinking specific timing -- just that the caller might be expecting it to give up quickly if it doesn't work. That's what the code is trying to do and I wonder if there is a reason for it. But you are probably correct and I'm just overthinking it. Regards, -- -David david@pgmasters.net
> On 21 Jul 2020, at 22:00, David Steele <david@pgmasters.net> wrote: > > On 7/21/20 3:44 PM, Daniel Gustafsson wrote: >>> On 21 Jul 2020, at 17:31, David Steele <david@pgmasters.net> wrote: >>> On 7/21/20 8:13 AM, Daniel Gustafsson wrote: >>>> Another thing that stood out when reviewing this code is that we optimize for >>>> RAND_poll failing in pg_strong_random, when we already have RAND_status >>>> checking for a sufficiently seeded RNG for us. ISTM that we can simplify the >>>> code by letting RAND_status do the work as per 0002, and also (while unlikely) >>>> survive any transient failures in RAND_poll by allowing all the retries we've >>>> defined for the loop. >>> >>> I wonder how effective the retries are going to be if they happen immediately. However, most of the code paths I followedended in a hard error when pg_strong_random() failed so it may not hurt to try. I just worry that some caller isdepending on a faster failure here. >> There is that, but I'm not convinced that relying on specific timing for >> anything RNG or similarly cryptographic-related is especially sane. > > I wasn't thinking specific timing -- just that the caller might be expecting it to give up quickly if it doesn't work.That's what the code is trying to do and I wonder if there is a reason for it. I think the original intention was to handle older OpenSSL versions where multiple successful RAND_poll calls were required for RAND_status to succeed, the check working as an optimization since a failing RAND_poll would render all efforts useless anyway. I'm not sure this is true for the OpenSSL versions we support in HEAD, and/or for modern platforms, but without proof of it not being useful I would opt for keeping it. cheers ./daniel
On Tue, Jul 21, 2020 at 10:36:53PM +0200, Daniel Gustafsson wrote: > I think the original intention was to handle older OpenSSL versions where > multiple successful RAND_poll calls were required for RAND_status to succeed, > the check working as an optimization since a failing RAND_poll would render all > efforts useless anyway. I'm not sure this is true for the OpenSSL versions we > support in HEAD, and/or for modern platforms, but without proof of it not being > useful I would opt for keeping it. Yeah, the retry loop refers to this part of the past discussion on the matter: https://www.postgresql.org/message-id/CAEZATCWYs6rAp36VKm4W7Sb3EF_7tNcRuhcnJC1P8=8W9nBm9w@mail.gmail.com During the rewrite of the RNG engines, there was also a retry logic introduced in 75e2c87, then removed in c16de9d for 1.1.1. In short, we may be able to live without that once we cut more support for OpenSSL versions (minimum version support of 1.1.1 is a couple of years ahead at least for us), but I see no reasons to not leave that in place either. And this visibly solved one problem for us. I don't see either a reason to not simplify the loop to fall back to RAND_status() for the validation. In short, the proposed patch set looks like a good idea to me to stick with the recommendations of upstream's wiki to use RAND_poll() after a fork, but only do that on HEAD (OpenSSL 1.1.0 mixes the current timestamp and the PID in the random seed of the default engine, 1.0.2 only the PID but RAND_cleanup is a no-op only after 1.1.0). -- Michael
Attachment
On Tue, Jul 21, 2020 at 02:13:32PM +0200, Daniel Gustafsson wrote: > The silver lining here is that while OpenSSL nooped RAND_cleanup, they also > changed what is mixed into seeding so we are still not sharing a sequence. To > fix this, changing the RAND_cleanup call to RAND_poll should be enough to > ensure re-seeding after forking across all supported OpenSSL versions. Patch > 0001 implements this along with a comment referencing when it can be removed > (which most likely won't be for quite some time). > > Another thing that stood out when reviewing this code is that we optimize for > RAND_poll failing in pg_strong_random, when we already have RAND_status > checking for a sufficiently seeded RNG for us. ISTM that we can simplify the > code by letting RAND_status do the work as per 0002, and also (while unlikely) > survive any transient failures in RAND_poll by allowing all the retries we've > defined for the loop. > Thoughts on these? These look good. I'll push them on Saturday or later. I wondered whether to do both RAND_cleanup() and RAND_poll(), to purge all traces of the old seed on versions supporting both. Since that would strictly (albeit negligibly) increase seed predictability, I like your decision here. Do you happen to know how OpenSSL 1.1.1 changed its reaction to forks in order to make the RAND_poll() superfluous? (No need to research it if you don't.)
On Tue, Jul 21, 2020 at 10:00:20PM -0700, Noah Misch wrote: > These look good. I'll push them on Saturday or later. I wondered whether to > do both RAND_cleanup() and RAND_poll(), to purge all traces of the old seed on > versions supporting both. Since that would strictly (albeit negligibly) > increase seed predictability, I like your decision here. Thanks Noah for taking care of it. No plans for a backpatch, right? -- Michael
Attachment
> On 22 Jul 2020, at 07:00, Noah Misch <noah@leadboat.com> wrote: > > On Tue, Jul 21, 2020 at 02:13:32PM +0200, Daniel Gustafsson wrote: >> The silver lining here is that while OpenSSL nooped RAND_cleanup, they also >> changed what is mixed into seeding so we are still not sharing a sequence. To >> fix this, changing the RAND_cleanup call to RAND_poll should be enough to >> ensure re-seeding after forking across all supported OpenSSL versions. Patch >> 0001 implements this along with a comment referencing when it can be removed >> (which most likely won't be for quite some time). >> >> Another thing that stood out when reviewing this code is that we optimize for >> RAND_poll failing in pg_strong_random, when we already have RAND_status >> checking for a sufficiently seeded RNG for us. ISTM that we can simplify the >> code by letting RAND_status do the work as per 0002, and also (while unlikely) >> survive any transient failures in RAND_poll by allowing all the retries we've >> defined for the loop. > >> Thoughts on these? > > These look good. I'll push them on Saturday or later. Thanks for picking it up! > I wondered whether to > do both RAND_cleanup() and RAND_poll(), to purge all traces of the old seed on > versions supporting both. Since that would strictly (albeit negligibly) > increase seed predictability, I like your decision here. That's a good question. I believe that if one actually do use RAND_cleanup as a re-seeding mechanism then that can break FIPS enabled OpenSSL installations as RAND_cleanup resets the RNG method from the FIPS RNG to the built-in one. I would be inclined to follow the upstream recommendations of using RAND_poll exclusively, but I'm far from an expert here. > Do you happen to know how OpenSSL 1.1.1 changed its reaction to forks in order > to make the RAND_poll() superfluous? (No need to research it if you don't.) I'm not entirely sure, but I do believe that 1.1.1 ported over the RNG from the FIPS module which re-seeds itself with fork() protection. There was however a bug, fixed in 1.1.1d or thereabouts as CVE-2019-1549, where the fork protection wasn't activated by default.. so there is that. Since that bug was found, there has been tests introduced to catch any regression on that which is comforting. cheers ./daniel
On Wed, Jul 22, 2020 at 11:31:38PM +0200, Daniel Gustafsson wrote: > Thanks for picking it up! For the archives, the patch set has been applied as ce4939f and 15e4419 on HEAD. Thanks, Noah. > That's a good question. I believe that if one actually do use RAND_cleanup as > a re-seeding mechanism then that can break FIPS enabled OpenSSL installations > as RAND_cleanup resets the RNG method from the FIPS RNG to the built-in one. I > would be inclined to follow the upstream recommendations of using RAND_poll > exclusively, but I'm far from an expert here. RAND_cleanup() can cause a failure telling that the RNG state is not initialized when attempting to use FIPS in 1.0.2. This is not officially supported by upstream AFAIK, and those APIs have been dropped later in 1.1.0. And FWIW, VMware's Photon actually applies some custom patches in this area: https://github.com/vmware/photon/tree/master/SPECS/openssl openssl-drbg-default-read-system-fips.patch is used to enforce the initialization state of FIPS for example. Anyway, I would just stick with the wiki recommendation. >> Do you happen to know how OpenSSL 1.1.1 changed its reaction to forks in order >> to make the RAND_poll() superfluous? (No need to research it if you don't.) > > I'm not entirely sure, but I do believe that 1.1.1 ported over the RNG from the > FIPS module which re-seeds itself with fork() protection. There was however a > bug, fixed in 1.1.1d or thereabouts as CVE-2019-1549, where the fork protection > wasn't activated by default.. so there is that. Since that bug was found, > there has been tests introduced to catch any regression on that which is > comforting. No idea about this one actually. -- Michael
Attachment
> On 26 Jul 2020, at 09:06, Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Jul 22, 2020 at 11:31:38PM +0200, Daniel Gustafsson wrote: >> Thanks for picking it up! > > For the archives, the patch set has been applied as ce4939f and > 15e4419 on HEAD. Thanks, Noah. Indeed, thanks! >>> Do you happen to know how OpenSSL 1.1.1 changed its reaction to forks in order >>> to make the RAND_poll() superfluous? (No need to research it if you don't.) >> >> I'm not entirely sure, but I do believe that 1.1.1 ported over the RNG from the >> FIPS module which re-seeds itself with fork() protection. There was however a >> bug, fixed in 1.1.1d or thereabouts as CVE-2019-1549, where the fork protection >> wasn't activated by default.. so there is that. Since that bug was found, >> there has been tests introduced to catch any regression on that which is >> comforting. > > No idea about this one actually. I did some more reading and AFAICT it won't be required in 1.1.1+, but it also won't cause any harm so unless evidence of the latter emerge we may just as well leave it as an extra safeguard. Somewhat on topic though, 1.1.1 adds a RAND_priv_bytes function for random numbers that are supposed to be private and extra protected via it's own DRBG. Maybe we should use that for SCRAM salts etc in case we detect 1.1.1? cheers ./daniel
On Thu, Jul 30, 2020 at 11:42:16PM +0200, Daniel Gustafsson wrote: > Somewhat on topic though, 1.1.1 adds a RAND_priv_bytes function for random > numbers that are supposed to be private and extra protected via it's own DRBG. > Maybe we should use that for SCRAM salts etc in case we detect 1.1.1? Maybe. Would you have a separate pg_private_random() function, or just use RAND_priv_bytes() for pg_strong_random()? No pg_strong_random() caller is clearly disinterested in privacy; gen_random_uuid() may come closest.
On Sat, Aug 01, 2020 at 11:48:23PM -0700, Noah Misch wrote: > On Thu, Jul 30, 2020 at 11:42:16PM +0200, Daniel Gustafsson wrote: >> Somewhat on topic though, 1.1.1 adds a RAND_priv_bytes function for random >> numbers that are supposed to be private and extra protected via it's own DRBG. >> Maybe we should use that for SCRAM salts etc in case we detect 1.1.1? > > Maybe. Would you have a separate pg_private_random() function, or just use > RAND_priv_bytes() for pg_strong_random()? No pg_strong_random() caller is > clearly disinterested in privacy; gen_random_uuid() may come closest. FWIW, I am not sure that we need extra level of complexity when it comes to random number generation, so having only one API to rule them all sounds sensible to me, particularly if we know that the API used has more private protections. -- Michael
Attachment
> On 2 Aug 2020, at 09:05, Michael Paquier <michael@paquier.xyz> wrote: > > On Sat, Aug 01, 2020 at 11:48:23PM -0700, Noah Misch wrote: >> On Thu, Jul 30, 2020 at 11:42:16PM +0200, Daniel Gustafsson wrote: >>> Somewhat on topic though, 1.1.1 adds a RAND_priv_bytes function for random >>> numbers that are supposed to be private and extra protected via it's own DRBG. >>> Maybe we should use that for SCRAM salts etc in case we detect 1.1.1? >> >> Maybe. Would you have a separate pg_private_random() function, or just use >> RAND_priv_bytes() for pg_strong_random()? No pg_strong_random() caller is >> clearly disinterested in privacy; gen_random_uuid() may come closest. > > FWIW, I am not sure that we need extra level of complexity when it > comes to random number generation, so having only one API to rule them > all sounds sensible to me, particularly if we know that the API used > has more private protections. I would agree with that, especially since we might not be able to provide an equivalent implementation of a pg_private_random() function in non-OpenSSL builds. Will do a bit more reading and poking and post a patch. cheers ./daniel