Re: Move OpenSSL random under USE_OPENSSL_RANDOM

On Mon, Nov 16, 2020 at 10:19 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:

> > On 16 Nov 2020, at 01:20, Michael Paquier <michael(at)paquier(dot)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.
>

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.

Keeping two defines and an extra configure check when they mean the same
thing seems like the worst combination of the two.

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