Re: SCRAM with channel binding downgrade attack

Michael Paquier <michael@paquier.xyz>

From: Michael Paquier <michael@paquier.xyz>

To: Postgres hackers <pgsql-hackers@postgresql.org>

Cc: Heikki Linnakangas <hlinnaka@iki.fi>, Robert Haas <robertmhaas@gmail.com>, Magnus Hagander <magnus@hagander.net>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Stephen Frost <sfrost@snowman.net>, Bruce Momjian <bruce@momjian.us>

Date: 2018-05-28T01:23:28Z

Lists: pgsql-hackers

Attachments

0001-Rework-scram_channel_binding-to-protect-from-downgra.patch (text/x-diff) patch 0001

On Sat, May 26, 2018 at 11:42:38PM +0900, Michael Paquier wrote:
> On Sat, May 26, 2018 at 09:08:50AM -0400, Bruce Momjian wrote:
>> On Sat, May 26, 2018 at 08:32:20AM +0900, Michael Paquier wrote:
>>>
>>> OK, I can live with that as well.  So we'll go in the direction of two
>>> parameters then:
>>> - scram_channel_binding, which can use "prefer" (default), "require" or
>>> "disable".
>>> - scram_channel_binding_name, developer option to choose the type of
>>> channel binding, with "tls-unique" (default) and "tls-server-end-point".
>>> We could also remove the prefix "scram_".  Ideas of names are welcome.
>> 
>> scram_channel_binding_method?
> 
> Or scram_channel_binding_type.  The first sentence of RFC 5929 uses this
> term.

I just went with scram_channel_binding_mode (require, disable and
prefer) and scram_channel_binding_type as parameter names, in the shape
of the attached patch.

>> Do we really know someone is going to want to actually specify the
>> channel binding type?  If it is only testing, maybe we don't need to
>> document this parameter.
> 
> Keeping everything documented is useful as well for new developers, as
> they need to guess less from the code.  So I would prefer seeing both
> connection parameters documented, and mentioning directly in the docs if
> a parameter is for developers or not.

So done this way.  Feel free to pick me up at PGcon this week if you
wish to discuss this issue.  Docs, tests and a commit message are
added.
--
Michael

Commits

doc: update PG 11 release notes
- 6eb612fea9d0 12.0 cited
Fix misspelled pg_trgm contrib name in PostgreSQL 11 release notes
- 3abc5a67edfd 12.0 landed
Doc: clarify release note text about v11's new window function features.
- 510421c45fb4 11.0 landed
- 11a3aeeb5e17 12.0 landed
Improve wording of release notes item
- bee6a683a5c3 11.0 landed
Fix typos in release notes
- fb6accd27b99 11.0 landed
Doc: preliminary list of PG11 major features.
- 4aad161c9a6d 11.0 landed
Make numeric power() handle NaNs according to the modern POSIX spec.
- d1fc750b5199 11.0 landed
Various improvements of skipping index scan during vacuum technics
- 8e12f4a250d2 11.0 cited
Revert back-branch changes in power()'s behavior for NaN inputs.
- f74d83b3034f 10.4 cited
Avoid wrong results for power() with NaN input on more platforms.
- 6bdf1303b34b 11.0 cited
Avoid wrong results for power() with NaN input on some platforms.
- 61b200e2f582 11.0 cited
Skip full index scan during cleanup of B-tree indexes when possible
- 857f9c36cda5 11.0 cited
Rewrite the code that applies scan/join targets to paths.
- 11cf92f6e2e1 11.0 cited
Postpone generate_gather_paths for topmost scan/join rel.
- 3f90ec8597c3 11.0 cited
Add casts from jsonb
- c0cbe00fee6d 11.0 cited
Make plpgsql use its DTYPE_REC code paths for composite-type variables.
- 4b93f57999a2 11.0 cited
Don't allow VACUUM VERBOSE ANALYZE VERBOSE.
- 921059bd66c7 11.0 cited
Pass InitPlan values to workers via Gather (Merge).
- e89a71fb449a 11.0 cited
Account for the effect of lossy pages when costing bitmap scans.
- 5edc63bda68a 11.0 cited
Allow no-op GiST support functions to be omitted.
- d3a4f89d8a3e 11.0 cited
Rearm statement_timeout after each executed query.
- f8e5f156b30e 11.0 cited
Push limit through subqueries to underlying sort, where possible.
- 1f6d515a67ec 11.0 cited