On Wed, 2019-08-21 at 16:12 +0900, Michael Paquier wrote:
> This counts for other auth requests as well like krb5, no? I think
> that we should also add the "disable" flavor for symmetry, and
> also...
Added "disable" option, and I refactored so that it would look
explicitly for an expected auth req based on the connection parameters.
I had to also make the client tell the server that it does not support
channel binding if channel_binding=disable, otherwise the server
complains.
> +#define DefaultChannelBinding "prefer"
> If the build of Postgres does not support SSL (USE_SSL is not
> defined), I think that DefaultChannelBinding should be "disable".
> That would make things consistent with sslmode.
Done.
> Doing the filtering at the time of AUTH_REQ_OK is necessary for
> "trust", but shouldn't this be done as well earlier for other request
> types? This could save round-trips to the server if we know that an
> exchange begins with a protocol which will never satisfy this
> request, saving efforts for a client doomed to fail after the first
> AUTH_REQ received. That would be the case for all AUTH_REQ, except
> the SASL ones and of course AUTH_REQ_OK.
Done.
>
> Could you please add negative tests in
> src/test/authentication/? What
> could be covered there is that the case where "prefer" (and
> "disable"?) is defined then the authentication is able to go through,
> and that with "require" we get a proper failure as SSL is not used.
> Tests in src/test/ssl/ could include:
> - Make sure that "require" works properly.
> - Test after "disable".
Done.
> + if (conn->channel_binding[0] == 'r')
> Maybe this should comment that this means "require", in a fashion
> similar to what is done when checking conn->sslmode.
Done.
New patch attached.
Regards,
Jeff Davis
From f3743a9a7c59b628249986d8b7252dfb13e1952d Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Tue, 20 Aug 2019 18:59:23 -0700
Subject: [PATCH] Add libpq parameter 'channel_binding'.
Allow clients to require channel binding to enhance security against
untrusted servers.
Author: Jeff Davis
Discussion: https://postgr.es/m/227015d8417f2b4fef03f8966dbfa5cbcc4f44da.camel%40j-davis.com
---
doc/src/sgml/libpq.sgml | 22 +++++++
src/interfaces/libpq/fe-auth-scram.c | 6 +-
src/interfaces/libpq/fe-auth.c | 71 +++++++++++++++++++++--
src/interfaces/libpq/fe-connect.c | 33 +++++++++++
src/interfaces/libpq/libpq-int.h | 2 +
src/test/authentication/t/002_saslprep.pl | 12 +++-
src/test/ssl/t/002_scram.pl | 8 ++-
7 files changed, 144 insertions(+), 10 deletions(-)
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index b7c3d96b01f..bc03a171f53 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1118,6 +1118,28 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</listitem>
</varlistentry>
+ <varlistentry id="libpq-connect-channel-binding" xreflabel="channel_binding">
+ <term><literal>channel_binding</literal></term>
+ <listitem>
+ <para>
+ This option controls the client's use of channel binding. A setting
+ of <literal>require</literal> means that the connection must employ
+ channel binding, <literal>prefer</literal> means that the client will
+ choose channel binding if available, and <literal>disable</literal>
+ prevents the use of channel binding. The default
+ is <literal>prefer</literal> if
+ <productname>PostgreSQL</productname> is compiled with SSL support;
+ otherwise the default is <literal>disable</literal>.
+ </para>
+ <para>
+ Channel binding is a method for the server to authenticate itself to
+ the client. It is only supported over SSL connections
+ with <productname>PostgreSQL</productname> 11 or later servers using
+ the <literal>scram-sha-256</literal> authentication method.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-connect-connect-timeout" xreflabel="connect_timeout">
<term><literal>connect_timeout</literal></term>
<listitem>
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index 7a8335bf9f8..1232aa8c6d2 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -358,7 +358,7 @@ build_client_first_message(fe_scram_state *state)
appendPQExpBufferStr(&buf, "p=tls-server-end-point");
}
#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
- else if (conn->ssl_in_use)
+ else if (conn->ssl_in_use && conn->channel_binding[0] != 'd')
{
/*
* Client supports channel binding, but thinks the server does not.
@@ -369,7 +369,7 @@ build_client_first_message(fe_scram_state *state)
else
{
/*
- * Client does not support channel binding.
+ * Client does not support channel binding, or has disabled it.
*/
appendPQExpBufferChar(&buf, 'n');
}
@@ -498,7 +498,7 @@ build_client_final_message(fe_scram_state *state)
#endif /* HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH */
}
#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
- else if (conn->ssl_in_use)
+ else if (conn->ssl_in_use && conn->channel_binding[0] != 'd')
appendPQExpBufferStr(&buf, "c=eSws"); /* base64 of "y,," */
#endif
else
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index ab227421b3b..dd883b6d8e9 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -423,6 +423,14 @@ pg_SASL_init(PGconn *conn, int payloadlen)
initPQExpBuffer(&mechanism_buf);
+ if (conn->channel_binding[0] == 'r' && /* require */
+ !conn->ssl_in_use)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("Channel binding required, but SSL not in use\n"));
+ goto error;
+ }
+
if (conn->sasl_state)
{
printfPQExpBuffer(&conn->errorMessage,
@@ -455,11 +463,12 @@ pg_SASL_init(PGconn *conn, int payloadlen)
/*
* Select the mechanism to use. Pick SCRAM-SHA-256-PLUS over anything
* else if a channel binding type is set and if the client supports
- * it. Pick SCRAM-SHA-256 if nothing else has already been picked. If
- * we add more mechanisms, a more refined priority mechanism might
- * become necessary.
+ * it (and did not set channel_binding=disable). Pick SCRAM-SHA-256 if
+ * nothing else has already been picked. If we add more mechanisms, a
+ * more refined priority mechanism might become necessary.
*/
- if (strcmp(mechanism_buf.data, SCRAM_SHA_256_PLUS_NAME) == 0)
+ if (conn->channel_binding[0] != 'd' &&
+ strcmp(mechanism_buf.data, SCRAM_SHA_256_PLUS_NAME) == 0)
{
if (conn->ssl_in_use)
{
@@ -488,7 +497,8 @@ pg_SASL_init(PGconn *conn, int payloadlen)
goto error;
}
}
- else if (strcmp(mechanism_buf.data, SCRAM_SHA_256_NAME) == 0 &&
+ else if (conn->channel_binding[0] != 'r' && /* require */
+ strcmp(mechanism_buf.data, SCRAM_SHA_256_NAME) == 0 &&
!selected_mechanism)
selected_mechanism = SCRAM_SHA_256_NAME;
}
@@ -565,6 +575,9 @@ pg_SASL_init(PGconn *conn, int payloadlen)
if (initialresponse)
free(initialresponse);
+ if (strcmp(selected_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0)
+ conn->channel_bound = true;
+
return STATUS_OK;
error:
@@ -774,6 +787,51 @@ pg_password_sendauth(PGconn *conn, const char *password, AuthRequest areq)
return ret;
}
+/*
+ * Verify that the authentication request is expected, given the connection
+ * parameters. This is especially important when the client wishes to
+ * authenticate the server before any sensitive information is exchanged.
+ */
+static bool
+check_expected_areq(AuthRequest areq, PGconn *conn)
+{
+ bool result = true;
+
+ /*
+ * When channel_binding=require, we must protect against two cases:
+ *
+ * 1. we must not respond to non-SASL authentication requests, which might
+ * leak information such as the client's password; and
+ * 2. even if we receive AUTH_REQ_OK, we still must ensure that channel
+ * binding has happened in order to authenticate the server
+ */
+ if (conn->channel_binding[0] == 'r' /* require */)
+ {
+ switch (areq)
+ {
+ case AUTH_REQ_SASL:
+ case AUTH_REQ_SASL_CONT:
+ case AUTH_REQ_SASL_FIN:
+ break;
+ case AUTH_REQ_OK:
+ if (!conn->channel_bound)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("Channel binding required but not offered by server\n"));
+ result = false;
+ }
+ break;
+ default:
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("Channel binding required but not supported by server's authentication request\n"));
+ result = false;
+ break;
+ }
+ }
+
+ return result;
+}
+
/*
* pg_fe_sendauth
* client demux routine for processing an authentication request
@@ -788,6 +846,9 @@ pg_password_sendauth(PGconn *conn, const char *password, AuthRequest areq)
int
pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn)
{
+ if (!check_expected_areq(areq, conn))
+ return STATUS_ERROR;
+
switch (areq)
{
case AUTH_REQ_OK:
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 7f1fd2f45eb..3f33dcdbfd6 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -124,6 +124,11 @@ static int ldapServiceLookup(const char *purl, PQconninfoOption *options,
#define DefaultTty ""
#define DefaultOption ""
#define DefaultAuthtype ""
+#ifdef USE_SSL
+#define DefaultChannelBinding "prefer"
+#else
+#define DefaultChannelBinding "disable"
+#endif
#define DefaultTargetSessionAttrs "any"
#ifdef USE_SSL
#define DefaultSSLMode "prefer"
@@ -211,6 +216,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"Database-Password-File", "", 64,
offsetof(struct pg_conn, pgpassfile)},
+ {"channel_binding", "PGCHANNELBINDING", NULL, NULL,
+ "Channel-Binding", "", 7, /* sizeof("require") */
+ offsetof(struct pg_conn, channel_binding)},
+
{"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL,
"Connect-timeout", "", 10, /* strlen(INT32_MAX) == 10 */
offsetof(struct pg_conn, connect_timeout)},
@@ -556,6 +565,7 @@ pqDropServerData(PGconn *conn)
conn->last_sqlstate[0] = '\0';
conn->auth_req_received = false;
conn->password_needed = false;
+ conn->channel_bound = false;
conn->write_failed = false;
if (conn->write_err_msg)
free(conn->write_err_msg);
@@ -1197,6 +1207,29 @@ connectOptions2(PGconn *conn)
}
}
+ /*
+ * validate channel_binding option
+ */
+ if (conn->channel_binding)
+ {
+ if (strcmp(conn->channel_binding, "disable") != 0
+ && strcmp(conn->channel_binding, "prefer") != 0
+ && strcmp(conn->channel_binding, "require") != 0)
+ {
+ conn->status = CONNECTION_BAD;
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("invalid channel_binding value: \"%s\"\n"),
+ conn->channel_binding);
+ return false;
+ }
+ }
+ else
+ {
+ conn->channel_binding = strdup(DefaultChannelBinding);
+ if (!conn->channel_binding)
+ goto oom_error;
+ }
+
/*
* validate sslmode option
*/
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index d37bb3ce404..70a5b445d72 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -347,6 +347,7 @@ struct pg_conn
char *pguser; /* Postgres username and password, if any */
char *pgpass;
char *pgpassfile; /* path to a file containing password(s) */
+ char *channel_binding; /* channel binding mode (require,prefer) */
char *keepalives; /* use TCP keepalives? */
char *keepalives_idle; /* time between TCP keepalives */
char *keepalives_interval; /* time between TCP keepalive
@@ -410,6 +411,7 @@ struct pg_conn
int sversion; /* server version, e.g. 70401 for 7.4.1 */
bool auth_req_received; /* true if any type of auth req received */
bool password_needed; /* true if server demanded a password */
+ bool channel_bound; /* true if channel_binding happened */
bool sigpipe_so; /* have we masked SIGPIPE via SO_NOSIGPIPE? */
bool sigpipe_flag; /* can we mask SIGPIPE via MSG_NOSIGNAL? */
bool write_failed; /* have we had a write failure on sock? */
diff --git a/src/test/authentication/t/002_saslprep.pl b/src/test/authentication/t/002_saslprep.pl
index c4b335c45fe..37f6d19c3a1 100644
--- a/src/test/authentication/t/002_saslprep.pl
+++ b/src/test/authentication/t/002_saslprep.pl
@@ -14,7 +14,7 @@ if ($windows_os)
}
else
{
- plan tests => 12;
+ plan tests => 14;
}
# Delete pg_hba.conf from the given node, add a new entry to it
@@ -104,3 +104,13 @@ test_login($node, 'saslpreptest6_role', "foobar", 2);
test_login($node, 'saslpreptest7_role', "foo\xd8\xa71bar", 0);
test_login($node, 'saslpreptest7_role', "foo1\xd8\xa7bar", 2);
test_login($node, 'saslpreptest7_role', "foobar", 2);
+
+# using the password authentication method; channel binding can't work
+reset_pg_hba($node, 'password');
+$ENV{"PGCHANNELBINDING"} = 'require';
+test_login($node, 'saslpreptest4a_role', "a", 2);
+
+# SSL not in use; channel binding still can't work
+reset_pg_hba($node, 'scram-sha-256');
+$ENV{"PGCHANNELBINDING"} = 'require';
+test_login($node, 'saslpreptest4a_role', "a", 2);
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index 7c4b821cb78..2b834c11ed4 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -18,7 +18,7 @@ if ($ENV{with_openssl} ne 'yes')
plan skip_all => 'SSL not supported by this build';
}
-my $number_of_tests = 1;
+my $number_of_tests = 3;
# This is the hostname used to connect to the server.
my $SERVERHOSTADDR = '127.0.0.1';
@@ -49,4 +49,10 @@ $common_connstr =
# Default settings
test_connect_ok($common_connstr, '', "Basic SCRAM authentication with SSL");
+# Test channel_binding
+$ENV{PGCHANNELBINDING} = "disable";
+test_connect_ok($common_connstr, '', "SCRAM with SSL and channel_binding=disable");
+$ENV{PGCHANNELBINDING} = "require";
+test_connect_ok($common_connstr, '', "SCRAM with SSL and channel_binding=require");
+
done_testing($number_of_tests);
--
2.17.1
