Re: BUG #17760: SCRAM authentication fails with "modern" (rsassaPss signature) server certificate

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: gunnar(dot)bluth(at)pro-open(dot)de, pgsql-bugs(at)lists(dot)postgresql(dot)org
Cc: Jacob Champion <jchampion(at)timescale(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>
Subject: Re: BUG #17760: SCRAM authentication fails with "modern" (rsassaPss signature) server certificate
Date: 2023-02-08 16:11:24
Message-ID: 584842ed-e476-6bac-bb36-2aec204e9eef@iki.fi
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs

cc'ing Jacob and Michael who have poked around channel binding before.
If you have a chance to review this patch, I would appreciate that.

On 05/02/2023 03:28, Heikki Linnakangas wrote:
> On 25/01/2023 13:32, PG Bug reporting form wrote:
>> I do think however that this is an oversight on our side and has to be
>> addressed. If not in code, the docs should point out that certain server
>> certificate types (PSS) may not work with SCRAM auth (or libpq needs to be
>> compiled against a minimum version of OpenSSL, if that's the root cause).
>
> There are a few things we change in PostgreSQL for this:
>
> 1. If the server cannot compute a hash of the certificate, because it
> cannot unambiguously determine the hash algorithm to use, it should not
> offer the SCRAM-SHA-256-PLUS authentication method to the client. In
> other words, if the server cannot do channel binding with it's
> certificate, it should say so, instead of erroring out later.

Here's a patch for that.

> 2. Similarly in the client: if libpq cannot determinate the hash
> algorithm to use with the server's certificate, it should not attempt
> channel binding.
>
> I believe this is OK from a security point of view. If the server is
> using a certificate that cannot be used with channel binding, and the
> client doesn't require channel binding, it's OK to not do it. A man in
> the middle can present a certificate to the client that cannot be used
> with channel binding, but if they can do that they could also just not
> offer the SCRAM-SHA-256-PLUS option to the client. So I don't see risk
> of downgrade attacks here.

On second thoughts, the above is wrong. We cannot safely downgrade to
not using channel binding just because we don't support the
certificate's signature algorithm. Otherwise a MITM could indeed do what
I said above and send a certificate that uses an unsupported signature
algorithm, to force downgrade. It cannot just modify the SASL mechanism
negotiation by leaving out the SCRAM-SHA-256-PLUS, because the SCRAM
handshake catches that case. The client sends a different gs2-cbind-flag
when it chooses to not do channel binding even though the server
supports it, and when it doesn't use channel binding because the server
didn't support it.

I improved the error message in the client, but I'm afraid that's the
best we can do in the client side. Fortunately, it is enough to upgrade
the server to include this fix, to allow clients to connect. (Albeit
without channel binding).

> 3. Add support for channel binding with RSA-PSS. The problem is that
> be_tls_get_certificate_hash() doesn't know which digest algorithm to
> use. As you noted, OBJ_find_sigid_algs() returns "undef" (NID_undef) for
> rsassaPss certificates. I did some googling: when certificate uses
> RSASSA-PSS as the signature algorithm, there is a separate field,
> RSASSA-PSS-params that specifies the hash algorithm
> (https://www.rfc-editor.org/rfc/rfc4055#section-3.1). If you ask me,
> OBJ_find_sigid_algs() should look into RSASSA-PSS-params and figure it
> out, but I'm no expert in the OpenSSL codebase so maybe that would be
> the wrong place to do it.

I didn't dare to make any such changes for now. Maybe we could do that
in 'master', but I would be wary of backpatching.

- Heikki

Attachment Content-Type Size
0001-Don-t-advertise-channel-binding-to-client-when-we-ca.patch text/x-patch 24.4 KB

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message Tom Lane 2023-02-08 17:11:52 Re: BUG #17781: Assert in setrefs.c
Previous Message Tom Lane 2023-02-08 16:05:16 Re: BUG #17774: Assert triggered on brin_minmax_multi.c