Re: SCRAM with channel binding downgrade attack

On 05/06/18 09:41, Michael Paquier wrote:
> 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:
>>> + 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.

Ok. Perhaps add a comment pointing out that as the code stands,
get_auth_request_str() is never called with AUTH_REQ_OK. So that if
someone starts calling it with that, maybe they'll know to revisit this.

> + <varlistentry>
> + <term><literal>prefer</literal> (default)</term>
> + <listitem>
> + <para>
> + Use channel binding if available. If the cluster does not
> + support it, then it is not used. This is the default.
> + </para>
> + </listitem>
> + </varlistentry>

I'd suggest s/cluster/server/. Here, and elsewhere in the docs changes.

There are some funny behaviors with different compinations of
"scram_channel_binding_mode=require" and different sslmode settings. For
example, with sslmode=disable, the client will attempt to connect, but
it's guaranteed to fail at authentication, because channel binding is
only possible with SSL. Perhaps it should throw an error without even
attempting to connect? Or at least, the "server does not support channel
binding" is misleading, as it was the client that insisted on an
impossible combination; the server might well support channel binding,
if SSL was used.

And with sslmode=allow, if the server allows a non-SSL connection, then
the client won't use SSL, and will fail, as with sslmode=disable. But if
the server requires SSL, then it will work. Perhaps sslmode=allow should
be "promoted" to "sslmode=require", if
scram_channel_binding_mode=require? Or don't let the user select a silly
combination like that at all, and throw an error.

If scram_channel_binding_mode=require, but the server doesn't support
SSL at all, you get:

psql: server does not support channel binding, and
scram_channel_binding_mode=require was used

Perhaps it would be more clear to say "server does not support SSL" or
something like that. I could imagine an admin spending a long time
looking for an "enable channel binding" option in server settings, not
realizing that it's actually "ssl=off" that's the problem.

As the patch stands, if the server is configured for "trust"
authentication, and the client uses
"scram_channel_binding_mode=require", you get this error:

psql: channel binding required for authentication but SASL exchange is
not finished

"SASL exchange is not finished" is quite technical. Can we have
something like "... but server was configured for \"trust\" authentication"?

So, in general, would be good to go through the different combinations
of scram_channel_binding_mode, sslmode, server configured for SSL or
not, server configured for different authentication methods, one more
time. To make sure the error message in each case makes sense, and
points to what the admin needs to change to make it work.

- Heikki