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-02T17:08:56Z
Lists: pgsql-hackers 
On 28/05/18 15:08, Michael Paquier wrote:
> On Mon, May 28, 2018 at 12:26:37PM +0300, Heikki Linnakangas wrote:
>> Sounds good.
> 
> Okay.  Done this way as attached.  If the backend forces anything else
> than SCRAM then the client gets an immediate error if channel binding is
> required, without waiting for the password prompt.

Thanks! The comments and error messages need some copy-editing:

>  		/*
>  		 * Select the mechanism to use.  Pick SCRAM-SHA-256-PLUS over anything
> 		 * else if a channel binding mode is not 'disable'.  Pick SCRAM-SHA-256
> 		 * if nothing else has already been picked.  If we add more mechanisms, a
>  		 * more refined priority mechanism might become necessary.
>  		 */

"else if a channel binding mode is not 'disable'" sounds a bit awkward. 
A double negative. (Also, "a" -> "the")

> +	/*
> +	 * If client is willing to enforce the use the channel binding but
> +	 * it has not been previously selected, because it was either not
> +	 * published by the server or could not be selected, then complain
> +	 * back.  If SSL is not used for this connection, still complain
> +	 * similarly, as the client may want channel binding but forgot
> +	 * to set it up to do so which could be the case with sslmode=prefer
> +	 * for example.  This protects from any kind of downgrade attacks
> +	 * from rogue servers attempting to bypass channel binding.
> +	 */

Instead of "is willing to enforce the use the channel binding", perhaps 
simply "requires channel binding".

> +  printfPQExpBuffer(&conn->errorMessage,
> +    libpq_gettext("channel binding required for SASL authentication but no valid mechanism could be selected\n"));

Channel binding is a property of SCRAM authentication specifically, it's 
not applicable to other SASL mechanisms. So I'd suggest something like:

"server does not support channel binding, and 
channel_binding_mode=require was used"

> +	/*
> +	 * Complain if channel binding mechanism is chosen but no channel
> +	 * binding type is defined.
> +	 */
> +	if (strcmp(selected_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0 &&
> +		conn->scram_channel_binding_type == NULL)
> +	{
> +		printfPQExpBuffer(&conn->errorMessage,
> +						  libpq_gettext("SASL authentication mechanism using channel binding supported but no channel binding type defined\n"));
> +		goto error;
> +	}

It's not immediately clear to me from the error message or the comment, 
when this error is thrown. Can this even happen?

> +	/*
> +	 * Before processing any message, perform security-related sanity
> +	 * checks.  If the client is willing to perform channel binding with
> +	 * SCRAM authentication, only a subset of messages can be used so
> +	 * as there is no transmission of any password data or such.
> +	 */

I'd suggest something like:

"If SCRAM with channel binding is required, refuse all other 
authentication methods. Requiring channel binding implies that the 
client doesn't trust the server, until the SCRAM authentication is 
completed, so it's important that we don't divulge the plaintext 
password, or perform some weaker authentication, even if the server asks 
for it. In all other authentication methods, it's currently assumed that 
the client trusts the server, either implicitly or because the SSL 
certificate was already verified during the SSL handshake."

> +  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". Is it possible to have an error hint here? Perhaps add 
"HINT: change the authentication method to scram-sha-256 in the server's 
pg_hba.conf file".

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