Remove support for tls-unique channel binding.

There are some problems with the tls-unique channel binding type. It's not
supported by all SSL libraries, and strictly speaking it's not defined for
TLS 1.3 at all, even though at least in OpenSSL, the functions used for it
still seem to work with TLS 1.3 connections. And since we had no
mechanism to negotiate what channel binding type to use, there would be
awkward interoperability issues if a server only supported some channel
binding types. tls-server-end-point seems feasible to support with any SSL
library, so let's just stick to that.

This removes the scram_channel_binding libpq option altogether, since there
is now only one supported channel binding type.

This also removes all the channel binding tests from the SSL test suite.
They were really just testing the scram_channel_binding option, which
is now gone. Channel binding is used if both client and server support it,
so it is used in the existing tests. It would be good to have some tests
specifically for channel binding, to make sure it really is used, and the
different combinations of a client and a server that support or doesn't
support it. The current set of settings we have make it hard to write such
tests, but I did test those things manually, by disabling
HAVE_BE_TLS_GET_CERTIFICATE_HASH and/or
HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH.

I also removed the SCRAM_CHANNEL_BINDING_TLS_END_POINT constant. This is a
matter of taste, but IMO it's more readable to just use the
"tls-server-end-point" string.

Refactor the checks on whether the SSL library supports the functions
needed for tls-server-end-point channel binding. Now the server won't
advertise, and the client won't choose, the SCRAM-SHA-256-PLUS variant, if
compiled with an OpenSSL version too old to support it.

In the passing, add some sanity checks to check that the chosen SASL
mechanism, SCRAM-SHA-256 or SCRAM-SHA-256-PLUS, matches whether the SCRAM
exchange used channel binding or not. For example, if the client selects
the non-channel-binding variant SCRAM-SHA-256, but in the SCRAM message
uses channel binding anyway. It's harmless from a security point of view,
I believe, and I'm not sure if there are some other conditions that would
cause the connection to fail, but it seems better to be strict about these
things and check explicitly.

Discussion: https://www.postgresql.org/message-id/ec787074-2305-c6f4-86aa-6902f98485a4%40iki.fi

src/interfaces/libpq/fe-auth-scram.c

@@ -352,17 +352,9 @@ build_client_first_message(fe_scram_state *state)
 	if (strcmp(state->sasl_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0)
 	{
 		Assert(conn->ssl_in_use);
-		appendPQExpBuffer(&buf, "p=%s", conn->scram_channel_binding);
-	}
-	else if (conn->scram_channel_binding == NULL ||
-			 strlen(conn->scram_channel_binding) == 0)
-	{
-		/*
-		 * Client has chosen to not show to server that it supports channel
-		 * binding.
-		 */
-		appendPQExpBuffer(&buf, "n");
+		appendPQExpBuffer(&buf, "p=tls-server-end-point");
 	}
+#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
 	else if (conn->ssl_in_use)
 	{
 		/*
@@ -370,6 +362,7 @@ build_client_first_message(fe_scram_state *state)
 		 */
 		appendPQExpBuffer(&buf, "y");
 	}
+#endif
 	else
 	{
 		/*
@@ -432,60 +425,28 @@ build_client_final_message(fe_scram_state *state)
 	 */
 	if (strcmp(state->sasl_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0)
 	{
+#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
 		char	   *cbind_data = NULL;
 		size_t		cbind_data_len = 0;
 		size_t		cbind_header_len;
 		char	   *cbind_input;
 		size_t		cbind_input_len;
 
-		if (strcmp(conn->scram_channel_binding, SCRAM_CHANNEL_BINDING_TLS_UNIQUE) == 0)
+		/* Fetch hash data of server's SSL certificate */
+		cbind_data =
+			pgtls_get_peer_certificate_hash(state->conn,
+											&cbind_data_len);
+		if (cbind_data == NULL)
 		{
-#ifdef USE_SSL
-			cbind_data = pgtls_get_finished(state->conn, &cbind_data_len);
-			if (cbind_data == NULL)
-				goto oom_error;
-#endif
-		}
-		else if (strcmp(conn->scram_channel_binding,
-						SCRAM_CHANNEL_BINDING_TLS_END_POINT) == 0)
-		{
-			/* Fetch hash data of server's SSL certificate */
-#ifdef USE_SSL
-			cbind_data =
-				pgtls_get_peer_certificate_hash(state->conn,
-												&cbind_data_len);
-			if (cbind_data == NULL)
-			{
-				/* error message is already set on error */
-				return NULL;
-			}
-#endif
-		}
-		else
-		{
-			/* should not happen */
+			/* error message is already set on error */
 			termPQExpBuffer(&buf);
-			printfPQExpBuffer(&conn->errorMessage,
-							  libpq_gettext("invalid channel binding type\n"));
-			return NULL;
-		}
-
-		/* should not happen */
-		if (cbind_data == NULL || cbind_data_len == 0)
-		{
-			if (cbind_data != NULL)
-				free(cbind_data);
-			termPQExpBuffer(&buf);
-			printfPQExpBuffer(&conn->errorMessage,
-							  libpq_gettext("empty channel binding data for channel binding type \"%s\"\n"),
-							  conn->scram_channel_binding);
 			return NULL;
 		}
 
 		appendPQExpBuffer(&buf, "c=");
 
 		/* p=type,, */
-		cbind_header_len = 4 + strlen(conn->scram_channel_binding);
+		cbind_header_len = strlen("p=tls-server-end-point,,");
 		cbind_input_len = cbind_header_len + cbind_data_len;
 		cbind_input = malloc(cbind_input_len);
 		if (!cbind_input)
@@ -493,8 +454,7 @@ build_client_final_message(fe_scram_state *state)
 			free(cbind_data);
 			goto oom_error;
 		}
-		snprintf(cbind_input, cbind_input_len, "p=%s,,",
-				 conn->scram_channel_binding);
+		memcpy(cbind_input, "p=tls-server-end-point,,", cbind_header_len);
 		memcpy(cbind_input + cbind_header_len, cbind_data, cbind_data_len);
 
 		if (!enlargePQExpBuffer(&buf, pg_b64_enc_len(cbind_input_len)))
@@ -508,12 +468,21 @@ build_client_final_message(fe_scram_state *state)
 
 		free(cbind_data);
 		free(cbind_input);
+#else
+		/*
+		 * Chose channel binding, but the SSL library doesn't support it.
+		 * Shouldn't happen.
+		 */
+		termPQExpBuffer(&buf);
+		printfPQExpBuffer(&conn->errorMessage,
+						  "channel binding not supported by this build\n");
+		return NULL;
+#endif	/* HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH */
 	}
-	else if (conn->scram_channel_binding == NULL ||
-			 strlen(conn->scram_channel_binding) == 0)
-		appendPQExpBuffer(&buf, "c=biws");	/* base64 of "n,," */
+#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
 	else if (conn->ssl_in_use)
 		appendPQExpBuffer(&buf, "c=eSws");	/* base64 of "y,," */
+#endif
 	else
 		appendPQExpBuffer(&buf, "c=biws");	/* base64 of "n,," */
 

src/interfaces/libpq/fe-auth.c

@@ -530,11 +530,26 @@ pg_SASL_init(PGconn *conn, int payloadlen)
 		 * nothing else has already been picked.  If we add more mechanisms, a
 		 * more refined priority mechanism might become necessary.
 		 */
-		if (conn->ssl_in_use &&
-			conn->scram_channel_binding &&
-			strlen(conn->scram_channel_binding) > 0 &&
-			strcmp(mechanism_buf.data, SCRAM_SHA_256_PLUS_NAME) == 0)
-			selected_mechanism = SCRAM_SHA_256_PLUS_NAME;
+		if (strcmp(mechanism_buf.data, SCRAM_SHA_256_PLUS_NAME) == 0)
+		{
+			if (conn->ssl_in_use)
+				selected_mechanism = SCRAM_SHA_256_PLUS_NAME;
+			else
+			{
+				/*
+				 * The server offered SCRAM-SHA-256-PLUS, but the connection
+				 * is not SSL-encrypted. That's not sane. Perhaps SSL was
+				 * stripped by a proxy? There's no point in continuing,
+				 * because the server will reject the connection anyway if we
+				 * try authenticate without channel binding even though both
+				 * the client and server supported it. The SCRAM exchange
+				 * checks for that, to prevent downgrade attacks.
+				 */
+				printfPQExpBuffer(&conn->errorMessage,
+								  libpq_gettext("server offered SCRAM-SHA-256-PLUS authentication over a non-SSL connection\n"));
+				goto error;
+			}
+		}
 		else if (strcmp(mechanism_buf.data, SCRAM_SHA_256_NAME) == 0 &&
 				 !selected_mechanism)
 			selected_mechanism = SCRAM_SHA_256_NAME;

src/interfaces/libpq/fe-connect.c

@@ -123,7 +123,6 @@ static int ldapServiceLookup(const char *purl, PQconninfoOption *options,
 #define DefaultOption	""
 #define DefaultAuthtype		  ""
 #define DefaultTargetSessionAttrs	"any"
-#define DefaultSCRAMChannelBinding	SCRAM_CHANNEL_BINDING_TLS_UNIQUE
 #ifdef USE_SSL
 #define DefaultSSLMode "prefer"
 #else
@@ -264,11 +263,6 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 		"TCP-Keepalives-Count", "", 10, /* strlen(INT32_MAX) == 10 */
 	offsetof(struct pg_conn, keepalives_count)},
 
-	{"scram_channel_binding", NULL, DefaultSCRAMChannelBinding, NULL,
-		"SCRAM-Channel-Binding", "D",
-		21,						/* sizeof("tls-server-end-point") == 21 */
-	offsetof(struct pg_conn, scram_channel_binding)},
-
 	/*
 	 * ssl options are allowed even without client SSL support because the
 	 * client can still handle SSL modes "disable" and "allow". Other
@@ -3481,8 +3475,6 @@ freePGconn(PGconn *conn)
 		free(conn->keepalives_interval);
 	if (conn->keepalives_count)
 		free(conn->keepalives_count);
-	if (conn->scram_channel_binding)
-		free(conn->scram_channel_binding);
 	if (conn->sslmode)
 		free(conn->sslmode);
 	if (conn->sslcert)

src/interfaces/libpq/fe-secure-openssl.c

@@ -369,30 +369,10 @@ pgtls_write(PGconn *conn, const void *ptr, size_t len)
 	return n;
 }
 
-char *
-pgtls_get_finished(PGconn *conn, size_t *len)
-{
-	char		dummy[1];
-	char	   *result;
-
-	/*
-	 * OpenSSL does not offer an API to get directly the length of the TLS
-	 * Finished message sent, so first do a dummy call to grab this
-	 * information and then do an allocation with the correct size.
-	 */
-	*len = SSL_get_finished(conn->ssl, dummy, sizeof(dummy));
-	result = malloc(*len);
-	if (result == NULL)
-		return NULL;
-	(void) SSL_get_finished(conn->ssl, result, *len);
-
-	return result;
-}
-
+#ifdef HAVE_X509_GET_SIGNATURE_NID
 char *
 pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
 {
-#ifdef HAVE_X509_GET_SIGNATURE_NID
 	X509	   *peer_cert;
 	const EVP_MD *algo_type;
 	unsigned char hash[EVP_MAX_MD_SIZE];	/* size for SHA-512 */
@@ -462,12 +442,8 @@ pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
 	*len = hash_size;
 
 	return cert_hash;
-#else
-	printfPQExpBuffer(&conn->errorMessage,
-					  libpq_gettext("channel binding type \"tls-server-end-point\" is not supported by this build\n"));
-	return NULL;
-#endif
 }
+#endif /* HAVE_X509_GET_SIGNATURE_NID */
 
 /* ------------------------------------------------------------ */
 /*						OpenSSL specific code					*/

src/interfaces/libpq/libpq-int.h

@@ -349,7 +349,6 @@ struct pg_conn
 										 * retransmits */
 	char	   *keepalives_count;	/* maximum number of TCP keepalive
 									 * retransmits */
-	char	   *scram_channel_binding;	/* SCRAM channel binding type */
 	char	   *sslmode;		/* SSL mode (require,prefer,allow,disable) */
 	char	   *sslcompression; /* SSL compression (0 or 1) */
 	char	   *sslkey;			/* client key filename */
@@ -715,22 +714,20 @@ extern bool pgtls_read_pending(PGconn *conn);
  */
 extern ssize_t pgtls_write(PGconn *conn, const void *ptr, size_t len);
 
-/*
- * Get the TLS finish message sent during last handshake.
- *
- * This information is useful for callers doing channel binding during
- * authentication.
- */
-extern char *pgtls_get_finished(PGconn *conn, size_t *len);
-
 /*
  * Get the hash of the server certificate, for SCRAM channel binding type
  * tls-server-end-point.
  *
  * NULL is sent back to the caller in the event of an error, with an
  * error message for the caller to consume.
+ *
+ * This is not supported with old versions of OpenSSL that don't have
+ * the X509_get_signature_nid() function.
  */
+#if defined(USE_OPENSSL) && defined(HAVE_X509_GET_SIGNATURE_NID)
+#define HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
 extern char *pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len);
+#endif
 
 /*
  * Verify that the server certificate matches the host name we connected to.