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

1 files changed, 151 insertions, 72 deletions

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 48eb531d0f0..8fbad53fa82 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -17,6 +17,19 @@
  *	 by the SASLprep profile, we skip the SASLprep pre-processing and use
  *	 the raw bytes in calculating the hash.
  *
+ * - If channel binding is used, the channel binding type is always
+ *	 "tls-server-end-point".  The spec says the default is "tls-unique"
+ *	 (RFC 5802, section 6.1. Default Channel Binding), but there are some
+ *	 problems with that.  Firstly, not all SSL libraries provide an API to
+ *	 get the TLS Finished message, required to use "tls-unique".  Secondly,
+ *	 "tls-unique" is not specified for TLS v1.3, and as of this writing,
+ *	 it's not clear if there will be a replacement.  We could support both
+ *	 "tls-server-end-point" and "tls-unique", but for our use case,
+ *	 "tls-unique" doesn't really have any advantages.  The main advantage
+ *	 of "tls-unique" would be that it works even if the server doesn't
+ *	 have a certificate, but PostgreSQL requires a server certificate
+ *	 whenever SSL is used, anyway.
+ *
  *
  * The password stored in pg_authid consists of the iteration count, salt,
  * StoredKey and ServerKey.
@@ -111,8 +124,7 @@ typedef struct
 	const char *username;		/* username from startup packet */
 
 	Port	   *port;
-	char		cbind_flag;
-	char	   *channel_binding_type;
+	bool		channel_binding_in_use;
 
 	int			iterations;
 	char	   *salt;			/* base64-encoded */
@@ -120,6 +132,7 @@ typedef struct
 	uint8		ServerKey[SCRAM_KEY_LEN];
 
 	/* Fields of the first message from client */
+	char		cbind_flag;
 	char	   *client_first_message_bare;
 	char	   *client_username;
 	char	   *client_nonce;
@@ -155,22 +168,58 @@ static void mock_scram_verifier(const char *username, int *iterations,
 					char **salt, uint8 *stored_key, uint8 *server_key);
 static bool is_scram_printable(char *p);
 static char *sanitize_char(char c);
+static char *sanitize_str(const char *s);
 static char *scram_mock_salt(const char *username);
 
 /*
+ * pg_be_scram_get_mechanisms
+ *
+ * Get a list of SASL mechanisms that this module supports.
+ *
+ * For the convenience of building the FE/BE packet that lists the
+ * mechanisms, the names are appended to the given StringInfo buffer,
+ * separated by '\0' bytes.
+ */
+void
+pg_be_scram_get_mechanisms(Port *port, StringInfo buf)
+{
+	/*
+	 * Advertise the mechanisms in decreasing order of importance.  So the
+	 * channel-binding variants go first, if they are supported.  Channel
+	 * binding is only supported with SSL, and only if the SSL implementation
+	 * has a function to get the certificate's hash.
+	 */
+#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH
+	if (port->ssl_in_use)
+	{
+		appendStringInfoString(buf, SCRAM_SHA_256_PLUS_NAME);
+		appendStringInfoChar(buf, '\0');
+	}
+#endif
+	appendStringInfoString(buf, SCRAM_SHA_256_NAME);
+	appendStringInfoChar(buf, '\0');
+}
+
+/*
  * pg_be_scram_init
  *
  * Initialize a new SCRAM authentication exchange status tracker.  This
  * needs to be called before doing any exchange.  It will be filled later
  * after the beginning of the exchange with verifier data.
  *
- * 'username' is the username provided by the client in the startup message.
+ * 'selected_mech' identifies the SASL mechanism that the client selected.
+ * It should be one of the mechanisms that we support, as returned by
+ * pg_be_scram_get_mechanisms().
+ *
  * 'shadow_pass' is the role's password verifier, from pg_authid.rolpassword.
- * If 'shadow_pass' is NULL, we still perform an authentication exchange, but
- * it will fail, as if an incorrect password was given.
+ * The username was provided by the client in the startup message, and is
+ * available in port->user_name.  If 'shadow_pass' is NULL, we still perform
+ * an authentication exchange, but it will fail, as if an incorrect password
+ * was given.
  */
 void *
 pg_be_scram_init(Port *port,
+				 const char *selected_mech,
 				 const char *shadow_pass)
 {
 	scram_state *state;
@@ -179,7 +228,27 @@ pg_be_scram_init(Port *port,
 	state = (scram_state *) palloc0(sizeof(scram_state));
 	state->port = port;
 	state->state = SCRAM_AUTH_INIT;
-	state->channel_binding_type = NULL;
+
+	/*
+	 * Parse the selected mechanism.
+	 *
+	 * Note that if we don't support channel binding, either because the SSL
+	 * implementation doesn't support it or we're not using SSL at all, we
+	 * would not have advertised the PLUS variant in the first place.  If the
+	 * client nevertheless tries to select it, it's a protocol violation like
+	 * selecting any other SASL mechanism we don't support.
+	 */
+#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH
+	if (strcmp(selected_mech, SCRAM_SHA_256_PLUS_NAME) == 0 && port->ssl_in_use)
+		state->channel_binding_in_use = true;
+	else
+#endif
+	if (strcmp(selected_mech, SCRAM_SHA_256_NAME) == 0)
+		state->channel_binding_in_use = false;
+	else
+		ereport(ERROR,
+				(errcode(ERRCODE_PROTOCOL_VIOLATION),
+				 errmsg("client selected an invalid SASL authentication mechanism")));
 
 	/*
 	 * Parse the stored password verifier.
@@ -656,6 +725,36 @@ sanitize_char(char c)
 }
 
 /*
+ * Convert an arbitrary string to printable form, for error messages.
+ *
+ * Anything that's not a printable ASCII character is replaced with
+ * '?', and the string is truncated at 30 characters.
+ *
+ * The returned pointer points to a static buffer.
+ */
+static char *
+sanitize_str(const char *s)
+{
+	static char buf[30 + 1];
+	int			i;
+
+	for (i = 0; i < sizeof(buf) - 1; i++)
+	{
+		char		c = s[i];
+
+		if (c == '\0')
+			break;
+
+		if (c >= 0x21 && c <= 0x7E)
+			buf[i] = c;
+		else
+			buf[i] = '?';
+	}
+	buf[i] = '\0';
+	return buf;
+}
+
+/*
  * Read the next attribute and value in a SCRAM exchange message.
  *
  * Returns NULL if there is attribute.
@@ -715,6 +814,8 @@ read_any_attr(char **input, char *attr_p)
 static void
 read_client_first_message(scram_state *state, char *input)
 {
+	char	   *channel_binding_type;
+
 	input = pstrdup(input);
 
 	/*------
@@ -790,6 +891,12 @@ read_client_first_message(scram_state *state, char *input)
 			 * The client does not support channel binding or has simply
 			 * decided to not use it.  In that case just let it go.
 			 */
+			if (state->channel_binding_in_use)
+				ereport(ERROR,
+						(errcode(ERRCODE_PROTOCOL_VIOLATION),
+						 errmsg("malformed SCRAM message"),
+						 errdetail("The client selected SCRAM-SHA-256-PLUS, but the SCRAM message does not include channel binding data.")));
+
 			input++;
 			if (*input != ',')
 				ereport(ERROR,
@@ -804,15 +911,22 @@ read_client_first_message(scram_state *state, char *input)
 			/*
 			 * The client supports channel binding and thinks that the server
 			 * does not.  In this case, the server must fail authentication if
-			 * it supports channel binding, which in this implementation is
-			 * the case if a connection is using SSL.
+			 * it supports channel binding.
 			 */
+			if (state->channel_binding_in_use)
+				ereport(ERROR,
+						(errcode(ERRCODE_PROTOCOL_VIOLATION),
+						 errmsg("malformed SCRAM message"),
+						 errdetail("The client selected SCRAM-SHA-256-PLUS, but the SCRAM message does not include channel binding data.")));
+
+#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH
 			if (state->port->ssl_in_use)
 				ereport(ERROR,
 						(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
 						 errmsg("SCRAM channel binding negotiation error"),
 						 errdetail("The client supports SCRAM channel binding but thinks the server does not.  "
 								   "However, this server does support channel binding.")));
+#endif
 			input++;
 			if (*input != ',')
 				ereport(ERROR,
@@ -826,44 +940,25 @@ read_client_first_message(scram_state *state, char *input)
 
 			/*
 			 * The client requires channel binding.  Channel binding type
-			 * follows, e.g., "p=tls-unique".
+			 * follows, e.g., "p=tls-server-end-point".
 			 */
-			{
-				char	   *channel_binding_type;
-
-				if (!state->port->ssl_in_use)
-				{
-					/*
-					 * Without SSL, we don't support channel binding.
-					 *
-					 * RFC 5802 specifies a particular error code,
-					 * e=server-does-support-channel-binding, for this.  But
-					 * it can only be sent in the server-final message, and we
-					 * don't want to go through the motions of the
-					 * authentication, knowing it will fail, just to send that
-					 * error message.
-					 */
-					ereport(ERROR,
-							(errcode(ERRCODE_PROTOCOL_VIOLATION),
-							 errmsg("client requires SCRAM channel binding, but it is not supported")));
-				}
+			if (!state->channel_binding_in_use)
+				ereport(ERROR,
+						(errcode(ERRCODE_PROTOCOL_VIOLATION),
+						 errmsg("malformed SCRAM message"),
+						 errdetail("The client selected SCRAM-SHA-256 without channel binding, but the SCRAM message includes channel binding data.")));
 
-				/*
-				 * Read value provided by client.  (It is not safe to print
-				 * the name of an unsupported binding type in the error
-				 * message.  Pranksters could print arbitrary strings into the
-				 * log that way.)
-				 */
-				channel_binding_type = read_attr_value(&input, 'p');
-				if (strcmp(channel_binding_type, SCRAM_CHANNEL_BINDING_TLS_UNIQUE) != 0 &&
-					strcmp(channel_binding_type, SCRAM_CHANNEL_BINDING_TLS_END_POINT) != 0)
-					ereport(ERROR,
-							(errcode(ERRCODE_PROTOCOL_VIOLATION),
-							 (errmsg("unsupported SCRAM channel-binding type"))));
-
-				/* Save the name for handling of subsequent messages */
-				state->channel_binding_type = pstrdup(channel_binding_type);
-			}
+			channel_binding_type = read_attr_value(&input, 'p');
+
+			/*
+			 * The only channel binding type we support is
+			 * tls-server-end-point.
+			 */
+			if (strcmp(channel_binding_type, "tls-server-end-point") != 0)
+				ereport(ERROR,
+						(errcode(ERRCODE_PROTOCOL_VIOLATION),
+						 (errmsg("unsupported SCRAM channel-binding type \"%s\"",
+								 sanitize_str(channel_binding_type)))));
 			break;
 		default:
 			ereport(ERROR,
@@ -1096,8 +1191,9 @@ read_client_final_message(scram_state *state, char *input)
 	 * then followed by the actual binding data depending on the type.
 	 */
 	channel_binding = read_attr_value(&p, 'c');
-	if (state->channel_binding_type)
+	if (state->channel_binding_in_use)
 	{
+#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH
 		const char *cbind_data = NULL;
 		size_t		cbind_data_len = 0;
 		size_t		cbind_header_len;
@@ -1108,39 +1204,18 @@ read_client_final_message(scram_state *state, char *input)
 
 		Assert(state->cbind_flag == 'p');
 
-		/*
-		 * Fetch data appropriate for channel binding type
-		 */
-		if (strcmp(state->channel_binding_type, SCRAM_CHANNEL_BINDING_TLS_UNIQUE) == 0)
-		{
-#ifdef USE_SSL
-			cbind_data = be_tls_get_peer_finished(state->port, &cbind_data_len);
-#endif
-		}
-		else if (strcmp(state->channel_binding_type,
-						SCRAM_CHANNEL_BINDING_TLS_END_POINT) == 0)
-		{
-			/* Fetch hash data of server's SSL certificate */
-#ifdef USE_SSL
-			cbind_data = be_tls_get_certificate_hash(state->port,
-													 &cbind_data_len);
-#endif
-		}
-		else
-		{
-			/* should not happen */
-			elog(ERROR, "invalid channel binding type");
-		}
+		/* Fetch hash data of server's SSL certificate */
+		cbind_data = be_tls_get_certificate_hash(state->port,
+												 &cbind_data_len);
 
 		/* should not happen */
 		if (cbind_data == NULL || cbind_data_len == 0)
-			elog(ERROR, "empty channel binding data for channel binding type \"%s\"",
-				 state->channel_binding_type);
+			elog(ERROR, "could not get server certificate hash");
 
-		cbind_header_len = 4 + strlen(state->channel_binding_type); /* p=type,, */
+		cbind_header_len = strlen("p=tls-server-end-point,,");	/* p=type,, */
 		cbind_input_len = cbind_header_len + cbind_data_len;
 		cbind_input = palloc(cbind_input_len);
-		snprintf(cbind_input, cbind_input_len, "p=%s,,", state->channel_binding_type);
+		snprintf(cbind_input, cbind_input_len, "p=tls-server-end-point,,");
 		memcpy(cbind_input + cbind_header_len, cbind_data, cbind_data_len);
 
 		b64_message = palloc(pg_b64_enc_len(cbind_input_len) + 1);
@@ -1156,6 +1231,10 @@ read_client_final_message(scram_state *state, char *input)
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
 					 (errmsg("SCRAM channel binding check failed"))));
+#else
+		/* shouldn't happen, because we checked this earlier already */
+		elog(ERROR, "channel binding not supported by this build");
+#endif
 	}
 	else
 	{