Re: SCRAM with channel binding downgrade attack

On Fri, May 18, 2018 at 10:46:46AM +0900, Michael Paquier wrote:
> From a security point of view, 1) is important for libpq, but I am not
> much enthusiast about 2) as a whole.  The backend has proper support for
> channel binding, hence other drivers speaking the protocol could have
> their own restriction mechanisms.

So, I have been playing with libpq to address point 1), and added a new
connection parameter called channel_binding_mode which can be set to
'prefer', which is what libpq uses now, and 'require'.  The patch has
two important parts:
1) If a server does not support channel binding, still it is sending
back a SCRAM authentication, but the client still wants to enforce the
use of channel binding, then libpq reacts as follows:
$ psql -d "dbname=foo channel_binding_mode=require"
psql: channel binding required for SASL authentication but no valid
mechanism could be selected
This requires pg_SASL_init() to be patched after the SASL mechanism has
been selected.  That error can be triggered with a v10 server with whom
a SCRAM authentication is done, as well as with a v11 server where SSL
is not used.  Some people may use sslmode=prefer in combination to
channel_binding_mode=require, in which case an error should be raised if
the SSL connection cannot be achieved first.  That addresses a bit of
the craziness of sslmode=prefer...
2) If client wants to use channel binding, but the server is trying to
enforce another protocol than SCRAM, like MD5, trust, gssapi or such,
then the following error happens:
$ psql -d "dbname=foo channel_binding_mode=require"
psql: channel binding required for authentication but no valid protocol are used
In this case, it seems to me that the best bet is to patch
pg_fe_sendauth() and filter by message types.

In the attached, I have added the parameter and some documentation.  I
have not added tests, but some things could be tested in the SSL suite:
- Check for incorrect values in the parameter.
- Test connection without SCRAM with "require"
- Test connection without SSL but SCRAM with "require"
I have not put much thoughts into the documentation, but the patch is
rather simple so hopefully that helps in making progress in the
discussion.
--
Michael