On 11/30/2016 09:01 AM, Michael Paquier wrote:
On Tue, Nov 29, 2016 at 10:02 PM, Heikki Linnakangas <hlinn...@iki.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
0001-Replace-PostmasterRandom-with-a-stronger-source-seco.patch.gz
Description: application/gzip
0002-Fix-MSVC-build.patch
Description: application/download
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
