Re: Password identifiers, protocol aging and SCRAM protocol

On 10/12/2016 11:11 AM, Michael Paquier wrote:
> And so we are back on that, with a new set:

Great! I'm looking at this first one for now:

> - 0001, introducing pg_strong_random() in src/port/ to have the
> backend portion of SCRAM use it instead of random(). This patch is
> from Magnus who has kindly sent is to me, so the authorship goes to
> him. This patch replaces at the same time PostmasterRandom() with it,
> this way once SCRAM gets integrated both the frontend and the backend
> finish using the same facility. I think that's good for consistency.
> Compared to the version Magnus has sent me, I have changed two things:
> -- Reading from /dev/urandom and /dev/random is not influenced by
> EINTR. read() handling is also made better in case of partial reads
> from a given source.
> -- Win32 Crypto routines use MS_DEF_PROV instead of NULL. I think
> that's a better idea to not let the user the choice of the encryption
> source here.

I spent some time whacking that around:

* Renamed the file to src/port/pg_strong_random.c "pgsrandom" makes me
think of srandom(), which this isn't.

* Changed pg_strong_random() to return false on error, and let the
callers handle errors. That's more error-prone than throwing an error in
the function itself, as it's an easy mistake to forget to check for the
return value, but we can't just "exit(1)" if called in the frontend. If
it gets called from libpq during authentication, as it will with SCRAM,
we want to close the connection and report an error, not exit the whole
user application. Likewise, in postmaster, if we fail to generate a
query cancel key when forking a backend, we don't want to FATAL and shut
down the whole postmaster.

* There used to be this:

> /*
> - * Precompute password salt values to use for this connection. It's
> - * slightly annoying to do this long in advance of knowing whether we'll
> - * need 'em or not, but we must do the random() calls before we fork, not
> - * after. Else the postmaster's random sequence won't get advanced, and
> - * all backends would end up using the same salt...
> - */
> - RandomSalt(port->md5Salt, sizeof(port->md5Salt));

But that whole business of advancing postmaster's random sequence is
moot now. So I moved the generation of md5 salt from postmaster to where
MD5 authentication is performed.

* This comment in postmaster.c was wrong:

> @@ -581,7 +571,7 @@ PostmasterMain(int argc, char *argv[])
> * Note: the seed is pretty predictable from externally-visible facts such
> * as postmaster start time, so avoid using random() for security-critical
> * random values during postmaster startup. At the time of first
> - * connection, PostmasterRandom will select a hopefully-more-random seed.
> + * connection, pg_strong_random will select a hopefully-more-random seed.
> */
> srandom((unsigned int) (MyProcPid ^ MyStartTime));

We don't use pg_strong_random() for that, the same PID+timestamp method
is still used as before. Adjusted the comment to reflect reality.

* Added "#include <Wincrypt.h>", for the CryptAcquireContext and
CryptGenRandom functions? It compiled OK without that, so I guess it got
pulled in via some other header file, but seems more clear and
future-proof to #include it directly.

* random comment kibitzing (no pun intended).

This is pretty much ready for commit now, IMO, but please do review one
more time. And I do have some small questions still:

* We now open and close /dev/(u)random on every pg_strong_random() call.
Should we be worried about performance of that?

* Now that we don't call random() in postmaster anymore, is there any
point in calling srandom() there (i.e. where the above incorrect comment
was)? Should we remove it? random() might be used by pre-loaded
extensions, though. (Hopefully not for cryptographic purposes.)

* Should we backport this? Sorry if we discussed that already, but I
don't remember.

- Heikki