Re: SCRAM with channel binding downgrade attack

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:

>          /*
>           * 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")

> +    /*
> +     * 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".

> +  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"

> +    /*
> +     * 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
channelbinding 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?

> +    /*
> +     * 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."

> +  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". 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".

- Heikki