Horton

Re: SCRAM with channel binding downgrade attack

Magnus Hagander <magnus@hagander.net>

View thread View on postgr.es
From: Magnus Hagander <magnus@hagander.net>
To: Michael Paquier <michael@paquier.xyz>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>, Heikki Linnakangas <hlinnaka@iki.fi>, Postgres hackers <pgsql-hackers@postgresql.org>, Robert Haas <robertmhaas@gmail.com>, Stephen Frost <sfrost@snowman.net>, Bruce Momjian <bruce@momjian.us>
Date: 2018-06-14T11:43:30Z
Lists: pgsql-hackers 
On Tue, Jun 12, 2018 at 6:49 AM, Michael Paquier <michael@paquier.xyz>
wrote:

> On Mon, Jun 11, 2018 at 04:54:45PM +0200, Magnus Hagander wrote:
> > I'm wondering if that means we should then also not do it specifically
> for
> > scram in this version. Otherwise we're likely to end up with a parameter
> > that only has a "lifetime" of one version, and that seems like a bad
> idea.
> > If nothing else we should clearly think out what the path is to make sure
> > that doesn't happen. (e.g. we don't want a
> > scram_channel_binding_mode=require in this version, if the next one is
> > going to replace it with something like heikkis suggested
> > allowed_authentication_methods=SCRAM-SHA-256-PLUS or whatever we end up
> > coming up with there).
>
> Conceptually, it depends on if we consider SCRAM and
> SCRAM+channel_binding as two separate authentication protocols.  However
> it seems to me that as both are the same thing as they use the same
> protocol so it would be confusing for the user to be able to define both
> SCRAM-SHA-256 and SCRAM-SHA-256-PLUS within the same list, so I would
> tend to think that we should have in this future
> "allowed_authentication_methods" only scram-sha-256 listed as an option,
> which counts for both SCRAM with and without channel binding.
>

One could argue they are equally the same protocol if we have
SCRAM-SHA-512 or whatever in the future. So how would those be handled?


Thinking harder about this thread, it could be as well possible in the
> future that we add support for channel binding for some other SASL
> mechanism, in which case I would tend to rename
> scram_channel_binding_type and scram_channel_binding_mode to simply
> channel_binding_type and channel_binding_mode, without any concepts of
> SCRAM attached to it.  So in short, I'd like to keep both enforcement
> mechanisms as separate concepts.  One future compatibility issue is how
> to deal with for example cases like allowed_authentication_methods="md5"
> and channel_binding_mode=require where an allowed authentication does
> not have channel binding?  And it seems to me that this should result in
> an error.
>

Yeah, not embedding scram in the name seems smarter. But you might still
need to define which one, so channel_binding_mode=require wouldn't be
enough in that case -- you'd need to have
channel_binding_mode=require-scram-sha-256-plus, wouldn't you?

And yes, in your suggested example, it should absolutely fail early, as
there is no way to actually connect with that setting. Arguably we should
also fail on e.g. sslmode=require over a unix socket, though, which we
don't. But that is probably not somethign to copy :)

I still think that the fact that we are still discussing what is basically
the *basic concepts* of how this would be set up after we have released
beta1 is a clear sign that this should not go into 11.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

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.