Thread: Move OpenSSL random under USE_OPENSSL_RANDOM

The USE_OPENSSL_RANDOM macro is defined when OpenSSL is used as a randomness
provider, but the implementation of strong randomness is guarded by USE_OPENSSL
in most places.  This is technically the same thing today, but it seems
hygienic to use the appropriate macro in case we ever want to allow OS
randomness together with OpenSSL or something similar (or just make git grep
easier which is my itch to scratch with this).

The attached moves all invocations under the correct guards.  RAND_poll() in
fork_process.c needs to happen for both OpenSSL and OpenSSL random, thus the
check for both.

cheers ./daniel

On Tue, Aug 25, 2020 at 03:52:14PM +0200, Daniel Gustafsson wrote:
> The USE_OPENSSL_RANDOM macro is defined when OpenSSL is used as a randomness
> provider, but the implementation of strong randomness is guarded by USE_OPENSSL
> in most places.  This is technically the same thing today, but it seems
> hygienic to use the appropriate macro in case we ever want to allow OS
> randomness together with OpenSSL or something similar (or just make git grep
> easier which is my itch to scratch with this).

@@ -24,7 +24,7 @@
 #include <unistd.h>
 #include <sys/time.h>

-#ifdef USE_OPENSSL
+#ifdef USE_OPENSSL_RANDOM
 #include <openssl/rand.h>
 #endif
I agree that this makes the header declarations more consistent with
WIN32.

> The attached moves all invocations under the correct guards.  RAND_poll() in
> fork_process.c needs to happen for both OpenSSL and OpenSSL random, thus the
> check for both.

Yeah, it could be possible that somebody still calls RAND_bytes() or
similar without going through pg_strong_random(), so we still need to
use USE_OPENSSL after forking.  Per this argument, I am not sure I see
the point of the change in fork_process.c as it seems to me that
USE_OPENSSL_RANDOM should only be tied to pg_strong_random.c, and
you'd still get a compilation failure if trying to use
USE_OPENSSL_RANDOM without --with-openssl.
--
Michael

> On 26 Aug 2020, at 09:56, Michael Paquier <michael@paquier.xyz> wrote:
> On Tue, Aug 25, 2020 at 03:52:14PM +0200, Daniel Gustafsson wrote:

>> The attached moves all invocations under the correct guards.  RAND_poll() in
>> fork_process.c needs to happen for both OpenSSL and OpenSSL random, thus the
>> check for both.
>
> Yeah, it could be possible that somebody still calls RAND_bytes() or
> similar without going through pg_strong_random(), so we still need to
> use USE_OPENSSL after forking.  Per this argument, I am not sure I see
> the point of the change in fork_process.c as it seems to me that
> USE_OPENSSL_RANDOM should only be tied to pg_strong_random.c, and
> you'd still get a compilation failure if trying to use
> USE_OPENSSL_RANDOM without --with-openssl.

That's certainly true.  The intention though is to make the code easier to
follow (more explicit/discoverable) for anyone trying to implement support for
TLS backends. It's a very git grep intense process already as it is.

cheers ./daniel

On Wed, Aug 26, 2020 at 2:19 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 26 Aug 2020, at 09:56, Michael Paquier <michael@paquier.xyz> wrote:
> > On Tue, Aug 25, 2020 at 03:52:14PM +0200, Daniel Gustafsson wrote:
>
> >> The attached moves all invocations under the correct guards.  RAND_poll() in
> >> fork_process.c needs to happen for both OpenSSL and OpenSSL random, thus the
> >> check for both.
> >
> > Yeah, it could be possible that somebody still calls RAND_bytes() or
> > similar without going through pg_strong_random(), so we still need to
> > use USE_OPENSSL after forking.  Per this argument, I am not sure I see
> > the point of the change in fork_process.c as it seems to me that
> > USE_OPENSSL_RANDOM should only be tied to pg_strong_random.c, and
> > you'd still get a compilation failure if trying to use
> > USE_OPENSSL_RANDOM without --with-openssl.
>
> That's certainly true.  The intention though is to make the code easier to
> follow (more explicit/discoverable) for anyone trying to implement support for

Is it really a reasonable usecase to use RAND_bytes() outside of both
pg_stroing_random() *and' outside of the openssl-specific files (like
be-secure-openssl.c)? Because it would only be those cases that would
have this case, right?

If anything, perhaps the call to RAND_poll() in fork_process.c should
actually be a call to a strong_random_initialize() or something which
would have an implementation in pg_strong_random.c, thereby isolating
the openssl specific code in there? (And with a void implementation
without openssl)

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

On Tue, Nov 03, 2020 at 10:15:48AM +0100, Magnus Hagander wrote:
> On Wed, Aug 26, 2020 at 2:19 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>> That's certainly true.  The intention though is to make the code easier to
>> follow (more explicit/discoverable) for anyone trying to implement support for
>
> Is it really a reasonable usecase to use RAND_bytes() outside of both
> pg_stroing_random() *and' outside of the openssl-specific files (like
> be-secure-openssl.c)? Because it would only be those cases that would
> have this case, right?

It does not sound that strange to me to assume if some out-of-core
code makes use of that to fetch a random set of bytes.  Now I don't
know of any code doing that.  Who knows.

> If anything, perhaps the call to RAND_poll() in fork_process.c should
> actually be a call to a strong_random_initialize() or something which
> would have an implementation in pg_strong_random.c, thereby isolating
> the openssl specific code in there? (And with a void implementation
> without openssl)

I don't think that we have any need to go to such extent just for this
case, as RAND_poll() after forking a process is irrelevant in 1.1.1.
We are still many years away from removing its support though.

No idea if other SSL implementations would require such a thing.
Daniel, what about NSS?
--
Michael

> On 3 Nov 2020, at 11:35, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Nov 03, 2020 at 10:15:48AM +0100, Magnus Hagander wrote:
>> On Wed, Aug 26, 2020 at 2:19 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>>> That's certainly true.  The intention though is to make the code easier to
>>> follow (more explicit/discoverable) for anyone trying to implement support for
>>
>> Is it really a reasonable usecase to use RAND_bytes() outside of both
>> pg_stroing_random() *and' outside of the openssl-specific files (like
>> be-secure-openssl.c)? Because it would only be those cases that would
>> have this case, right?
>
> It does not sound that strange to me to assume if some out-of-core
> code makes use of that to fetch a random set of bytes.  Now I don't
> know of any code doing that.  Who knows.

Even if there are, I doubt such codepaths will be stumped by using
USE_OPENSSL_RANDOM for pg_strong_random code as opposed to USE_OPENSSL.

>> If anything, perhaps the call to RAND_poll() in fork_process.c should
>> actually be a call to a strong_random_initialize() or something which
>> would have an implementation in pg_strong_random.c, thereby isolating
>> the openssl specific code in there? (And with a void implementation
>> without openssl)
>
> I don't think that we have any need to go to such extent just for this
> case, as RAND_poll() after forking a process is irrelevant in 1.1.1.
> We are still many years away from removing its support though.

Agreed, I doubt we'll be able to retire our <1.1.1 suppport any time soon.

> No idea if other SSL implementations would require such a thing.
> Daniel, what about NSS?

PK11_GenerateRandom in NSS requires an NSSContext to be set up after fork,
which could be performed in such an _initialize function.  The PRNG in NSPR has
a similar requirement (which may be the one the NSS patch end up using, not
sure yet).

I kind of like the idea of continuing to abstract this functionality, not
pulling in OpenSSL headers in fork_process.c is a neat bonus.  I'd say it's
worth implementing to see what it would imply, and am happy to do unless
someone beats me to it.

cheers ./daniel

On Tue, Nov 3, 2020 at 1:00 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 3 Nov 2020, at 11:35, Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Tue, Nov 03, 2020 at 10:15:48AM +0100, Magnus Hagander wrote:
> >> On Wed, Aug 26, 2020 at 2:19 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> >>> That's certainly true.  The intention though is to make the code easier to
> >>> follow (more explicit/discoverable) for anyone trying to implement support for
> >>
> >> Is it really a reasonable usecase to use RAND_bytes() outside of both
> >> pg_stroing_random() *and' outside of the openssl-specific files (like
> >> be-secure-openssl.c)? Because it would only be those cases that would
> >> have this case, right?
> >
> > It does not sound that strange to me to assume if some out-of-core
> > code makes use of that to fetch a random set of bytes.  Now I don't
> > know of any code doing that.  Who knows.
>
> Even if there are, I doubt such codepaths will be stumped by using
> USE_OPENSSL_RANDOM for pg_strong_random code as opposed to USE_OPENSSL.
>
> >> If anything, perhaps the call to RAND_poll() in fork_process.c should
> >> actually be a call to a strong_random_initialize() or something which
> >> would have an implementation in pg_strong_random.c, thereby isolating
> >> the openssl specific code in there? (And with a void implementation
> >> without openssl)
> >
> > I don't think that we have any need to go to such extent just for this
> > case, as RAND_poll() after forking a process is irrelevant in 1.1.1.
> > We are still many years away from removing its support though.
>
> Agreed, I doubt we'll be able to retire our <1.1.1 suppport any time soon.
>
> > No idea if other SSL implementations would require such a thing.
> > Daniel, what about NSS?
>
> PK11_GenerateRandom in NSS requires an NSSContext to be set up after fork,
> which could be performed in such an _initialize function.  The PRNG in NSPR has
> a similar requirement (which may be the one the NSS patch end up using, not
> sure yet).
>
> I kind of like the idea of continuing to abstract this functionality, not
> pulling in OpenSSL headers in fork_process.c is a neat bonus.  I'd say it's
> worth implementing to see what it would imply, and am happy to do unless
> someone beats me to it.

Yeah, if it's likely to be usable in the other implementations, then I
think we should definitely explore exactly what that kind of an
abstraction would imply. Anything isolating the dependency on OpenSSL
would likely have to be done at that time anyway in that case, so
better have it ready.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

On Tue, Nov 03, 2020 at 01:46:38PM +0100, Magnus Hagander wrote:
> On Tue, Nov 3, 2020 at 1:00 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>> I kind of like the idea of continuing to abstract this functionality, not
>> pulling in OpenSSL headers in fork_process.c is a neat bonus.  I'd say it's
>> worth implementing to see what it would imply, and am happy to do unless
>> someone beats me to it.
>
> Yeah, if it's likely to be usable in the other implementations, then I
> think we should definitely explore exactly what that kind of an
> abstraction would imply. Anything isolating the dependency on OpenSSL
> would likely have to be done at that time anyway in that case, so
> better have it ready.

With the NSS argument, agreed.  Documenting when this initialization
routine is used is important.  And I think that we should force to
look at this code when adding a new SSL implementation to make sure
that we never see CVE-2013-1900 again, say:
void
pg_strong_random_init(void)
{
#ifdef USE_SSL
#ifdef USE_OPENSSL
    RAND_poll();
#elif USE_NSS
    /* do the NSS initialization */
#else
    Hey, you are missing something here.
#endif
#endif
}
--
Michael

On Wed, Nov 4, 2020 at 2:01 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Nov 03, 2020 at 01:46:38PM +0100, Magnus Hagander wrote:
> > On Tue, Nov 3, 2020 at 1:00 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> >> I kind of like the idea of continuing to abstract this functionality, not
> >> pulling in OpenSSL headers in fork_process.c is a neat bonus.  I'd say it's
> >> worth implementing to see what it would imply, and am happy to do unless
> >> someone beats me to it.
> >
> > Yeah, if it's likely to be usable in the other implementations, then I
> > think we should definitely explore exactly what that kind of an
> > abstraction would imply. Anything isolating the dependency on OpenSSL
> > would likely have to be done at that time anyway in that case, so
> > better have it ready.
>
> With the NSS argument, agreed.  Documenting when this initialization
> routine is used is important.  And I think that we should force to
> look at this code when adding a new SSL implementation to make sure
> that we never see CVE-2013-1900 again, say:
> void
> pg_strong_random_init(void)
> {
> #ifdef USE_SSL
> #ifdef USE_OPENSSL
>         RAND_poll();
> #elif USE_NSS
>         /* do the NSS initialization */
> #else
>         Hey, you are missing something here.
> #endif
> #endif
> }

Yes, we should absolutely do that. We already do this for
pg_strong_random() itself, and we should definitely repeat the pattern
in the init function.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

On Wed, Nov 04, 2020 at 10:05:48AM +0100, Magnus Hagander wrote:
> Yes, we should absolutely do that. We already do this for
> pg_strong_random() itself, and we should definitely repeat the pattern
> in the init function.

This poked at my curiosity, so I looked at it.  The result looks
indeed like an improvement to me, while taking care of the point of
upthread to make the implementation stuff controlled only by
USE_OPENSSL_RANDOM.  Per se the attached.

This could make random number generation predictible when an extension
calls directly RAND_bytes() if USE_OPENSSL_RANDOM is not used while
building with OpenSSL, but perhaps I am just too much of a pessimistic
nature.
--
Michael

> On 5 Nov 2020, at 04:44, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Nov 04, 2020 at 10:05:48AM +0100, Magnus Hagander wrote:
>> Yes, we should absolutely do that. We already do this for
>> pg_strong_random() itself, and we should definitely repeat the pattern
>> in the init function.
>
> This poked at my curiosity, so I looked at it.  The result looks
> indeed like an improvement to me, while taking care of the point of
> upthread to make the implementation stuff controlled only by
> USE_OPENSSL_RANDOM.  Per se the attached.

This must check for USE_OPENSSL as well as per my original patch, since we'd
otherwise fail to perform post-fork initialization in case one use OpenSSL with
anothe PRNG for pg_strong_random.  That might be theoretical at this point, but
if we ever support that and miss updating this it would be problematic.

+#if defined(USE_OPENSSL_RANDOM)

I'm not sure this comment adds any value, we currently have two non-TLS library
PRNGs in pg_strong_random, so even if we add NSS it will at best be 50%:

+ * Note that this applies normally to SSL implementations, so when
+ * implementing a new one, be careful to consider this initialization
+ * step.

> This could make random number generation predictible when an extension
> calls directly RAND_bytes() if USE_OPENSSL_RANDOM is not used while
> building with OpenSSL, but perhaps I am just too much of a pessimistic
> nature.

Any extension blindly calling RAND_bytes and expecting there to automagically
be an OpenSSL library available seems questionable.

cheers ./daniel

On Thu, Nov 05, 2020 at 10:49:45AM +0100, Daniel Gustafsson wrote:
> This must check for USE_OPENSSL as well as per my original patch, since we'd
> otherwise fail to perform post-fork initialization in case one use OpenSSL with
> anothe PRNG for pg_strong_random.  That might be theoretical at this point, but
> if we ever support that and miss updating this it would be problematic.

That's actually the same point I tried to make at the end of my last
email, but worded differently, isn't it?  In short we have
USE_OPENSSL, but !USE_OPENSSL_RANDOM and we still need an
initialization.  We could just do something like the following:
#ifdef USE_OPENSSL
    RAND_poll();
#endif
#if defined(USE_OPENSSL_RANDOM)
    /* OpenSSL is done above, because blah.. */
#elif etc..
[...]
#error missing an init, pal.
#endif

Or do you jave something else in mind?

> +#if defined(USE_OPENSSL_RANDOM)
>
> I'm not sure this comment adds any value, we currently have two non-TLS library
> PRNGs in pg_strong_random, so even if we add NSS it will at best be 50%:

I don't mind removing this part, the compilation hint may be enough,
indeed.
--
Michael

> On 5 Nov 2020, at 13:12, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Nov 05, 2020 at 10:49:45AM +0100, Daniel Gustafsson wrote:
>> This must check for USE_OPENSSL as well as per my original patch, since we'd
>> otherwise fail to perform post-fork initialization in case one use OpenSSL with
>> anothe PRNG for pg_strong_random.  That might be theoretical at this point, but
>> if we ever support that and miss updating this it would be problematic.
>
> That's actually the same point I tried to make at the end of my last
> email, but worded differently, isn't it?

Ah, ok, then I failed to parse it that way.  At least we are in agreement then
which is good.

> In short we have
> USE_OPENSSL, but !USE_OPENSSL_RANDOM and we still need an
> initialization.  We could just do something like the following:
> #ifdef USE_OPENSSL
>    RAND_poll();
> #endif
> #if defined(USE_OPENSSL_RANDOM)
>    /* OpenSSL is done above, because blah.. */
> #elif etc..
> [...]
> #error missing an init, pal.
> #endif
>
> Or do you jave something else in mind?

What about the (hypothetical) situation where USE_OPENSSL_RANDOM is used
without USE_OPENSSL? Wouldn't the below make sure we cover all bases?

    #if defined(USE_OPENSSL) || defined(USE_OPENSSL_RANDOM)

cheers ./daniel

On Thu, Nov 05, 2020 at 01:18:15PM +0100, Daniel Gustafsson wrote:
> What about the (hypothetical) situation where USE_OPENSSL_RANDOM is used
> without USE_OPENSSL? Wouldn't the below make sure we cover all bases?

You can actually try that combination, because it is possible today to
compile without --with-openssl but try to enforce USE_OPENSSL_RANDOM.
This leads to a compilation failure.  I think that it is important to
have the #if/#elif business in the init function match the conditions
of the main function.

>     #if defined(USE_OPENSSL) || defined(USE_OPENSSL_RANDOM)

It seems to me that this one would become incorrect if compiling with
OpenSSL but select a random source that requires an initialization, as
it would enforce only OpenSSL initialization all the time.
Theoretical point now, of course, because such combination does not
exist yet in the code.
--
Michael

On Thu, Nov 5, 2020 at 1:28 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Nov 05, 2020 at 01:18:15PM +0100, Daniel Gustafsson wrote:
> > What about the (hypothetical) situation where USE_OPENSSL_RANDOM is used
> > without USE_OPENSSL? Wouldn't the below make sure we cover all bases?
>
> You can actually try that combination, because it is possible today to
> compile without --with-openssl but try to enforce USE_OPENSSL_RANDOM.
> This leads to a compilation failure.  I think that it is important to
> have the #if/#elif business in the init function match the conditions
> of the main function.

+1 -- whatever those are, they should be the same.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

> On 5 Nov 2020, at 13:28, Michael Paquier <michael@paquier.xyz> wrote:

> It seems to me that this one would become incorrect if compiling with
> OpenSSL but select a random source that requires an initialization, as
> it would enforce only OpenSSL initialization all the time.

Right, how about something like the attached (untested) diff?

> Theoretical point now, of course, because such combination does not
> exist yet in the code.

Not yet, and potentially never will.  Given the consequences of a PRNG which
hasn't been properly initialized I think it's ok to be defensive in this
codepath however.

cheers ./daniel

On Thu, Nov 05, 2020 at 01:59:11PM +0100, Daniel Gustafsson wrote:
> Not yet, and potentially never will.  Given the consequences of a PRNG which
> hasn't been properly initialized I think it's ok to be defensive in this
> codepath however.

+   /*
+    * In case the backend is using the PRNG from OpenSSL without being built
+    * with support for OpenSSL, make sure to perform post-fork initialization.
+    * If the backend is using OpenSSL then we have already performed this
+    * step. The same version caveat as discussed in the comment above applies
+    * here as well.
+    */
+#ifndef USE_OPENSSL
+   RAND_poll();
+#endif

I still don't see the point of this extra complexity, as
USE_OPENSSL_RANDOM implies USE_OPENSSL, and we also call RAND_poll() a
couple of lines down in the main function under USE_OPENSSL_RANDOM.
So I would just remove this whole block, and replace the comment by a
simple "initialization already done above".
--
Michael

> On 6 Nov 2020, at 00:36, Michael Paquier <michael@paquier.xyz> wrote:

> I still don't see the point of this extra complexity, as
> USE_OPENSSL_RANDOM implies USE_OPENSSL,

As long as we're sure that we'll remember to fix this when that assumption no
longer holds (intentional or unintentional) then it's fine to skip and instead
be defensive in documentation rather than code.

cheers ./daniel

On Fri, Nov 6, 2020 at 12:08 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 6 Nov 2020, at 00:36, Michael Paquier <michael@paquier.xyz> wrote:
>
> > I still don't see the point of this extra complexity, as
> > USE_OPENSSL_RANDOM implies USE_OPENSSL,
>
> As long as we're sure that we'll remember to fix this when that assumption no
> longer holds (intentional or unintentional) then it's fine to skip and instead
> be defensive in documentation rather than code.

I think the defensive-in-code instead of defensive-in-docs is a really
strong argument, so I have pushed it as such.


--
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

On Fri, Nov 06, 2020 at 01:31:44PM +0100, Magnus Hagander wrote:
> I think the defensive-in-code instead of defensive-in-docs is a really
> strong argument, so I have pushed it as such.

Fine by me.  Thanks for the commit.
--
Michael

Magnus Hagander <magnus@hagander.net> writes:
> I think the defensive-in-code instead of defensive-in-docs is a really
> strong argument, so I have pushed it as such.

I notice warnings that I think are caused by this patch on some buildfarm
members, eg

 drongo        | 2020-11-15 06:59:05 | C:\prog\bf\root\HEAD\pgsql.build\src\port\pg_strong_random.c(96,11): warning
C4013:'RAND_poll' undefined; assuming extern returning int [C:\prog\bf\root\HEAD\pgsql.build\postgres.vcxproj] 
 drongo        | 2020-11-15 06:59:05 | C:\prog\bf\root\HEAD\pgsql.build\src\port\pg_strong_random.c(96,11): warning
C4013:'RAND_poll' undefined; assuming extern returning int [C:\prog\bf\root\HEAD\pgsql.build\libpgport.vcxproj] 
 drongo        | 2020-11-15 06:59:05 |   C:\prog\bf\root\HEAD\pgsql.build\src\port\pg_strong_random.c(96,11): warning
C4013:'RAND_poll' undefined; assuming extern returning int [C:\prog\bf\root\HEAD\pgsql.build\postgres.vcxproj] 
 drongo        | 2020-11-15 06:59:05 |   C:\prog\bf\root\HEAD\pgsql.build\src\port\pg_strong_random.c(96,11): warning
C4013:'RAND_poll' undefined; assuming extern returning int [C:\prog\bf\root\HEAD\pgsql.build\libpgport.vcxproj] 

(bowerbird and hamerkop are showing the same).

My first thought about it was that this bit is busted:

+#ifndef USE_OPENSSL
+   RAND_poll();
+#endif

The obvious problem with this is that if !USE_OPENSSL, we will not have
pulled in openssl's headers.

However ... all these machines are pointing at line 96, which is not
that one but the one under "#if defined(USE_OPENSSL)".  So I'm not sure
what to make of that, except that a bit more finesse seems required.

            regards, tom lane

On Sun, Nov 15, 2020 at 12:16:56PM -0500, Tom Lane wrote:
> The obvious problem with this is that if !USE_OPENSSL, we will not have
> pulled in openssl's headers.

FWIW, I argued upthread against including this part because it is
useless: if not building with OpenSSL, we'll never have the base to be
able to use RAND_poll().

> However ... all these machines are pointing at line 96, which is not
> that one but the one under "#if defined(USE_OPENSSL)".  So I'm not sure
> what to make of that, except that a bit more finesse seems required.

The build scripts of src/tools/msvc/ choose to not use OpenSSL as
strong random source even if building with OpenSSL.  The top of the
file only includes openssl/rand.h if using USE_OPENSSL_RANDOM.

Thinking about that afresh, I think that we got that wrong here on
three points:
- If attempting to use OpenSSL on Windows, let's just bite the bullet
and use OpenSSL as random source, using Windows as source only when
not building with OpenSSL.
- Instead of using a call to RAND_poll() that we know will never work,
let's just issue a compilation failure if attempting to use
USE_OPENSSL_RANDOM without USE_OPENSSL.
- rand.h needs to be included under USE_OPENSSL.
--
Michael

> On 16 Nov 2020, at 01:20, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sun, Nov 15, 2020 at 12:16:56PM -0500, Tom Lane wrote:
>> The obvious problem with this is that if !USE_OPENSSL, we will not have
>> pulled in openssl's headers.
>
> FWIW, I argued upthread against including this part because it is
> useless: if not building with OpenSSL, we'll never have the base to be
> able to use RAND_poll().

How do you mean?  The OpenSSL PRNG can be used without setting up a context
etc, the code in pg_strong_random is all we need to use it without USE_OPENSSL
(whether we'd like to is another story) or am I missing something?

>> However ... all these machines are pointing at line 96, which is not
>> that one but the one under "#if defined(USE_OPENSSL)".  So I'm not sure
>> what to make of that, except that a bit more finesse seems required.
>
> The build scripts of src/tools/msvc/ choose to not use OpenSSL as
> strong random source even if building with OpenSSL.  The top of the
> file only includes openssl/rand.h if using USE_OPENSSL_RANDOM.

The fallout here is precisely the reason why I argued for belts and suspenders
such that PRNG init is performed for (USE_OPENSSL || USE_OPENSSL_RANDOM).  I
don't trust the assumption that if one is there other will always be there as
well as long as they are disjoint.  Since we expose this PRNG to users, there
is a vector for spooling the rand state via UUID generation in case the PRNG is
faulty and have predictability, so failing to protect the after-fork case can
be expensive.  Granted, such vulnerabilities are rare but not inconcievable.

Now, this patch didn't get the header inclusion right which is why thise broke.

> Thinking about that afresh, I think that we got that wrong here on
> three points:
> - If attempting to use OpenSSL on Windows, let's just bite the bullet
> and use OpenSSL as random source, using Windows as source only when
> not building with OpenSSL.
> - Instead of using a call to RAND_poll() that we know will never work,
> let's just issue a compilation failure if attempting to use
> USE_OPENSSL_RANDOM without USE_OPENSSL.

Taking a step back, what is the usecase of USE_OPENSSL_RANDOM if we force it to
be equal to USE_OPENSSL?  Shouldn't we in that case just have USE_OPENSSL,
adjust the logic and remove the below comment from configure.ac which isn't
really telling the truth?

  # Select random number source
  #
  # You can override this logic by setting the appropriate USE_*RANDOM flag to 1
  # in the template or configure command line.

I might be thick but I'm struggling to see the use for complications when we
don't support any pluggability.  Having said that, it might be the sane way in
the end to forcibly use the TLS library as a randomness source should there be
one (FIPS compliance comes to mind), but then we might as well spell that out.

> - rand.h needs to be included under USE_OPENSSL.


It needs to be included for both USE_OPENSSL and USE_OPENSSL_RANDOM unless we
combine them as per the above.

cheers ./daniel

Magnus Hagander <magnus@hagander.net> writes:
> I agree with those -- either we remove the ability to choose random source
> independently of the SSL library (and then only use the windows crypto
> provider or /dev/urandom as platform-specific choices when *no* SSL library
> is used), and in that case we should not have separate #ifdef's for them.
> Or we fix the includes. Which is obviously easier, but we should take the
> time to do what we think is right long-term of course.

FWIW, I'd vote for the former.  I think the presumption that OpenSSL's
random-number machinery can be used without any other initialization is
shaky as heck.

            regards, tom lane

> On 16 Nov 2020, at 16:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Magnus Hagander <magnus@hagander.net> writes:
>> I agree with those -- either we remove the ability to choose random source
>> independently of the SSL library (and then only use the windows crypto
>> provider or /dev/urandom as platform-specific choices when *no* SSL library
>> is used), and in that case we should not have separate #ifdef's for them.
>> Or we fix the includes. Which is obviously easier, but we should take the
>> time to do what we think is right long-term of course.
>
> FWIW, I'd vote for the former.  I think the presumption that OpenSSL's
> random-number machinery can be used without any other initialization is
> shaky as heck.

I tend to agree, randomness is complicated enough without adding a compile time
extensibility which few (if anyone) will ever use.  Attached is an attempt at
this.

cheers ./daniel

On Tue, Nov 17, 2020 at 09:24:30PM +0100, Daniel Gustafsson wrote:
> I tend to agree, randomness is complicated enough without adding a compile time
> extensibility which few (if anyone) will ever use.  Attached is an attempt at
> this.

Going down to that, it seems to me that we could just remove
USE_WIN32_RANDOM (as this is implied by WIN32), as well as
USE_DEV_URANDOM because configure.ac checks for the existence of
/dev/urandom, no?  In short, configure.ac could be changed to check
after /dev/urandom if not using OpenSSL and not being on Windows.

-elif test x"$USE_WIN32_RANDOM" = x"1" ; then
+elif test x"$PORTANME" = x"win32" ; then
Typo here, s/PORTANME/PORTNAME.
--
Michael

> On 18 Nov 2020, at 02:31, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Nov 17, 2020 at 09:24:30PM +0100, Daniel Gustafsson wrote:
>> I tend to agree, randomness is complicated enough without adding a compile time
>> extensibility which few (if anyone) will ever use.  Attached is an attempt at
>> this.
>
> Going down to that, it seems to me that we could just remove
> USE_WIN32_RANDOM (as this is implied by WIN32), as well as
> USE_DEV_URANDOM because configure.ac checks for the existence of
> /dev/urandom, no?  In short, configure.ac could be changed to check
> after /dev/urandom if not using OpenSSL and not being on Windows.

Technically that is what it does, except for setting the USE_*RANDOM variables
for non-OpenSSL builds.  We could skip that too, which I think is what you're
proposing, but it seems to me that we'll end up with another set of entangled
logic in pg_strong_random if we do since there then needs to be precedence in
checking (one might be on Windows with OpenSSL for example, where OpenSSL >
Windows API).

> -elif test x"$USE_WIN32_RANDOM" = x"1" ; then
> +elif test x"$PORTANME" = x"win32" ; then
> Typo here, s/PORTANME/PORTNAME.

Fixed.

cheers ./daniel

On Wed, Nov 18, 2020 at 09:25:44AM +0100, Daniel Gustafsson wrote:
> Technically that is what it does, except for setting the USE_*RANDOM variables
> for non-OpenSSL builds.  We could skip that too, which I think is what you're
> proposing, but it seems to me that we'll end up with another set of entangled
> logic in pg_strong_random if we do since there then needs to be precedence in
> checking (one might be on Windows with OpenSSL for example, where OpenSSL >
> Windows API).

Yes, I am suggesting to just remove both USE_*_RANDOM flags, and use
the following structure instead in pg_strong_random.c for both the
init and main functions:
#ifdef USE_OPENSSL
    /* foo */
#elif WIN32
    /* bar*/
#else
    /* hoge urandom */
#endif

And complain in configure.ac if we miss urandom for the fallback case.

Now, it would not be the first time I suggest something on this thread
that nobody likes :)
--
Michael

> On 18 Nov 2020, at 09:54, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Nov 18, 2020 at 09:25:44AM +0100, Daniel Gustafsson wrote:
>> Technically that is what it does, except for setting the USE_*RANDOM variables
>> for non-OpenSSL builds.  We could skip that too, which I think is what you're
>> proposing, but it seems to me that we'll end up with another set of entangled
>> logic in pg_strong_random if we do since there then needs to be precedence in
>> checking (one might be on Windows with OpenSSL for example, where OpenSSL >
>> Windows API).
>
> Yes, I am suggesting to just remove both USE_*_RANDOM flags, and use
> the following structure instead in pg_strong_random.c for both the
> init and main functions:
> #ifdef USE_OPENSSL
>     /* foo */
> #elif WIN32
>     /* bar*/
> #else
>     /* hoge urandom */
> #endif
>
> And complain in configure.ac if we miss urandom for the fallback case.
>
> Now, it would not be the first time I suggest something on this thread
> that nobody likes :)

While it does simplify configure.ac, I'm just not a fan of the strict ordering
which is required without the labels even implying it.  But that might just be
my personal preference.

cheers ./daniel

On Wed, Nov 18, 2020 at 10:43:35AM +0100, Daniel Gustafsson wrote:
> While it does simplify configure.ac, I'm just not a fan of the strict ordering
> which is required without the labels even implying it.  But that might just be
> my personal preference.

I just looked at that, and the attached seems more intuitive to me.
There is more code removed, but not that much either.
--
Michael

> On 19 Nov 2020, at 04:34, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Nov 18, 2020 at 10:43:35AM +0100, Daniel Gustafsson wrote:
>> While it does simplify configure.ac, I'm just not a fan of the strict ordering
>> which is required without the labels even implying it.  But that might just be
>> my personal preference.
>
> I just looked at that, and the attached seems more intuitive to me.

Ok.  I would add a strongly worded comment about the importance of the ordering
since that is now crucial not to break.

-#ifdef USE_WIN32_RANDOM
+#ifdef WIN32
 #include <wincrypt.h>
 #endif

-#ifdef USE_WIN32_RANDOM
+#ifdef WIN32
 /*
  * Cache a global crypto provider that only gets freed when the process
  * exits, in case we need random numbers more than once.
@@ -39,7 +39,7 @@
 static HCRYPTPROV hProvider = 0;
 #endif

This will pull in headers and define hProvider for all Windows builds even if
they use OpenSSL, but perhaps that doesn't matter?

cheers ./daniel

On Thu, Nov 19, 2020 at 4:34 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Nov 18, 2020 at 10:43:35AM +0100, Daniel Gustafsson wrote:
> > While it does simplify configure.ac, I'm just not a fan of the strict ordering
> > which is required without the labels even implying it.  But that might just be
> > my personal preference.
>
> I just looked at that, and the attached seems more intuitive to me.
> There is more code removed, but not that much either.

First -- your patch mi9sses the comment in front of pg_strong_random
which still says configure will set the USE_*_RANDOM macros.

That said, I agree with daniel that it's a bit "scary" that it's the
order of ifdefs that now decide on the whole thing, especially since
there are two sets of those (one in the init function, one in the
random one), which could potentially end up out of order if someone
makes a mistake.

I'm thinking the code might get a lot cleaner if we just make a single
set of ifdefs, even if that means repeating the function header. In
theory we could put them in different *.c files as well, but that
seems overkill given how tiny they are.

Patch is the same as your v3 in all parts except for the
pg_strong_random.c changes.

In summary, here's my suggestion for color of the bikeshed.


--
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

On Thu, Nov 19, 2020 at 11:00:40AM +0100, Magnus Hagander wrote:
> I'm thinking the code might get a lot cleaner if we just make a single
> set of ifdefs, even if that means repeating the function header. In
> theory we could put them in different *.c files as well, but that
> seems overkill given how tiny they are.

If you reorganize the code this way, I think that make coverage
(genhtml mainly) would complain because the same function is defined
multiple times.  I have fallen in this trap recently, with 2771fcee.
--
Michael

On Thu, Nov 19, 2020 at 12:04 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Nov 19, 2020 at 11:00:40AM +0100, Magnus Hagander wrote:
> > I'm thinking the code might get a lot cleaner if we just make a single
> > set of ifdefs, even if that means repeating the function header. In
> > theory we could put them in different *.c files as well, but that
> > seems overkill given how tiny they are.
>
> If you reorganize the code this way, I think that make coverage
> (genhtml mainly) would complain because the same function is defined
> multiple times.  I have fallen in this trap recently, with 2771fcee.

Ugh, that's pretty terrible.

I guess the only way around that is then to split it up into separate
files. And while I think this way makes the code a lot easier to read,
and thereby safer, I'm not sure it's worth quite *that*.

Or do you know of some other way to get around that?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

On Thu, Nov 19, 2020 at 09:49:05PM +0100, Magnus Hagander wrote:
> Ugh, that's pretty terrible.

I have spent some time testing this part this morning, and I can see
that genhtml does not complain with your patch.  It looks like in the
case of 2771fce the tool got confused because the same function was
getting compiled twice for the backend and the frontend, but here you
only get one code path compiled depending on the option used.

+#else /* not OPENSSL or WIN32 */
I think you mean USE_OPENSSL or just OpenSSL here, but not "OPENSSL".

pg_strong_random.c needs a pgindent run, there are two inconsistent
diffs.  Looks fine except for those nits.
--
Michael

> On 20 Nov 2020, at 03:31, Michael Paquier <michael@paquier.xyz> wrote

> pg_strong_random.c needs a pgindent run, there are two inconsistent
> diffs.  Looks fine except for those nits.

Agreed, this is the least complicated (and most readable) we can make this
file, especially if we add more providers.  +1.

cheers ./daniel

On Fri, Nov 20, 2020 at 3:31 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Nov 19, 2020 at 09:49:05PM +0100, Magnus Hagander wrote:
> > Ugh, that's pretty terrible.
>
> I have spent some time testing this part this morning, and I can see
> that genhtml does not complain with your patch.  It looks like in the
> case of 2771fce the tool got confused because the same function was
> getting compiled twice for the backend and the frontend, but here you
> only get one code path compiled depending on the option used.
>
> +#else /* not OPENSSL or WIN32 */
> I think you mean USE_OPENSSL or just OpenSSL here, but not "OPENSSL".

Yeah. Well, I either meant "OpenSSL or Win32" or "USE_OPENSSL or
WIN32", and ended up with some incorrect mix :) Thanks, fixed.


> pg_strong_random.c needs a pgindent run, there are two inconsistent
> diffs.  Looks fine except for those nits.

I saw only one after this, but maybe I ended up auto-fixing it whenI
changed that define.

That said, pgindent now run, and patch pushed.

Thanks!


--
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/