Re: SCRAM with channel binding downgrade attack

On Wed, Jun 06, 2018 at 11:46:11PM +0300, Heikki Linnakangas wrote:
> 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.

That makes sense. Done.

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

Done.

> There are some funny behaviors with different combinations 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.

I was not really planning to make the code more complicated with
cross-option checks (that's complicated enough), but isn't what you are
looking for here to make sslmode=require a mandatory thing if
channel_binding_mode=require is set? It is possible to add checks like
that in connectOptions2 for example after setting all the values.
Promoting automatically sslmode="allow" to "require" if
scram_channel_binding_mode=require feels like a trap for users as well.

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

It really depends on the connection context as a v10 server allows SSL
without channel binding, so just saying that server does not support SSL
is not correct either. It seems to me that if you want an operator to
take correct actions then we want to see if the backend connected to is
a v10 server or something newer, or actually uses SSL in the connection
context. If that's a v10 or older, then libpq should still complain
about channel binding not supported. If it is a v11 or newer, then
libpq should complain about SSL not being supported. By adding the
cross-option check in connectOptions2, the error message about SSL not
being supported is moot if requiring sslmode=require for
channel_binding_mode=require. I have added a more complicated logic in
pg_SASL_init() for that (grep for "server does not support channel
binding") as it looks safer to me to have that for future sanity checks,
so please let me know your thoughts. And it could be perfectly possible
that an attacking server tries to make the client think that it is a v11
server but tries to downgrade to SCRAM without channel binding.

Still, the tests reach their limit here, as you would normally bump into
HBA entries which don't match normally, still it is easy to add a test
which checks that sslmode!=require fails with scram_channel_binding=require.

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

OK, here is a suggestion then as in the attached:
"channel binding required, but the server requested authentication \"trust\""

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

In the updated patch attached, you get the following failures with all
those configurations (making sslmode=require a requirement with
channel_binding_mode=require reduces the set of combinations a lot):
1) Server supporting SSL.
1-1) v10 server with SCRAM, failure:
$ psql "sslmode=require scram_channel_binding_mode=require host=localhost"
psql: server does not support channel binding, and scram_channel_binding_mode=require was used
1-2) v10 server with md5, password or trust, failures:
$ psql "sslmode=require scram_channel_binding_mode=require host=localhost"
psql: SSL enabled and channel binding required, but the server requested authentication "trust
$ psql "sslmode=require scram_channel_binding_mode=require host=localhost"
psql: SSL enabled and channel binding required, but the server requested
authentication "md5"
1-3) v11 server with SCRAM, works up to password processing... Of course.
1-4) v11 server with md5, password or trust:
$ psql "sslmode=require scram_channel_binding_mode=require host=localhost"
psql: SSL enabled and channel binding required, but the server requested
authentication "trust"
$ psql "sslmode=require scram_channel_binding_mode=require host=localhost"
psql: SSL enabled and channel binding required, but the server requested
authentication "md5"

So I have added an extra hint to tell that SSL is enabled and added an
assertion as well as this is enforced by sslmode=require.

2) Server not supporting SSL, with failures for all cases as sslmode
overrides any settings of scram_channel_binding_mode=require
2-1) v10 server with SCRAM.
2-2) v10 server with md5, password or trust, failures.
2-3) v11 server with SCRAM: failure.
2-4) v11 server with md5, password or trust: failures.
$ psql "sslmode=require scram_channel_binding_mode=require host=localhost"
psql: server does not support SSL, but SSL was required

Thanks,
--
Michael