Re: Password identifiers, protocol aging and SCRAM protocol

On Tue, Sep 27, 2016 at 9:01 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> * Added error-handling for OOM and other errors in liybpq
> * In libpq, added check that the server sent back the same client-nonce
> * Turned ERRORs into COMMERRORs and removed DEBUG4 lines (they could reveal
> useful information to an attacker)
> * Improved comments

Thanks!

> * A source of random values. This currently uses PostmasterRandom()
> similarly to how the MD5 salt is generated, in the server, but plain old
> random() in the client. If built with OpenSSL, we should probably use
> RAND_bytes(). But what if the client is built without OpenSSL? I believe the
> protocol doesn't require cryptographically strong randomness for the nonces,
> i.e. it's OK if they're predictable, but they should be different for each
> session.

And what if we just replace PostmasterRandom()? pgcrypto is a useful
source of inspiration here. If the server is built with OpenSSL we use
RAND_bytes all the time. If not, let's use /dev/urandom. If urandom is
not there, we fallback to /dev/random. For WIN32, there is
CryptGenRandom(). This could just be done as an independent patch with
a routine in src/common/ for example to allow both frontend and
backend to use it. Do you think that this is a requirement for this
patch? I think not really for the first shot.

> * Nonce and salt lengths. The patch currently uses 10 bytes for both, but I
> think I just pulled number that out of thin air. The spec doesn't say
> anything about nonce and salt lengths AFAICS. What do other implementations
> use? Is 10 bytes enough?

Good question, but that seems rather short to me now that you mention
it. Mongo has implemented already SCRAM-SHA-1 and they are using 3
uint64 so that's 24 bytes (sasl_scramsha1_client_conversation.cpp for
example). For the salt I am seeing a reference to a string "salt"
only, which is too short.

> * The spec defines a final "server-error" message that the server sends on
> authentication failure, or e.g. if a required extension is not supported.
> The patch just uses FATAL for those. Should we try to send a server-error
> message instead, or before, the elog(FATAL) ?

It seems to me that sending back the error while the context is still
alive, aka before the FATAL would be the way to go. That could be
nicely done with an error callback while the exchange is happening. I
missed that while going through the spec.
--
Michael