Re: Random number generation, take two

On 11/30/2016 09:01 AM, Michael Paquier wrote:
> On Tue, Nov 29, 2016 at 10:02 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>> 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?

I was thinking that with --disable-strong-random, we'd use plain
random() in libpq as well. I believe SCRAM is analogous to the MD5 salt
generation in the backend, in its requirement for randomness. The SCRAM
spec (RFC5802) says:

> It is important that this value [nonce] be different for each
> authentication (see [RFC4086] for more details on how to achieve
> this)

So the nonces need to be different for each session, to avoid replay
attacks. But they don't necessarily need to be unpredictable, they are
transmitted in plaintext during the authentication, anyway. If an
attacker can calculate them in advance, it only buys him more time, but
doesn't give any new information.

If we were 100% confident on that point, we could just always use
current timestamp and a counter for the nonces. But I'm not that
confident, certainly feels better to use a stronger random number when
available. The quote above points at RFC4086, which actually talks about
cryptographically strong random numbers, rather than just generating a
unique nonce. So I'm not sure if the authors of SCRAM considered that
point in any detail, seems like they just assumed that you might as well
use a strong random source because everyone's got one.

>> 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

Ok, I bit the bullet and modified those pgcrypto functions that truly
need cryptographically strong random numbers to throw an error with
--disable-strong-random. Notably, the pgp encrypt functions mostly fail now.

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

Ah, I didn't remember we have a section in the user manual for these. Added.

> + int32 MyCancelKey;
> Those would be better as unsigned?

Perhaps, but it's historically been signed, I'm afraid of changing it..

> +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.

No, it needs to be exclusive. Each pg_jrand48() call updates the state,
aka seed.

> 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.

Great, thanks! I wonder if this is overly complicated, though. For
comparison, we haven't bothered to expose --disable-spinlocks in
config_default.pl either. Perhaps we should just always use the Windows
native function on MSVC, whether or not configured with OpenSSL, and
just put USE_WIN32_RANDOM in pg_config.h.win32? See 2nd attached patch
(untested).

- Heikki