On Fri, 2019-09-06 at 16:05 +0900, Michael Paquier wrote:
> Nit here: "scram-sha-256" refers to the HBA entry. I would
> just use "SCRAM" instead.
Done.
> In pg_SASL_init(), if the server sends SCRAM-SHA-256-PLUS as SASL
> mechanism over a non-SSL connection, should we complain even if
> the "disable" mode is used? It seems to me that there is a point to
> complain in this case as a sanity check as the server should really
> publicize "SCRAM-SHA-256-PLUS" only over SSL.
Done.
> When the server only sends SCRAM-SHA-256 in the mechanism list and
> "require" mode is used, we complain about "none of the server's SASL
> authentication mechanisms are supported" which can be confusing. Why
> not generating a custom error if libpq selects SCRAM-SHA-256 when
> "require" is used, say:
> "SASL authentication mechanism SCRAM-SHA-256 selected but channel
> binding is required"
> That could be done by adding an error message when selecting
> SCRAM-SHA-256 and then goto the error step.
Done.
> Actually, it looks that the handling of channel_bound is incorrect.
> If the server sends AUTH_REQ_SASL and libpq processes it, then the
> flag gets already set. Now let's imagine that the server is a rogue
> one and sends AUTH_REQ_OK just after AUTH_REQ_SASL, then your check
> would pass. It seems to me that we should switch the flag once we
> are
> sure that the exchange is completed, aka with AUTH_REQ_SASL_FIN when
> the final message is done within pg_SASL_continue().
Thank you! Fixed. I now track whether channel binding is selected, and
also whether SASL actually finished successfully.
> +# 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);
> Worth testing md5 here?
I added a new md5 test in the ssl test suite. Testing it in the non-SSL
path doesn't seem like it adds much.
> PGCHANNELBINDING needs documentation.
Done.
Regards,
Jeff Davis
From 8b753cc78834825d7e10321da369127aadb7cfca Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
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 | 32 +++++++++
src/interfaces/libpq/fe-auth-scram.c | 10 ++-
src/interfaces/libpq/fe-auth.c | 82 +++++++++++++++++++++--
src/interfaces/libpq/fe-connect.c | 36 ++++++++++
src/interfaces/libpq/libpq-int.h | 3 +
src/test/authentication/t/002_saslprep.pl | 12 +++-
src/test/ssl/t/002_scram.pl | 15 ++++-
src/test/ssl/t/SSLServer.pm | 9 ++-
8 files changed, 184 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 0a187e256a4..be9cd4ade75 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1122,6 +1122,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</literal> authentication method.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-connect-connect-timeout" xreflabel="connect_timeout">
<term><literal>connect_timeout</literal></term>
<listitem>
@@ -6864,6 +6886,16 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
</para>
</listitem>
+ <listitem>
+ <para>
+ <indexterm>
+ <primary><envar>PGCHANNELBINDING</envar></primary>
+ </indexterm>
+ <envar>PGCHANNELBINDING</envar> behaves the same as the <xref
+ linkend="libpq-connect-channel-binding"/> connection parameter.
+ </para>
+ </listitem>
+
<listitem>
<para>
<indexterm>
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index 7a8335bf9f8..875cd07bc05 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -225,9 +225,7 @@ pg_fe_scram_exchange(void *opaq, char *input, int inputlen,
/*
* Verify server signature, to make sure we're talking to the
- * genuine server. XXX: A fake server could simply not require
- * authentication, though. There is currently no option in libpq
- * to reject a connection, if SCRAM authentication did not happen.
+ * genuine server.
*/
if (verify_server_signature(state))
*success = true;
@@ -358,7 +356,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 +367,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 +496,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..3394ce5a142 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,9 +463,9 @@ 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)
{
@@ -466,10 +474,11 @@ pg_SASL_init(PGconn *conn, int payloadlen)
/*
* The server has offered SCRAM-SHA-256-PLUS, which is only
* supported by the client if a hash of the peer certificate
- * can be created.
+ * can be created, and if channel_binding is not disabled.
*/
#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
- selected_mechanism = SCRAM_SHA_256_PLUS_NAME;
+ if (conn->channel_binding[0] != 'd') /* disable */
+ selected_mechanism = SCRAM_SHA_256_PLUS_NAME;
#endif
}
else
@@ -493,6 +502,14 @@ pg_SASL_init(PGconn *conn, int payloadlen)
selected_mechanism = SCRAM_SHA_256_NAME;
}
+ if (conn->channel_binding[0] == 'r' && /* require */
+ strcmp(selected_mechanism, SCRAM_SHA_256_NAME) == 0)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("SASL authentication mechanism SCRAM-SHA-256 selected but channel binding is required\n"));
+ goto error;
+ }
+
if (!selected_mechanism)
{
printfPQExpBuffer(&conn->errorMessage,
@@ -565,6 +582,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 +794,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 || !conn->sasl_finished)
+ {
+ 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 +853,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:
@@ -968,6 +1036,10 @@ pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn)
"fe_sendauth: error in SASL authentication\n");
return STATUS_ERROR;
}
+
+ if (areq == AUTH_REQ_SASL_FIN)
+ conn->sasl_finished = true;
+
break;
case AUTH_REQ_SCM_CREDS:
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 8ba0159313c..7113a47bd34 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)},
@@ -500,6 +509,8 @@ pqDropConnection(PGconn *conn, bool flushInput)
pg_fe_scram_free(conn->sasl_state);
conn->sasl_state = NULL;
}
+ conn->sasl_finished = false;
+ conn->channel_bound = false;
}
@@ -1197,6 +1208,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
*/
@@ -3905,6 +3939,8 @@ freePGconn(PGconn *conn)
}
if (conn->pgpassfile)
free(conn->pgpassfile);
+ if (conn->channel_binding)
+ free(conn->channel_binding);
if (conn->keepalives)
free(conn->keepalives);
if (conn->keepalives_idle)
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index d37bb3ce404..d6b7d3db013 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? */
@@ -462,6 +464,7 @@ struct pg_conn
/* Assorted state for SASL, SSL, GSS, etc */
void *sasl_state;
+ bool sasl_finished;
/* SSL structures */
bool ssl_in_use;
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..af508e74f5c 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 = 5;
# This is the hostname used to connect to the server.
my $SERVERHOSTADDR = '127.0.0.1';
@@ -42,11 +42,22 @@ $node->start;
configure_test_server_for_ssl($node, $SERVERHOSTADDR, "scram-sha-256",
"pass", "scram-sha-256");
switch_server_cert($node, 'server-cn-only');
+$ENV{PGUSER} = "ssltestuser";
$ENV{PGPASSWORD} = "pass";
$common_connstr =
- "user=ssltestuser dbname=trustdb sslmode=require sslcert=invalid sslrootcert=invalid hostaddr=$SERVERHOSTADDR";
+ "dbname=trustdb sslmode=require sslcert=invalid sslrootcert=invalid hostaddr=$SERVERHOSTADDR";
# 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");
+
+# Now test when the user has an MD5-encrypted password; should fail
+$ENV{PGUSER} = "md5testuser";
+test_connect_fails($common_connstr, '', qr/Channel binding required but not supported by server's authentication request/, "MD5 with SSL and channel_binding=require");
+
done_testing($number_of_tests);
diff --git a/src/test/ssl/t/SSLServer.pm b/src/test/ssl/t/SSLServer.pm
index d25c38dbbc7..9288b428115 100644
--- a/src/test/ssl/t/SSLServer.pm
+++ b/src/test/ssl/t/SSLServer.pm
@@ -102,6 +102,7 @@ sub configure_test_server_for_ssl
# Create test users and databases
$node->psql('postgres', "CREATE USER ssltestuser");
+ $node->psql('postgres', "CREATE USER md5testuser");
$node->psql('postgres', "CREATE USER anotheruser");
$node->psql('postgres', "CREATE USER yetanotheruser");
$node->psql('postgres', "CREATE DATABASE trustdb");
@@ -113,6 +114,10 @@ sub configure_test_server_for_ssl
{
$node->psql('postgres',
"SET password_encryption='$password_enc'; ALTER USER ssltestuser PASSWORD '$password';"
+ );
+ # A special user that always has an md5-encrypted password
+ $node->psql('postgres',
+ "SET password_encryption='md5'; ALTER USER md5testuser PASSWORD '$password';"
);
$node->psql('postgres',
"SET password_encryption='$password_enc'; ALTER USER anotheruser PASSWORD '$password';"
@@ -128,7 +133,7 @@ sub configure_test_server_for_ssl
print $conf "log_statement=all\n";
# enable SSL and set up server key
- print $conf "include 'sslconfig.conf'";
+ print $conf "include 'sslconfig.conf'\n";
close $conf;
@@ -186,6 +191,8 @@ sub configure_hba_for_ssl
open my $hba, '>', "$pgdata/pg_hba.conf";
print $hba
"# TYPE DATABASE USER ADDRESS METHOD OPTIONS\n";
+ print $hba
+ "hostssl trustdb md5testuser $serverhost/32 md5\n";
print $hba
"hostssl trustdb all $serverhost/32 $authmethod\n";
print $hba
--
2.17.1
