On Mon, Nov 16, 2020 at 10:19 AM Daniel Gustafsson <dan...@yesql.se> wrote:
> > On 16 Nov 2020, at 01:20, Michael Paquier <mich...@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. > 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/>
