Horton

Re: SCRAM with channel binding downgrade attack

Heikki Linnakangas <hlinnaka@iki.fi>

View thread View on postgr.es
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Michael Paquier <michael@paquier.xyz>
Cc: Postgres hackers <pgsql-hackers@postgresql.org>, Robert Haas <robertmhaas@gmail.com>, Magnus Hagander <magnus@hagander.net>, Stephen Frost <sfrost@snowman.net>, Bruce Momjian <bruce@momjian.us>
Date: 2018-06-06T20:46:11Z
Lists: pgsql-hackers 
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

Commits

  1. doc: update PG 11 release notes

  2. Fix misspelled pg_trgm contrib name in PostgreSQL 11 release notes

  3. Doc: clarify release note text about v11's new window function features.

  4. Improve wording of release notes item

  5. Fix typos in release notes

  6. Doc: preliminary list of PG11 major features.

  7. Make numeric power() handle NaNs according to the modern POSIX spec.

  8. Various improvements of skipping index scan during vacuum technics

  9. Revert back-branch changes in power()'s behavior for NaN inputs.

  10. Avoid wrong results for power() with NaN input on more platforms.

  11. Avoid wrong results for power() with NaN input on some platforms.

  12. Skip full index scan during cleanup of B-tree indexes when possible

  13. Rewrite the code that applies scan/join targets to paths.

  14. Postpone generate_gather_paths for topmost scan/join rel.

  15. Add casts from jsonb

  16. Make plpgsql use its DTYPE_REC code paths for composite-type variables.

  17. Don't allow VACUUM VERBOSE ANALYZE VERBOSE.

  18. Pass InitPlan values to workers via Gather (Merge).

  19. Account for the effect of lossy pages when costing bitmap scans.

  20. Allow no-op GiST support functions to be omitted.

  21. Rearm statement_timeout after each executed query.

  22. Push limit through subqueries to underlying sort, where possible.