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