From d6e612f837e235db0411e8b67558c9a6b3e9f41f Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Mon, 23 Sep 2019 13:45:23 -0700 Subject: [PATCH] Add libpq parameter 'channel_binding'. Allow clients to require channel binding to enhance security against untrusted servers. Author: Jeff Davis Reviewed-by: Michael Paquier Discussion: https://postgr.es/m/227015d8417f2b4fef03f8966dbfa5cbcc4f44da.camel%40j-davis.com --- doc/src/sgml/libpq.sgml | 32 ++++++++++ src/interfaces/libpq/fe-auth-scram.c | 41 ++++++++++-- src/interfaces/libpq/fe-auth.c | 76 +++++++++++++++++++++-- src/interfaces/libpq/fe-auth.h | 1 + src/interfaces/libpq/fe-connect.c | 41 +++++++++++- src/interfaces/libpq/libpq-int.h | 2 + src/test/authentication/t/001_password.pl | 12 +++- src/test/ssl/t/002_scram.pl | 39 +++++++++++- src/test/ssl/t/SSLServer.pm | 9 ++- 9 files changed, 233 insertions(+), 20 deletions(-) diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 1189341ca1..c58527b0c3 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -1122,6 +1122,28 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname + + channel_binding + + + This option controls the client's use of channel binding. A setting + of require means that the connection must employ + channel binding, prefer means that the client will + choose channel binding if available, and disable + prevents the use of channel binding. The default + is prefer if + PostgreSQL is compiled with SSL support; + otherwise the default is disable. + + + Channel binding is a method for the server to authenticate itself to + the client. It is only supported over SSL connections + with PostgreSQL 11 or later servers using + the SCRAM authentication method. + + + + connect_timeout @@ -6864,6 +6886,16 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough) + + + + PGCHANNELBINDING + + PGCHANNELBINDING behaves the same as the connection parameter. + + + diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c index 7a8335bf9f..693739c544 100644 --- a/src/interfaces/libpq/fe-auth-scram.c +++ b/src/interfaces/libpq/fe-auth-scram.c @@ -119,6 +119,35 @@ pg_fe_scram_init(PGconn *conn, return state; } +/* + * Return true if channel binding was employed and the SCRAM exchange + * completed. This should be used after a successful exchange to determine + * whether the server authenticated itself to the client. + * + * Note that the caller must also ensure that the exchange was actually + * successful. + */ +bool +pg_fe_scram_channel_bound(void *opaq) +{ + fe_scram_state *state = (fe_scram_state *) opaq; + + /* no SCRAM exchange done */ + if (state == NULL) + return false; + + /* SCRAM exchange not completed */ + if (state->state != FE_SCRAM_FINISHED) + return false; + + /* channel binding mechanism not used */ + if (strcmp(state->sasl_mechanism, SCRAM_SHA_256_PLUS_NAME) != 0) + return false; + + /* all clear! */ + return true; +} + /* * Free SCRAM exchange status */ @@ -225,9 +254,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 +385,8 @@ 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->channel_binding[0] != 'd' && /* disable */ + conn->ssl_in_use) { /* * Client supports channel binding, but thinks the server does not. @@ -369,7 +397,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 +526,8 @@ 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->channel_binding[0] != 'd' && /* disable */ + conn->ssl_in_use) 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 ab227421b3..cd29e8bd12 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, @@ -454,10 +462,10 @@ 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. + * else if a channel binding type is set and if the client supports 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_PLUS_NAME) != 0) + { + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("channel binding is required, but server did not offer an authentication method that supports channel binding\n")); + goto error; + } + if (!selected_mechanism) { printfPQExpBuffer(&conn->errorMessage, @@ -774,6 +791,50 @@ 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 (!pg_fe_scram_channel_bound(conn->sasl_state)) + { + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("Channel binding required, but server authenticated client without channel binding\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 +849,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-auth.h b/src/interfaces/libpq/fe-auth.h index 122ba5ccba..2f1af53fb0 100644 --- a/src/interfaces/libpq/fe-auth.h +++ b/src/interfaces/libpq/fe-auth.h @@ -26,6 +26,7 @@ extern char *pg_fe_getauthname(PQExpBuffer errorMessage); extern void *pg_fe_scram_init(PGconn *conn, const char *password, const char *sasl_mechanism); +extern bool pg_fe_scram_channel_bound(void *opaq); extern void pg_fe_scram_free(void *opaq); extern void pg_fe_scram_exchange(void *opaq, char *input, int inputlen, char **output, int *outputlen, diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 8ba0159313..f91f0f2efe 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)}, @@ -1197,6 +1206,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 */ @@ -3485,10 +3517,11 @@ keep_going: /* We will come back to here until there is case CONNECTION_SETENV: { /* - * Do post-connection housekeeping (only needed in protocol 2.0). + * Do post-connection housekeeping (only needed in protocol + * 2.0). * - * We pretend that the connection is OK for the duration of these - * queries. + * We pretend that the connection is OK for the duration of + * these queries. */ conn->status = CONNECTION_OK; @@ -3905,6 +3938,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 d37bb3ce40..64468ab4da 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -347,6 +347,8 @@ 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,disable) */ char *keepalives; /* use TCP keepalives? */ char *keepalives_idle; /* time between TCP keepalives */ char *keepalives_interval; /* time between TCP keepalive diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl index 3a3b0eb7e8..aae6de8b34 100644 --- a/src/test/authentication/t/001_password.pl +++ b/src/test/authentication/t/001_password.pl @@ -17,7 +17,7 @@ if ($windows_os) } else { - plan tests => 8; + plan tests => 10; } @@ -86,3 +86,13 @@ test_role($node, 'md5_role', 'scram-sha-256', 2); reset_pg_hba($node, 'md5'); test_role($node, 'scram_role', 'md5', 0); test_role($node, 'md5_role', 'md5', 0); + +# Tests for channel binding without SSL. +# Using the password authentication method; channel binding can't work +reset_pg_hba($node, 'password'); +$ENV{"PGCHANNELBINDING"} = 'require'; +test_role($node, 'scram_role', 'scram-sha-256', 2); +# SSL not in use; channel binding still can't work +reset_pg_hba($node, 'scram-sha-256'); +$ENV{"PGCHANNELBINDING"} = 'require'; +test_role($node, 'scram_role', 'scram-sha-256', 2); diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl index 7c4b821cb7..5fa2dbde1c 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 = 9; # This is the hostname used to connect to the server. my $SERVERHOSTADDR = '127.0.0.1'; @@ -44,9 +44,42 @@ configure_test_server_for_ssl($node, $SERVERHOSTADDR, "scram-sha-256", switch_server_cert($node, 'server-cn-only'); $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_connect_ok($common_connstr, "user=ssltestuser", + "Basic SCRAM authentication with SSL"); + +# Test channel_binding +test_connect_fails( + $common_connstr, + "user=ssltestuser channel_binding=invalid_value", + qr/invalid channel_binding value: "invalid_value"/, + "SCRAM with SSL and channel_binding=invalid_value"); +test_connect_ok( + $common_connstr, + "user=ssltestuser channel_binding=disable", + "SCRAM with SSL and channel_binding=disable"); +test_connect_ok( + $common_connstr, + "user=ssltestuser channel_binding=require", + "SCRAM with SSL and channel_binding=require"); + +# Now test when the user has an MD5-encrypted password; should fail +test_connect_fails( + $common_connstr, + "user=md5testuser channel_binding=require", + qr/Channel binding required but not supported by server's authentication request/, + "MD5 with SSL and channel_binding=require"); + +# Now test with auth method 'cert' by connecting to 'certdb'. Should +# fail, because channel binding is not performed. +copy("ssl/client.key", "ssl/client_tmp.key"); +chmod 0600, "ssl/client_tmp.key"; +test_connect_fails( + "sslcert=ssl/client.crt sslkey=ssl/client_tmp.key hostaddr=$SERVERHOSTADDR", + "dbname=certdb user=ssltestuser channel_binding=require", + qr/Channel binding required, but server authenticated client without channel binding/, + "Cert authentication 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 d25c38dbbc..005955a2ff 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"); @@ -114,6 +115,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.40.0