Re: Random number generation, take two

On Tue, Nov 29, 2016 at 10:02 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Ok, here's my second attempt at refactoring random number generation.
> Previous attempt crashed and burned, see
> https://www.postgresql.org/message-id/E1bw3g3-0003st-6M@gemulon.postgresql.org.
> This addresses the issues pointed out in that thread.

Yeah. (Nit: I want this badly)

> Autoconf
> --------
>
> * Configure chooses the implementation to use: OpenSSL, Windows native, or
> /dev/urandom, in that order. You can also force it, by specifying
> USE_OPENSSL_RANDOM=1, USE_WIN32_RANDOM=1 or USE_DEV_URANDOM=1 on the
> configure command line. That's useful for testing.

It's that or a configure option able to take an optional argument but
that conflicts with --with-openssl so your way looks better.

> * Remove the support for /dev/random (only support /dev/urandom). I don't
> think we have any platforms where /dev/urandom is not present, but
> /dev/random is. If we do, the buildfarm will tell us, but until then let's
> keep it simple.

Yes, I don't recall of a modern platform without urandom if random is
there, recalling when researching on the matter a couple of weeks back
or even now. Even newest HP/UX boxes have it!

> Fallback implementation (with --disable-strong-random)
> -----------
>
> In postmaster, the algorithm is similar to the existing PostmasterRandom()
> function. It's based on the built-in PRNG, seeded by postmaster and first
> backend start time. It's been rewritten to fit the rest of the code better,
> however.
>
> In backends, there is a new function, pg_backend_random(). It also uses the
> built-in PRNG, but the seed is shared between all backends, in shared
> memory. (Otherwise all backends would inherit the same seed from
> postmaster.)

Sounds fine for me.

> Phew, this has been way more complicated than it seemed at first. Thoughts?

One of the goals of this patch is to be able to have a strong random
function as well for the frontend, which is fine. But any build where
--disable-strong-random is used is not going to have a random function
to rely on. Disabling SCRAM for such builds is a possibility, because
we assume that any build using --disable-strong-random is aware of
security risks, still that's not really appealing in terms of
portability. Another possibility would be to have an extra routine
like pg_frontend_random(), wrapping pg_strong_backend() and using a
combination of getpid + gettimeofday to generate a seed with just a
random() call? That's what we were fighting against previously, so my
mind is telling me that just returning an error when the code paths of
the SCRAM client code is used when built with --disable-strong-random
is the way to go, because we want SCRAM to be fundamentally safe to
use. What do you think?

> pgcrypto
> --------
>
> pgcrypto doesn't have the same requirements for "strongness" as cancel keys
> and MD5 salts have. pgcrypto uses random numbers for generating salts, too,
> which I think has similar requirements. But it also uses random numbers for
> generating encryption keys, which I believe ought to be harder to predict.
> If you compile with --disable-strong-random, do we want the encryption key
> generation routines to fail, or to return known-weak keys?
>
> This patch removes the Fortuna algorithm, that was used to generate fairly
> strong random numbers, if OpenSSL was not present. One option would be to
> keep that code as a fallback. I wanted to get rid of that, since it's only
> used on a few old platforms, but OTOH it's been around for a long time with
> little issues.
>
> As this patch stands, it removes Fortuna, and returns known-weak keys, but
> there's a good argument to be made for throwing an error instead.

IMO, leading to an error would make the users aware of them playing
with the fire... Now pademelon's owner may likely have a different
opinion on the matter :p

This patch needs to initialize the variable "res" in
pad_eme_pkcs1_v15(), creating compilation warnings.

Documentation for --disable-strong-random needs to be added.

Cancel keys are not broken.

+   int32       MyCancelKey;
Those would be better as unsigned?

+bool
+pg_backend_random(char *dst, int len)
+{
+   int         i;
+   char       *end = dst + len;
+
+   /* should not be called in postmaster */
+   Assert (IsUnderPostmaster || !IsPostmasterEnvironment);
+
+   LWLockAcquire(BackendRandomLock, LW_EXCLUSIVE);
Shouldn't an exclusive lock be taken only when the initialization
phase is called? When reading the value a shared lock would be fine.

Attached is a patch for MSVC to apply on top of yours to enable the
build for strong and weak random functions. Feel free to hack it as
needs be, this base implementation works for the current
implementation.
--
Michael