Re: BUG #17760: SCRAM authentication fails with "modern" (rsassaPss signature) server certificate - Mailing list pgsql-bugs

On Wed, Feb 08, 2023 at 06:11:24PM +0200, Heikki Linnakangas wrote:
> On 05/02/2023 03:28, Heikki Linnakangas wrote:
>> There are a few things we change in PostgreSQL for this:

Nice digging in finding the set of openssl commands that would be
needed to produce certs able to trigger the error.

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

If the server cannot do it, not offering -PLUS makes sense and sending
the mechanisms available back to the client is the first step of the
SASL exchange as far as I recall.

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

But the client has the choice to decide if it wants to use channel
binding, does it?  In this case, it would send the non-PLUS mechanism
followed by 'n' as gs2-cbind-flag, no?  If the client requires channel
binding with channel_binding=require and the server does not support
it, the client should fail if the server has a certificate that does
not support 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.

Perhaps that's something worth reporting to upstream?  Not having the
interface to be able to find out the hash algo to use for a rsapss
certificate sounds like an issue on its own.  Or it's just that rsapss
has nothing of the kind?

be_tls_get_certificate_hash() as presented just returns the hash
calculated in the TLS init phase, cached in TopMemoryContext.  So you
are doing so because you need to know at a stage earlier than the SASL
exchange if channel binding would be a possibility or not.  I am
wondering how this would complicate a potential implementation of
tls-exporter, as deciding if -PLUS should be sent depends on more
parameters.  Anyway, this part is fine here.

+ * or TLS v1.3 to be secure.  In the future, we should implement the
+ * tls-exporter channel binding type as defined in RFC9266.
One thing here is the complexity that we get in terms of channel
binding negotiation, and what to do with endpoint once tls-exporter is
around.  See recent patch https://commitfest.postgresql.org/40/3850/
and its associated thread.

+   /*
+    * Pre-calculate the hash of the certificate, for use in channel binding.
+    */
+   new_cert = SSL_CTX_get0_certificate(context);
The minimal version supported on HEAD is OpenSSL 1.0.1, so this will
not compile as this routine exists only since 1.0.2.  I am afraid that
this is going to require an extra configure flag.  Using a wrapper
that englobes calculate_certificate_hash() would do the work, as
well.

+       oldcxt = MemoryContextSwitchTo(TopMemoryContext);
+       new_hash = calculate_certificate_hash(new_cert, &new_hash_len, isServerStart);
+       MemoryContextSwitchTo(oldcxt);
Shouldn't this bit in be_tls_init() have a "goto error" if the result
of calculate_certificate_hash() is NULL in some cases?  On server
startup, that would be a FATAL, but not on reloads where we would have
a LOG.  There are indeed two code paths where you still want to
proceed with the next TLS init steps, so this had better return a
boolean to let its caller know if an error should be triggered or not?

+           ereport(LOG,
+                   (errmsg("channel binding is disabled; server certificate does not unambiguously specify a signature
algorithm")));
[...]
+       ereport(LOG,
+               (errmsg("channel binding is disabled; server certificate has unknown signature algorithm")));
No FATAL if isServerStart, because the certificate in use does not
provide a hash algorithm that can be used..  I think that we'd better
document that, with a warning for example.  On the client-side, it is
fortunate that we have channel_binding in libpq so as one would know
when the server does not support it, still it worries me that if
things are not set up correctly, then instances could finish by being
unprotected without knowing about it, especially since the default is
channel_binding=prefer.

It seems so wrong to me that we would just silently disable this
feature because RSA-PSS does not give you an algo type to do the
computation work.  I'll try to look at bit at the OpenSSL code base
and see if we could not extract the algo information in this case.
Unfortunately, RFC 5929 is very clear:
"if the certificate's signatureAlgorithm uses no hash functions or
uses multiple hash functions, then this channel binding type's
channel bindings are undefined at this time (updates to is channel
binding type may occur to address this issue if it ever arises)."

I understand from this sentence that if a certificate has no hash
functions, then there's nothing you can do about it.

So as much as I'd like to be aggressive and potentially enforce the
use of SHA256 to compute the certificate hash, what you are doing is
RFC-compliant.  I am wondering whether it would make sense to force a
failure with a GUC if the server cannot compute the hash of the
certificate at startup, though, rather than hope that a user knows
about that via a client forcing channel_binding=require at connection
time.
--
Michael