Channel Binding patch posted in postgresql ml

0001-Rework-scram_channel_binding-to-protect-from-downgra.patch

From 5bf51e7bdcfaf2d6e8af5132bb7884bc307f440b Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Tue, 22 May 2018 17:03:48 +0900
Subject: [PATCH] Rework scram_channel_binding to protect from downgrade
 attacks

When a client attempts to connect to a PostgreSQL cluster, it may be
possible that it requested channel binding with SCRAM authentication,
but that the server tricks the clister and forcibly downgrades the
authentication request.  For example, a v10 cluster supports SCRAM but
not channel binding so authentication could be achieved without channel
binding used.

In order to protect from such attacks, rework libpq handling of the
connection parameter scram_channel_binding as follows:
- "prefer", is the default and behaves so as channel binding is used if
available.  If the cluster does not support it then it is not used.
This does not protect from downgrade attacks.
- "disable", which is the equivalent of the empty value previously,
disables channel binding.
- "tls-unique" and "tls-server-end-point" make channel binding a
requirement and use the channel binding of the same name for
connection.

For both "tls-unique" and "tsl-server-end-point, if the cluster does not
support channel binding with SCRAM (v10 server) or if SSL is not used,
then the connection is refused by libpq, which is something that can
happen if SSL is not used (prevention also for users forgetting to use
sslmode=require at least in connection strings).  This also allows users
to enforce the use of SCRAM with channel binding even if the targetted
cluster downgrades to "trust" or similar.

In order to achieve that, when receiving AUTH_REQ_OK from the server, we
check if a SASL exchange has happened and has finished, where the client
makes sure that the server knows the connection proof.  If the cluster
does not publish the -PLUS mechanism, then connection is also refused.

Discussion: https://postgr.es/m/[email protected]
---
 doc/src/sgml/libpq.sgml              | 34 +++++++++++-----
 src/interfaces/libpq/fe-auth-scram.c | 59 ++++++++++++++++++++++------
 src/interfaces/libpq/fe-auth.c       | 57 +++++++++++++++++++++++++--
 src/interfaces/libpq/fe-auth.h       |  4 +-
 src/interfaces/libpq/fe-connect.c    | 20 +++++++++-
 src/test/ssl/ServerSetup.pm          | 22 +++++++----
 src/test/ssl/t/002_scram.pl          | 38 ++++++++++++++++--
 7 files changed, 198 insertions(+), 36 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 498b8df988..f1fa744a8b 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1250,18 +1250,34 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
 
        <para>
         The list of channel binding types supported by the server are
-        listed in <xref linkend="sasl-authentication"/>.  An empty value
-        specifies that the client will not use channel binding.  If this
-        parameter is not specified, <literal>tls-unique</literal> is used,
-        if supported by both server and client.
-        Channel binding is only supported on SSL connections.  If the
-        connection is not using SSL, then this setting is ignored.
+        listed in <xref linkend="sasl-authentication"/>.  A value of
+        <literal>disable</literal> disables channel binding completely,
+        hence the client decides to not use it even if the server supports
+        it.  A value of <literal>prefer</literal>, the default, means that
+        channel binding will be used if the server supports it, and not
+        used if there is no server-side support.  This is useful when
+        connecting to a <productname>PostgreSQL</productname> cluster
+        of version 10 which has support for <acronym>SCRAM</acronym> but
+        no channel binding support, however this does not protect from
+        protocol downgrade attacks.  In this case
+        <literal>tls-unique</literal> is used as channel binding type.
        </para>
 
        <para>
-        This parameter is mainly intended for protocol testing.  In normal
-        use, there should not be a need to choose a channel binding type other
-        than the default one.
+        Specifying directly the channel binding name as value
+        (<literal>tls-unique</literal> or
+        <literal>tls-server-end-point</literal>) makes channel binding
+        a requirement and <application>libpq</application> will refuse
+        the connection to any cluster not supporting it, protecting from
+        protocol downgrade attacks.
+       </para>
+
+       <para>
+        Channel binding is only supported on SSL connections.  However, if
+        the connection is not using SSL, then this setting is ignored except
+        if the channel binding name is explicitely defined as value, in which
+        case connection to the cluster is refused if SSL is not enabled or
+        if the cluster does not support channel binding.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index 8415bbb5c6..85d3795189 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -45,6 +45,7 @@ typedef struct
 	PGconn	   *conn;
 	char	   *password;
 	char	   *sasl_mechanism;
+	char	   *channel_binding_type;
 
 	/* We construct these */
 	uint8		SaltedPassword[SCRAM_KEY_LEN];
@@ -80,7 +81,8 @@ static bool pg_frontend_random(char *dst, int len);
 void *
 pg_fe_scram_init(PGconn *conn,
 				 const char *password,
-				 const char *sasl_mechanism)
+				 const char *sasl_mechanism,
+				 const char *channel_binding_type)
 {
 	fe_scram_state *state;
 	char	   *prep_password;
@@ -102,10 +104,19 @@ pg_fe_scram_init(PGconn *conn,
 		return NULL;
 	}
 
+	state->channel_binding_type = strdup(channel_binding_type);
+	if (!state->channel_binding_type)
+	{
+		free(state->sasl_mechanism);
+		free(state);
+		return NULL;
+	}
+
 	/* Normalize the password with SASLprep, if possible */
 	rc = pg_saslprep(password, &prep_password);
 	if (rc == SASLPREP_OOM)
 	{
+		free(state->channel_binding_type);
 		free(state->sasl_mechanism);
 		free(state);
 		return NULL;
@@ -115,6 +126,7 @@ pg_fe_scram_init(PGconn *conn,
 		prep_password = strdup(password);
 		if (!prep_password)
 		{
+			free(state->channel_binding_type);
 			free(state->sasl_mechanism);
 			free(state);
 			return NULL;
@@ -137,6 +149,8 @@ pg_fe_scram_free(void *opaq)
 		free(state->password);
 	if (state->sasl_mechanism)
 		free(state->sasl_mechanism);
+	if (state->channel_binding_type)
+		free(state->channel_binding_type);
 
 	/* client messages */
 	if (state->client_nonce)
@@ -352,10 +366,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);
+		appendPQExpBuffer(&buf, "p=%s", state->channel_binding_type);
 	}
-	else if (conn->scram_channel_binding == NULL ||
-			 strlen(conn->scram_channel_binding) == 0)
+	else if (strcmp(conn->scram_channel_binding, "disable") == 0)
 	{
 		/*
 		 * Client has chosen to not show to server that it supports channel
@@ -438,7 +451,8 @@ build_client_final_message(fe_scram_state *state)
 		char	   *cbind_input;
 		size_t		cbind_input_len;
 
-		if (strcmp(conn->scram_channel_binding, SCRAM_CHANNEL_BINDING_TLS_UNIQUE) == 0)
+		if (strcmp(state->channel_binding_type,
+				   SCRAM_CHANNEL_BINDING_TLS_UNIQUE) == 0)
 		{
 #ifdef USE_SSL
 			cbind_data = pgtls_get_finished(state->conn, &cbind_data_len);
@@ -446,7 +460,7 @@ build_client_final_message(fe_scram_state *state)
 				goto oom_error;
 #endif
 		}
-		else if (strcmp(conn->scram_channel_binding,
+		else if (strcmp(state->channel_binding_type,
 						SCRAM_CHANNEL_BINDING_TLS_END_POINT) == 0)
 		{
 			/* Fetch hash data of server's SSL certificate */
@@ -478,14 +492,14 @@ build_client_final_message(fe_scram_state *state)
 			termPQExpBuffer(&buf);
 			printfPQExpBuffer(&conn->errorMessage,
 							  libpq_gettext("empty channel binding data for channel binding type \"%s\"\n"),
-							  conn->scram_channel_binding);
+							  state->channel_binding_type);
 			return NULL;
 		}
 
 		appendPQExpBuffer(&buf, "c=");
 
 		/* p=type,, */
-		cbind_header_len = 4 + strlen(conn->scram_channel_binding);
+		cbind_header_len = 4 + strlen(state->channel_binding_type);
 		cbind_input_len = cbind_header_len + cbind_data_len;
 		cbind_input = malloc(cbind_input_len);
 		if (!cbind_input)
@@ -494,7 +508,7 @@ build_client_final_message(fe_scram_state *state)
 			goto oom_error;
 		}
 		snprintf(cbind_input, cbind_input_len, "p=%s,,",
-				 conn->scram_channel_binding);
+				 state->channel_binding_type);
 		memcpy(cbind_input + cbind_header_len, cbind_data, cbind_data_len);
 
 		if (!enlargePQExpBuffer(&buf, pg_b64_enc_len(cbind_input_len)))
@@ -509,8 +523,7 @@ build_client_final_message(fe_scram_state *state)
 		free(cbind_data);
 		free(cbind_input);
 	}
-	else if (conn->scram_channel_binding == NULL ||
-			 strlen(conn->scram_channel_binding) == 0)
+	else if (strcmp(conn->scram_channel_binding, "disable") == 0)
 		appendPQExpBuffer(&buf, "c=biws");	/* base64 of "n,," */
 	else if (conn->ssl_in_use)
 		appendPQExpBuffer(&buf, "c=eSws");	/* base64 of "y,," */
@@ -811,6 +824,30 @@ pg_fe_scram_build_verifier(const char *password)
 	return result;
 }
 
+/*
+ * Check if a SCRAM exchange has been finished or not.  This can
+ * be used for security-related checks.
+ */
+bool
+pg_fe_scram_exchange_finished(PGconn *conn)
+{
+	fe_scram_state *state;
+
+	if (conn == NULL ||
+		conn->sasl_state == NULL)
+		return false;
+
+	state = (fe_scram_state *) conn->sasl_state;
+
+	/*
+	 * A exchange is considered as complete only once the server
+	 * signature has been checked.
+	 */
+	if (state->state != FE_SCRAM_FINISHED)
+		return false;
+	return true;
+}
+
 /*
  * Random number generator.
  */
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 3b2073a47f..d6e6e06237 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -490,6 +490,7 @@ pg_SASL_init(PGconn *conn, int payloadlen)
 	bool		done;
 	bool		success;
 	const char *selected_mechanism;
+	const char *selected_channel_binding;
 	PQExpBufferData mechanism_buf;
 	char	   *password;
 
@@ -548,8 +549,42 @@ pg_SASL_init(PGconn *conn, int payloadlen)
 	}
 
 	/*
-	 * Now that the SASL mechanism has been chosen for the exchange,
-	 * initialize its state information.
+	 * If client is willing to enforce the use the channel binding but
+	 * it has not been previously selected, because it was either not
+	 * published by the server or could not be selected, then complain
+	 * back.  If SSL is not used for this connection, still complain
+	 * similarly, as the client may want channel binding but forgot
+	 * to set it up to do so which could be the case with sslmode=prefer
+	 * for example.  This protects from any kind of downgrade attacks
+	 * from rogue servers attempting to bypass channel binding.  Channel
+	 * binding is required if the client is explicitely defining a channel
+	 * binding name as input parameter.
+	 */
+	if (conn->scram_channel_binding &&
+		(strcmp(conn->scram_channel_binding, SCRAM_CHANNEL_BINDING_TLS_UNIQUE) == 0 ||
+		 strcmp(conn->scram_channel_binding, SCRAM_CHANNEL_BINDING_TLS_END_POINT) == 0
+			) &&
+		strcmp(selected_mechanism, SCRAM_SHA_256_PLUS_NAME) != 0)
+	{
+		printfPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("channel binding required for SASL authentication but no valid mechanism could be selected\n"));
+		goto error;
+	}
+
+	/*
+	 * Choose channel binding type depending on user's input.  Note that
+	 * even if channel binding is disabled, the default still falls back
+	 * to tls-unique for simplicity.
+	 */
+	if (strcmp(conn->scram_channel_binding,
+			   SCRAM_CHANNEL_BINDING_TLS_END_POINT) == 0)
+		selected_channel_binding = SCRAM_CHANNEL_BINDING_TLS_END_POINT;
+	else
+		selected_channel_binding = SCRAM_CHANNEL_BINDING_TLS_UNIQUE;
+
+	/*
+	 * Now that the SASL mechanism and channel binding have been chosen for
+	 * the exchange, initialize its state information.
 	 */
 
 	/*
@@ -576,7 +611,8 @@ pg_SASL_init(PGconn *conn, int payloadlen)
 	 */
 	conn->sasl_state = pg_fe_scram_init(conn,
 										password,
-										selected_mechanism);
+										selected_mechanism,
+										selected_channel_binding);
 	if (!conn->sasl_state)
 		goto oom_error;
 
@@ -838,6 +874,21 @@ pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn)
 	switch (areq)
 	{
 		case AUTH_REQ_OK:
+			/*
+			 * If channel binding has been requested for authentication,
+			 * make sure that the client has not been tricked and that
+			 * a full SASL exchange has happened.  This happens at this
+			 * stage as it could be possible that a backend with "trust"
+			 * sends directly this message type.
+			 */
+			if ((strcmp(conn->scram_channel_binding, SCRAM_CHANNEL_BINDING_TLS_UNIQUE) == 0 ||
+				 strcmp(conn->scram_channel_binding, SCRAM_CHANNEL_BINDING_TLS_END_POINT) == 0) &&
+				!pg_fe_scram_exchange_finished(conn))
+			{
+				printfPQExpBuffer(&conn->errorMessage,
+								  libpq_gettext("channel binding required for authentication but invalid protocol was used\n"));
+				return STATUS_ERROR;
+			}
 			break;
 
 		case AUTH_REQ_KRB4:
diff --git a/src/interfaces/libpq/fe-auth.h b/src/interfaces/libpq/fe-auth.h
index a8a27c24a6..76cf6a2f2e 100644
--- a/src/interfaces/libpq/fe-auth.h
+++ b/src/interfaces/libpq/fe-auth.h
@@ -25,11 +25,13 @@ extern char *pg_fe_getauthname(PQExpBuffer errorMessage);
 /* Prototypes for functions in fe-auth-scram.c */
 extern void *pg_fe_scram_init(PGconn *conn,
 				 const char *password,
-				 const char *sasl_mechanism);
+				 const char *sasl_mechanism,
+				 const char *channel_binding_type);
 extern void pg_fe_scram_free(void *opaq);
 extern void pg_fe_scram_exchange(void *opaq, char *input, int inputlen,
 					 char **output, int *outputlen,
 					 bool *done, bool *success);
 extern char *pg_fe_scram_build_verifier(const char *password);
+extern bool pg_fe_scram_exchange_finished(PGconn *conn);
 
 #endif							/* FE_AUTH_H */
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index a7e969d7c1..cfc3e9942f 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -123,7 +123,7 @@ static int ldapServiceLookup(const char *purl, PQconninfoOption *options,
 #define DefaultOption	""
 #define DefaultAuthtype		  ""
 #define DefaultTargetSessionAttrs	"any"
-#define DefaultSCRAMChannelBinding	SCRAM_CHANNEL_BINDING_TLS_UNIQUE
+#define DefaultSCRAMChannelBinding	"prefer"
 #ifdef USE_SSL
 #define DefaultSSLMode "prefer"
 #else
@@ -1166,6 +1166,24 @@ connectOptions2(PGconn *conn)
 			goto oom_error;
 	}
 
+	/* validate scram_channel_binding option */
+	if (conn->scram_channel_binding)
+	{
+		if (strcmp(conn->scram_channel_binding, "prefer") != 0 &&
+			strcmp(conn->scram_channel_binding, "disable") != 0 &&
+			strcmp(conn->scram_channel_binding,
+				   SCRAM_CHANNEL_BINDING_TLS_UNIQUE) != 0 &&
+			strcmp(conn->scram_channel_binding,
+				   SCRAM_CHANNEL_BINDING_TLS_END_POINT) != 0)
+		{
+			conn->status = CONNECTION_BAD;
+			printfPQExpBuffer(&conn->errorMessage,
+							  libpq_gettext("invalid scram_channel_binding value: \"%s\"\n"),
+							  conn->scram_channel_binding);
+			return false;
+		}
+	}
+
 	/*
 	 * Resolve special "auto" client_encoding from the locale
 	 */
diff --git a/src/test/ssl/ServerSetup.pm b/src/test/ssl/ServerSetup.pm
index ced279c31b..f49a04a84c 100644
--- a/src/test/ssl/ServerSetup.pm
+++ b/src/test/ssl/ServerSetup.pm
@@ -27,6 +27,7 @@ use Test::More;
 use Exporter 'import';
 our @EXPORT = qw(
   configure_test_server_for_ssl
+  configure_hba_for_ssl
   switch_server_cert
   test_connect_fails
   test_connect_ok
@@ -129,7 +130,7 @@ sub configure_test_server_for_ssl
 	$node->restart;
 
 	# Change pg_hba after restart because hostssl requires ssl=on
-	configure_hba_for_ssl($node, $serverhost, $authmethod);
+	configure_hba_for_ssl($node, $serverhost, "hostssl", $authmethod);
 }
 
 # Change the configuration to use given server cert file, and reload
@@ -154,7 +155,7 @@ sub switch_server_cert
 
 sub configure_hba_for_ssl
 {
-	my ($node, $serverhost, $authmethod) = @_;
+	my ($node, $serverhost, $hosttype, $authmethod) = @_;
 	my $pgdata = $node->data_dir;
 
 	# Only accept SSL connections from localhost. Our tests don't depend on this
@@ -165,12 +166,17 @@ sub configure_hba_for_ssl
 	print $hba
 	  "# TYPE  DATABASE        USER            ADDRESS                 METHOD\n";
 	print $hba
-	  "hostssl trustdb         all             $serverhost/32            $authmethod\n";
+	  "$hosttype trustdb         all             $serverhost/32            $authmethod\n";
 	print $hba
-	  "hostssl trustdb         all             ::1/128                 $authmethod\n";
-	print $hba
-	  "hostssl certdb          all             $serverhost/32            cert\n";
-	print $hba
-	  "hostssl certdb          all             ::1/128                 cert\n";
+	  "$hosttype trustdb         all             ::1/128                 $authmethod\n";
+	# Certificate authentication is only authorized with hostssl
+	# as entry type.
+	if ($hosttype eq "hostssl")
+	{
+		print $hba
+		  "$hosttype certdb          all             $serverhost/32            cert\n";
+		print $hba
+		  "$hosttype certdb          all             ::1/128                 cert\n";
+	}
 	close $hba;
 }
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index 52a8f458cb..3eba6d71b4 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -13,7 +13,7 @@ if ($ENV{with_openssl} ne 'yes')
 	plan skip_all => 'SSL not supported by this build';
 }
 
-my $number_of_tests = 6;
+my $number_of_tests = 11;
 
 # This is the hostname used to connect to the server.
 my $SERVERHOSTADDR = '127.0.0.1';
@@ -54,7 +54,7 @@ test_connect_ok(
 	$common_connstr,
 	"scram_channel_binding=tls-unique",
 	"SCRAM authentication with tls-unique as channel binding");
-test_connect_ok($common_connstr, "scram_channel_binding=''",
+test_connect_ok($common_connstr, "scram_channel_binding='disable'",
 	"SCRAM authentication without channel binding");
 if ($supports_tls_server_end_point)
 {
@@ -75,7 +75,39 @@ else
 test_connect_fails(
 	$common_connstr,
 	"scram_channel_binding=not-exists",
-	qr/unsupported SCRAM channel-binding type/,
+	qr/invalid scram_channel_binding value/,
 	"SCRAM authentication with invalid channel binding");
 
+# Downgrade attack tests
+# Update server's pg_hba.conf with "trust" level and try to
+# connect to the server with tls-unique.
+configure_hba_for_ssl($node, $SERVERHOSTADDR, "hostssl", "trust");
+$node->reload;
+test_connect_fails(
+	$common_connstr,
+	"scram_channel_binding=tls-unique",
+	qr/channel binding required for authentication but invalid protocol was used/,
+	"Connection to trust server refused with required tls-unique");
+
+# Now test "md5" level, which should pass as user has a SCRAM verifier.
+configure_hba_for_ssl($node, $SERVERHOSTADDR, "hostssl", "md5");
+$node->reload;
+test_connect_ok($common_connstr, "scram_channel_binding=tls-unique",
+	"SCRAM authentication with channel binding required");
+
+# Now a tricky one, update pg_hba.conf so as non-SSL connections
+# are authorized.  Requiring channel binding should fail, with
+# incorrect SASL mechanism received from server.  Note that this
+# can trigger only with SCRAM used without SSL, or pre-10 servers
+# which have no channel binding support.
+configure_hba_for_ssl($node, $SERVERHOSTADDR, "hostnossl", "scram-sha-256");
+$node->reload;
+$common_connstr =
+  "user=ssltestuser dbname=trustdb sslmode=disable hostaddr=$SERVERHOSTADDR";
+test_connect_fails(
+	$common_connstr,
+	"scram_channel_binding=tls-unique",
+	qr/channel binding required for SASL authentication but no valid mechanism could be selected/,
+	"Connection to trust server refused with required tls-unique");
+
 done_testing($number_of_tests);
-- 
2.17.0