On 23/04/2024 20:02, Jacob Champion wrote:
On Fri, Apr 19, 2024 at 2:43 PM Heikki Linnakangas <hlinn...@iki.fi> wrote:
On 19/04/2024 19:48, Jacob Champion wrote:
On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas <hlinn...@iki.fi> wrote:
With direct SSL negotiation, we always require ALPN.
(As an aside: I haven't gotten to test the version of the patch that
made it into 17 yet, but from a quick glance it looks like we're not
rejecting mismatched ALPN during the handshake as noted in [1].)
Ah, good catch, that fell through the cracks. Agreed, the client should
reject a direct SSL connection if the server didn't send ALPN. I'll add
that to the Open Items so we don't forget again.
Yes, the client should also reject, but that's not what I'm referring
to above. The server needs to fail the TLS handshake itself with the
proper error code (I think it's `no_application_protocol`?); otherwise
a client implementing a different protocol could consume the
application-level bytes coming back from the server and act on them.
That's the protocol confusion attack from ALPACA we're trying to
avoid.
I finally understood what you mean. So if the client supports ALPN, but
the list of protocols that it provides does not include 'postgresql',
the server should reject the connection with 'no_applicaton_protocol'
alert. Makes sense. I thought OpenSSL would do that with the alpn
callback we have, but it does not.
The attached patch makes that change. I used the alpn_cb() function in
openssl's own s_server program as example for that.
Unfortunately the error message you got in the client with that was
horrible (I modified the server to not accept the 'postgresql' protocol):
psql "dbname=postgres sslmode=require host=localhost"
psql: error: connection to server at "localhost" (::1), port 5432
failed: SSL error: SSL error code 167773280
This is similar to the case with system errors discussed at
https://postgr.es/m/b6fb018b-f05c-4afd-abd3-318c649fa...@highgo.ca, but
this one is equally bad on OpenSSL 1.1.1 and 3.3.0. It seems like an
OpenSSL bug to me, because there is an error string "no application
protocol" in the OpenSSL sources (ssl/ssl_err.c):
{ERR_PACK(ERR_LIB_SSL, 0, SSL_R_NO_APPLICATION_PROTOCOL),
"no application protocol"},
and in the server log, you get that message. But the error code seen in
the client is different. There are also messages for other alerts, for
example:
{ERR_PACK(ERR_LIB_SSL, 0, SSL_R_TLSV13_ALERT_MISSING_EXTENSION),
"tlsv13 alert missing extension"},
The bottom line is that that seems like a bug of omission to me in
OpenSSL, but I wouldn't hold my breath waiting for it to be fixed. We
can easily check for that error code and print the right message
ourselves however, as in the attached patch.
--
Heikki Linnakangas
Neon (https://neon.tech)
From 125e9adda6cdab644b660772e29c713863e93cc2 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Sat, 27 Apr 2024 01:47:55 +0300
Subject: [PATCH 1/1] Reject SSL connection if ALPN is used but there's no
common protocol
If the client supports ALPN but tries to use some other protocol, like
HTTPS, reject the connection in the server. That is surely a confusion
of some sort like trying to PostgreSQL server with a
browser. Furthermore, the ALPN RFC 7301 says:
> In the event that the server supports no protocols that the client
> advertises, then the server SHALL respond with a fatal
> "no_application_protocol" alert.
This commit makes the server follow that advice.
In the client, specifically check for the OpenSSL error code for the
"no_application_protocol" alert. Otherwise you got a cryptic "SSL
error: SSL error code 167773280" error if you tried to connect to a
non-PostgreSQL server that rejects the connection with
"no_application_protocol". ERR_reason_error_string() returns NULL for
that code, which frankly seems like an OpenSSL bug to me, but we can
easily print a better message ourselves.
---
src/backend/libpq/be-secure-openssl.c | 10 +++++++---
src/interfaces/libpq/fe-secure-openssl.c | 12 ++++++++++++
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index fc46a33539..60cf68aac4 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -1336,10 +1336,14 @@ alpn_cb(SSL *ssl,
if (retval == OPENSSL_NPN_NEGOTIATED)
return SSL_TLSEXT_ERR_OK;
- else if (retval == OPENSSL_NPN_NO_OVERLAP)
- return SSL_TLSEXT_ERR_NOACK;
else
- return SSL_TLSEXT_ERR_NOACK;
+ {
+ /*
+ * The client doesn't support our protocol. Reject the connection
+ * with TLS "no_application_protocol" alert, per RFC 7301.
+ */
+ return SSL_TLSEXT_ERR_ALERT_FATAL;
+ }
}
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 0de21dc7e4..ee1a47f2b1 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1741,6 +1741,18 @@ SSLerrmessage(unsigned long ecode)
return errbuf;
}
+ if (ERR_GET_LIB(ecode) == ERR_LIB_SSL &&
+ ERR_GET_REASON(ecode) == SSL_AD_REASON_OFFSET + SSL_AD_NO_APPLICATION_PROTOCOL)
+ {
+ /*
+ * Server aborted the connection with TLS "no_application_protocol"
+ * alert. The ERR_reason_error_string() function doesn't give any
+ * error string for that for some reason, so do it ourselves.
+ */
+ snprintf(errbuf, SSL_ERR_LEN, libpq_gettext("no application protocol"));
+ return errbuf;
+ }
+
/*
* In OpenSSL 3.0.0 and later, ERR_reason_error_string randomly refuses to
* map system errno values. We can cover that shortcoming with this bit
--
2.39.2
