Re: SCRAM with channel binding downgrade attack

On Sat, Jun 02, 2018 at 01:08:56PM -0400, Heikki Linnakangas wrote:
> On 28/05/18 15:08, Michael Paquier wrote:
>> On Mon, May 28, 2018 at 12:26:37PM +0300, Heikki Linnakangas wrote:
>> > Sounds good.
>> 
>> Okay.  Done this way as attached.  If the backend forces anything else
>> than SCRAM then the client gets an immediate error if channel binding is
>> required, without waiting for the password prompt.
> 
> Thanks! The comments and error messages need some copy-editing:

Thanks Heikki for the input.  I have reworked all the points you raised
in the attached.

>>  		/*
>>  		 * Select the mechanism to use.  Pick SCRAM-SHA-256-PLUS over anything
>> 		 * else if a channel binding mode is not 'disable'.  Pick SCRAM-SHA-256
>> 		 * if nothing else has already been picked.  If we add more mechanisms, a
>>  		 * more refined priority mechanism might become necessary.
>>  		 */
> 
> "else if a channel binding mode is not 'disable'" sounds a bit awkward. A
> double negative. (Also, "a" -> "the")

Okay.  I have completely rewritten this block.  Hopefully that's clearer.

>> +	/*
>> +	 * If client is willing to enforce the use the channel binding but
>> +	 * it has not been previously selected, because it was either not
>> +	 * published by the server or could not be selected, then complain
>> +	 * back.  If SSL is not used for this connection, still complain
>> +	 * similarly, as the client may want channel binding but forgot
>> +	 * to set it up to do so which could be the case with sslmode=prefer
>> +	 * for example.  This protects from any kind of downgrade attacks
>> +	 * from rogue servers attempting to bypass channel binding.
>> +	 */
> 
> Instead of "is willing to enforce the use the channel binding", perhaps
> simply "requires channel binding".

Check.

>> +  printfPQExpBuffer(&conn->errorMessage,
>> +    libpq_gettext("channel binding required for SASL authentication but no valid mechanism could be selected\n"));
> 
> Channel binding is a property of SCRAM authentication specifically, it's not
> applicable to other SASL mechanisms. So I'd suggest something like:
> 
> "server does not support channel binding, and channel_binding_mode=require
> was used"

Changed as such, except that this is using scram_channel_binding_mode in
the error message.

>> +	/*
>> +	 * Complain if channel binding mechanism is chosen but no channel
>> +	 * binding type is defined.
>> +	 */
>> +	if (strcmp(selected_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0 &&
>> +		conn->scram_channel_binding_type == NULL)
>> +	{
>> +		printfPQExpBuffer(&conn->errorMessage,
>> +		 libpq_gettext("SASL authentication mechanism using channel binding supported but no channel binding type defined\n"));
>> +		goto error;
>> +	}
> 
> It's not immediately clear to me from the error message or the comment, when
> this error is thrown. Can this even happen?

No, let's not make this happen then.  I have added NULL handling in
connectOptions2 related to the initialization of the options so both
scram_channel_binding_mode and scram_channel_binding_type will never be
NULL like sslmode.

>> +	/*
>> +	 * Before processing any message, perform security-related sanity
>> +	 * checks.  If the client is willing to perform channel binding with
>> +	 * SCRAM authentication, only a subset of messages can be used so
>> +	 * as there is no transmission of any password data or such.
>> +	 */
> 
> I'd suggest something like:
> 
> "If SCRAM with channel binding is required, refuse all other authentication
> methods. Requiring channel binding implies that the client doesn't trust the
> server, until the SCRAM authentication is completed, so it's important that
> we don't divulge the plaintext password, or perform some weaker
> authentication, even if the server asks for it. In all other authentication
> methods, it's currently assumed that the client trusts the server, either
> implicitly or because the SSL certificate was already verified during the
> SSL handshake."

Check.  Thanks for the suggestion.

>> +  printfPQExpBuffer(&conn->errorMessage,
>> +    libpq_gettext("channel binding required for authentication but invalid protocol used\n"));
> 
> If I understand correctly, you get this error, e.g. if you have "password"
> or "sspi" in pg_hba.conf, and the client uses
> "channel_binding_mode=require". "Invalid protocol" doesn't sound right for
> that. Perhaps "channel binding required, but the server requested %s
> authentication".

Right, even "md5" enters in this category if the user has a MD5
verifier, but not if it has a SCRAM verifier.  I have done that with 
get_auth_request_str(), which maps authentication requests to the
probable hba entry.  It feels like cheating to map "trust" to
AUTH_REQ_OK as all methods use it as final message though, even if it is  
not triggered by this patch.  So I have used no mapping name for it.

> Is it possible to have an error hint here? Perhaps add
> "HINT: change the authentication method to scram-sha-256 in the server's
> pg_hba.conf file".

Hm.  A HINT would require something similar to PQresultErrorField for
error hints, which is a fight I not much willing to do just for this
patch.  Spawning a new error message with a newline would also be
considered as a new error message.  So I discard this one.

I have also added a test with a "password" server which stresses this
code path actually, and I just indented the patch.  What do you think?
--
Michael