Re: SCRAM with channel binding downgrade attack

On 22/05/18 11:22, Michael Paquier wrote:
> On Sat, May 19, 2018 at 09:35:57PM +0900, Michael Paquier wrote:
>> The previous patch has actually problems with servers using "trust",
>> "password" and any protocol which can send directly AUTH_REQ_OK as those
>> could still enforce a downgrade to not use channel binding, so we
>> actually need to make sure that AUTH_REQ_SASL_FIN has been received when
>> channel binding is required when looking at a AUTH_REQ_OK message.
> 
> Okay, so I have digested the previous comments with the attached.
> scram_channel_binding is modified as follows (from commit message):
> - "prefer", is the default and behaves so as channel binding is used if
> available.  If the cluster does not support it then it is not used. This
> does not protect from downgrade attacks.
> - "disable", which is the equivalent of the empty value previously,
> disables channel binding.
> - "tls-unique" and "tls-server-end-point" make channel binding a
> requirement and use the channel binding of the same name for
> connection.  Note that in this case, if the connection is attempted
> without SSL or if server does not support channel binding then libpq
> refuses the connection as well.

"tls-unique" and "tls-server-end-point" are overly technical to users. 
They don't care which one is used, there's no difference in security. 
Furthermore, if we add another channel binding type in the future, 
perhaps because someone finds a vulnerability in those types and a new 
one is added to address it, users would surely like to be automatically 
switched over to the new binding type. If they have "tls-unique" 
hardcoded in connection strings, that won't happen.

So I think the options should be "prefer", "disable", and "require". 
That's also symmetrical with sslmode, which is nice.

We could provide "tls-unique" and "tls-server-end-point" in addition to 
those, but I'd consider those to be developer only settings, useful only 
for testing the protocol.

It would be nice to have a libpq setting like 
"authenticate_server=require", which would mean "I want 
man-in-the-middle protection". With that, a connection would be allowed, 
if either the server's SSL certificate is verified as with 
"sslmode=verify-full", *or* SCRAM authentication with channel binding 
was used. Or perhaps cram it into sslmode, 
"sslmode=verify-full-or-scram-channel-binding", just with a nicer name. 
(We can do that after v11 though, I think.)

> In order to make sure that a client is not tricked by a "trust"
> connection downgrade which sends back directly AUTH_REQ_OK, one way I
> have thought about is to check if the client has achieved with a server
> a full SASL exchange when receiving this message type, which is
> something that we know about as the exchange state is saved in
> PGconn->sasl_state.

It'd be nice if the client could tell the server that it requires 
authentication, so that the server would go through the SCRAM 
authentication even with "trust". With channel binding, SCRAM not only 
authenticates the client to the server, but also the server to the 
client. But that's not urgent, I think we can live without it for v11.

- Heikki