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