diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index caab9700b8..c24a69f00c 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -1245,34 +1245,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname - - scram_channel_binding - - - Specifies the channel binding type to use with SCRAM - authentication. While SCRAM alone prevents - the replay of transmitted hashed passwords, channel binding also - prevents man-in-the-middle attacks. - - - - The list of channel binding types supported by the server are - listed in . An empty value - specifies that the client will not use channel binding. If this - parameter is not specified, tls-unique 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. - - - - 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. - - - - replication diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index 46d7e19f10..f0b2145208 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -1576,12 +1576,8 @@ the password is in. Channel binding is supported in PostgreSQL builds with SSL support. The SASL mechanism name for SCRAM with channel binding is -SCRAM-SHA-256-PLUS. Two channel binding types are -supported: tls-unique and -tls-server-end-point, both defined in RFC 5929. Clients -should use tls-unique if they can support it. -tls-server-end-point is intended for third-party clients -that cannot support tls-unique for some reason. +SCRAM-SHA-256-PLUS. The channel binding type used by +PostgreSQL is tls-server-end-point. @@ -1596,19 +1592,11 @@ that cannot support tls-unique for some reason. SCRAM with channel binding prevents such - man-in-the-middle attacks by mixing a value into the transmitted - password hash that cannot be retransmitted by a fake server. - In SCRAM with tls-unique - channel binding, the shared secret negotiated during the SSL session - is mixed into the user-supplied password hash. The shared secret - is partly chosen by the server, but not directly transmitted, making - it impossible for a fake server to create an SSL connection with the - client that has the same shared secret it has with the real server. - SCRAM with tls-server-end-point - mixes a hash of the server's certificate into the user-supplied password - hash. While a fake server can retransmit the real server's certificate, - it doesn't have access to the private key matching that certificate, and - therefore cannot prove it is the owner, causing SSL connection failure. + man-in-the-middle attacks by mixing the signature of the server's + certificate into the transmitted password hash. While a fake server can + retransmit the real server's certificate, it doesn't have access to the + private key matching that certificate, and therefore cannot prove it is + the owner, causing SSL connection failure. diff --git a/doc/src/sgml/release-11.sgml b/doc/src/sgml/release-11.sgml index 95e6e06cd3..9723bc2d1f 100644 --- a/doc/src/sgml/release-11.sgml +++ b/doc/src/sgml/release-11.sgml @@ -2693,10 +2693,7 @@ same commits as above the feature currently does not prevent man-in-the-middle attacks when using libpq and interfaces built using it. It is expected that future versions of libpq and interfaces not built - using libpq, e.g. JDBC, will allow this capability. The libpq - options to control the optional channel binding type are - and . + using libpq, e.g. JDBC, will allow this capability. diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c index 48eb531d0f..8fbad53fa8 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,8 +168,38 @@ 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 * @@ -164,13 +207,19 @@ static char *scram_mock_salt(const char *username); * 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. @@ -655,6 +724,36 @@ sanitize_char(char c) return buf; } +/* + * 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. * @@ -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->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."))); - 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"))); - } + channel_binding_type = read_attr_value(&input, 'p'); - /* - * 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); - } + /* + * 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 { diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index 63f37902e6..cecd104b4a 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -862,8 +862,7 @@ CheckMD5Auth(Port *port, char *shadow_pass, char **logdetail) static int CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail) { - char *sasl_mechs; - char *p; + StringInfoData sasl_mechs; int mtype; StringInfoData buf; void *scram_opaq; @@ -889,42 +888,16 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail) /* * Send the SASL authentication request to user. It includes the list of - * authentication mechanisms that are supported. The order of mechanisms - * is advertised in decreasing order of importance. So the - * channel-binding variants go first, if they are supported. Channel - * binding is only supported in SSL builds. + * authentication mechanisms that are supported. */ - sasl_mechs = palloc(strlen(SCRAM_SHA_256_PLUS_NAME) + - strlen(SCRAM_SHA_256_NAME) + 3); - p = sasl_mechs; - - if (port->ssl_in_use) - { - strcpy(p, SCRAM_SHA_256_PLUS_NAME); - p += strlen(SCRAM_SHA_256_PLUS_NAME) + 1; - } - - strcpy(p, SCRAM_SHA_256_NAME); - p += strlen(SCRAM_SHA_256_NAME) + 1; + initStringInfo(&sasl_mechs); + pg_be_scram_get_mechanisms(port, &sasl_mechs); /* Put another '\0' to mark that list is finished. */ - p[0] = '\0'; + appendStringInfoChar(&sasl_mechs, '\0'); - sendAuthRequest(port, AUTH_REQ_SASL, sasl_mechs, p - sasl_mechs + 1); - pfree(sasl_mechs); - - /* - * Initialize the status tracker for message exchanges. - * - * If the user doesn't exist, or doesn't have a valid password, or it's - * expired, we still go through the motions of SASL authentication, but - * tell the authentication method that the authentication is "doomed". - * That is, it's going to fail, no matter what. - * - * This is because we don't want to reveal to an attacker what usernames - * are valid, nor which users have a valid password. - */ - scram_opaq = pg_be_scram_init(port, shadow_pass); + sendAuthRequest(port, AUTH_REQ_SASL, sasl_mechs.data, sasl_mechs.len); + pfree(sasl_mechs.data); /* * Loop through SASL message exchange. This exchange can consist of @@ -973,13 +946,20 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail) const char *selected_mech; selected_mech = pq_getmsgrawstring(&buf); - if (strcmp(selected_mech, SCRAM_SHA_256_NAME) != 0 && - strcmp(selected_mech, SCRAM_SHA_256_PLUS_NAME) != 0) - { - ereport(ERROR, - (errcode(ERRCODE_PROTOCOL_VIOLATION), - errmsg("client selected an invalid SASL authentication mechanism"))); - } + + /* + * Initialize the status tracker for message exchanges. + * + * If the user doesn't exist, or doesn't have a valid password, or + * it's expired, we still go through the motions of SASL + * authentication, but tell the authentication method that the + * authentication is "doomed". That is, it's going to fail, no + * matter what. + * + * This is because we don't want to reveal to an attacker what + * usernames are valid, nor which users have a valid password. + */ + scram_opaq = pg_be_scram_init(port, selected_mech, shadow_pass); inputlen = pq_getmsgint(&buf, 4); if (inputlen == -1) diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c index 310e9ba348..1b659a5870 100644 --- a/src/backend/libpq/be-secure-openssl.c +++ b/src/backend/libpq/be-secure-openssl.c @@ -1105,28 +1105,10 @@ be_tls_get_peerdn_name(Port *port, char *ptr, size_t len) ptr[0] = '\0'; } -char * -be_tls_get_peer_finished(Port *port, size_t *len) -{ - char dummy[1]; - char *result; - - /* - * OpenSSL does not offer an API to directly get the length of the - * expected TLS Finished message, so just do a dummy call to grab this - * information to allow caller to do an allocation with a correct size. - */ - *len = SSL_get_peer_finished(port->ssl, dummy, sizeof(dummy)); - result = palloc(*len); - (void) SSL_get_peer_finished(port->ssl, result, *len); - - return result; -} - +#ifdef HAVE_X509_GET_SIGNATURE_NID char * be_tls_get_certificate_hash(Port *port, size_t *len) { -#ifdef HAVE_X509_GET_SIGNATURE_NID X509 *server_cert; char *cert_hash; const EVP_MD *algo_type = NULL; @@ -1176,13 +1158,8 @@ be_tls_get_certificate_hash(Port *port, size_t *len) *len = hash_size; return cert_hash; -#else - ereport(ERROR, - (errcode(ERRCODE_PROTOCOL_VIOLATION), - errmsg("channel binding type \"tls-server-end-point\" is not supported by this build"))); - return NULL; -#endif } +#endif /* * Convert an X509 subject name to a cstring. diff --git a/src/include/common/scram-common.h b/src/include/common/scram-common.h index dcb5d69078..2131303169 100644 --- a/src/include/common/scram-common.h +++ b/src/include/common/scram-common.h @@ -19,10 +19,6 @@ #define SCRAM_SHA_256_NAME "SCRAM-SHA-256" #define SCRAM_SHA_256_PLUS_NAME "SCRAM-SHA-256-PLUS" /* with channel binding */ -/* Channel binding types */ -#define SCRAM_CHANNEL_BINDING_TLS_UNIQUE "tls-unique" -#define SCRAM_CHANNEL_BINDING_TLS_END_POINT "tls-server-end-point" - /* Length of SCRAM keys (client and server) */ #define SCRAM_KEY_LEN PG_SHA256_DIGEST_LENGTH diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h index 7698cd1f88..ef5528c897 100644 --- a/src/include/libpq/libpq-be.h +++ b/src/include/libpq/libpq-be.h @@ -260,24 +260,23 @@ extern const char *be_tls_get_version(Port *port); extern const char *be_tls_get_cipher(Port *port); extern void be_tls_get_peerdn_name(Port *port, char *ptr, size_t len); -/* - * Get the expected TLS Finished message information from the client, useful - * for authorization when doing channel binding. - * - * Result is a palloc'd copy of the TLS Finished message with its size. - */ -extern char *be_tls_get_peer_finished(Port *port, size_t *len); - /* * Get the server certificate hash for SCRAM channel binding type * tls-server-end-point. * * The result is a palloc'd hash of the server certificate with its * size, and NULL if there is no certificate available. + * + * This is not supported with old versions of OpenSSL that don't have + * the X509_get_signature_nid() function. */ +#if defined(USE_OPENSSL) && defined(HAVE_X509_GET_SIGNATURE_NID) +#define HAVE_BE_TLS_GET_CERTIFICATE_HASH extern char *be_tls_get_certificate_hash(Port *port, size_t *len); #endif +#endif /* USE_SSL */ + extern ProtocolVersion FrontendProtocol; /* TCP keepalives configuration. These are no-ops on an AF_UNIX socket. */ diff --git a/src/include/libpq/scram.h b/src/include/libpq/scram.h index 91872fcd08..f7865ca5fc 100644 --- a/src/include/libpq/scram.h +++ b/src/include/libpq/scram.h @@ -13,6 +13,7 @@ #ifndef PG_SCRAM_H #define PG_SCRAM_H +#include "lib/stringinfo.h" #include "libpq/libpq-be.h" /* Status codes for message exchange */ @@ -21,7 +22,8 @@ #define SASL_EXCHANGE_FAILURE 2 /* Routines dedicated to authentication */ -extern void *pg_be_scram_init(Port *port, const char *shadow_pass); +extern void pg_be_scram_get_mechanisms(Port *port, StringInfo buf); +extern void *pg_be_scram_init(Port *port, const char *selected_mech, const char *shadow_pass); extern int pg_be_scram_exchange(void *opaq, char *input, int inputlen, char **output, int *outputlen, char **logdetail); diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c index 8415bbb5c6..1d9c937118 100644 --- a/src/interfaces/libpq/fe-auth-scram.c +++ b/src/interfaces/libpq/fe-auth-scram.c @@ -352,17 +352,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); - } - else if (conn->scram_channel_binding == NULL || - strlen(conn->scram_channel_binding) == 0) - { - /* - * Client has chosen to not show to server that it supports channel - * binding. - */ - appendPQExpBuffer(&buf, "n"); + appendPQExpBuffer(&buf, "p=tls-server-end-point"); } +#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH else if (conn->ssl_in_use) { /* @@ -370,6 +362,7 @@ build_client_first_message(fe_scram_state *state) */ appendPQExpBuffer(&buf, "y"); } +#endif else { /* @@ -432,60 +425,28 @@ build_client_final_message(fe_scram_state *state) */ if (strcmp(state->sasl_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0) { +#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH char *cbind_data = NULL; size_t cbind_data_len = 0; size_t cbind_header_len; char *cbind_input; size_t cbind_input_len; - if (strcmp(conn->scram_channel_binding, SCRAM_CHANNEL_BINDING_TLS_UNIQUE) == 0) + /* Fetch hash data of server's SSL certificate */ + cbind_data = + pgtls_get_peer_certificate_hash(state->conn, + &cbind_data_len); + if (cbind_data == NULL) { -#ifdef USE_SSL - cbind_data = pgtls_get_finished(state->conn, &cbind_data_len); - if (cbind_data == NULL) - goto oom_error; -#endif - } - else if (strcmp(conn->scram_channel_binding, - SCRAM_CHANNEL_BINDING_TLS_END_POINT) == 0) - { - /* Fetch hash data of server's SSL certificate */ -#ifdef USE_SSL - cbind_data = - pgtls_get_peer_certificate_hash(state->conn, - &cbind_data_len); - if (cbind_data == NULL) - { - /* error message is already set on error */ - return NULL; - } -#endif - } - else - { - /* should not happen */ + /* error message is already set on error */ termPQExpBuffer(&buf); - printfPQExpBuffer(&conn->errorMessage, - libpq_gettext("invalid channel binding type\n")); - return NULL; - } - - /* should not happen */ - if (cbind_data == NULL || cbind_data_len == 0) - { - if (cbind_data != NULL) - free(cbind_data); - termPQExpBuffer(&buf); - printfPQExpBuffer(&conn->errorMessage, - libpq_gettext("empty channel binding data for channel binding type \"%s\"\n"), - conn->scram_channel_binding); return NULL; } appendPQExpBuffer(&buf, "c="); /* p=type,, */ - cbind_header_len = 4 + strlen(conn->scram_channel_binding); + cbind_header_len = strlen("p=tls-server-end-point,,"); cbind_input_len = cbind_header_len + cbind_data_len; cbind_input = malloc(cbind_input_len); if (!cbind_input) @@ -493,8 +454,7 @@ build_client_final_message(fe_scram_state *state) free(cbind_data); goto oom_error; } - snprintf(cbind_input, cbind_input_len, "p=%s,,", - conn->scram_channel_binding); + memcpy(cbind_input, "p=tls-server-end-point,,", cbind_header_len); memcpy(cbind_input + cbind_header_len, cbind_data, cbind_data_len); if (!enlargePQExpBuffer(&buf, pg_b64_enc_len(cbind_input_len))) @@ -508,12 +468,21 @@ build_client_final_message(fe_scram_state *state) free(cbind_data); free(cbind_input); +#else + /* + * Chose channel binding, but the SSL library doesn't support it. + * Shouldn't happen. + */ + termPQExpBuffer(&buf); + printfPQExpBuffer(&conn->errorMessage, + "channel binding not supported by this build\n"); + return NULL; +#endif /* HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH */ } - else if (conn->scram_channel_binding == NULL || - strlen(conn->scram_channel_binding) == 0) - appendPQExpBuffer(&buf, "c=biws"); /* base64 of "n,," */ +#ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH else if (conn->ssl_in_use) appendPQExpBuffer(&buf, "c=eSws"); /* base64 of "y,," */ +#endif else appendPQExpBuffer(&buf, "c=biws"); /* base64 of "n,," */ diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index 09012c562d..540aba98b3 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -530,11 +530,26 @@ pg_SASL_init(PGconn *conn, int payloadlen) * nothing else has already been picked. If we add more mechanisms, a * more refined priority mechanism might become necessary. */ - if (conn->ssl_in_use && - conn->scram_channel_binding && - strlen(conn->scram_channel_binding) > 0 && - strcmp(mechanism_buf.data, SCRAM_SHA_256_PLUS_NAME) == 0) - selected_mechanism = SCRAM_SHA_256_PLUS_NAME; + if (strcmp(mechanism_buf.data, SCRAM_SHA_256_PLUS_NAME) == 0) + { + if (conn->ssl_in_use) + selected_mechanism = SCRAM_SHA_256_PLUS_NAME; + else + { + /* + * The server offered SCRAM-SHA-256-PLUS, but the connection + * is not SSL-encrypted. That's not sane. Perhaps SSL was + * stripped by a proxy? There's no point in continuing, + * because the server will reject the connection anyway if we + * try authenticate without channel binding even though both + * the client and server supported it. The SCRAM exchange + * checks for that, to prevent downgrade attacks. + */ + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("server offered SCRAM-SHA-256-PLUS authentication over a non-SSL connection\n")); + goto error; + } + } else if (strcmp(mechanism_buf.data, SCRAM_SHA_256_NAME) == 0 && !selected_mechanism) selected_mechanism = SCRAM_SHA_256_NAME; diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 4b35994394..221f1ecdaf 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -123,7 +123,6 @@ static int ldapServiceLookup(const char *purl, PQconninfoOption *options, #define DefaultOption "" #define DefaultAuthtype "" #define DefaultTargetSessionAttrs "any" -#define DefaultSCRAMChannelBinding SCRAM_CHANNEL_BINDING_TLS_UNIQUE #ifdef USE_SSL #define DefaultSSLMode "prefer" #else @@ -264,11 +263,6 @@ static const internalPQconninfoOption PQconninfoOptions[] = { "TCP-Keepalives-Count", "", 10, /* strlen(INT32_MAX) == 10 */ offsetof(struct pg_conn, keepalives_count)}, - {"scram_channel_binding", NULL, DefaultSCRAMChannelBinding, NULL, - "SCRAM-Channel-Binding", "D", - 21, /* sizeof("tls-server-end-point") == 21 */ - offsetof(struct pg_conn, scram_channel_binding)}, - /* * ssl options are allowed even without client SSL support because the * client can still handle SSL modes "disable" and "allow". Other @@ -3481,8 +3475,6 @@ freePGconn(PGconn *conn) free(conn->keepalives_interval); if (conn->keepalives_count) free(conn->keepalives_count); - if (conn->scram_channel_binding) - free(conn->scram_channel_binding); if (conn->sslmode) free(conn->sslmode); if (conn->sslcert) diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index 045405e92b..bbae8eff81 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -369,30 +369,10 @@ pgtls_write(PGconn *conn, const void *ptr, size_t len) return n; } -char * -pgtls_get_finished(PGconn *conn, size_t *len) -{ - char dummy[1]; - char *result; - - /* - * OpenSSL does not offer an API to get directly the length of the TLS - * Finished message sent, so first do a dummy call to grab this - * information and then do an allocation with the correct size. - */ - *len = SSL_get_finished(conn->ssl, dummy, sizeof(dummy)); - result = malloc(*len); - if (result == NULL) - return NULL; - (void) SSL_get_finished(conn->ssl, result, *len); - - return result; -} - +#ifdef HAVE_X509_GET_SIGNATURE_NID char * pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len) { -#ifdef HAVE_X509_GET_SIGNATURE_NID X509 *peer_cert; const EVP_MD *algo_type; unsigned char hash[EVP_MAX_MD_SIZE]; /* size for SHA-512 */ @@ -462,12 +442,8 @@ pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len) *len = hash_size; return cert_hash; -#else - printfPQExpBuffer(&conn->errorMessage, - libpq_gettext("channel binding type \"tls-server-end-point\" is not supported by this build\n")); - return NULL; -#endif } +#endif /* HAVE_X509_GET_SIGNATURE_NID */ /* ------------------------------------------------------------ */ /* OpenSSL specific code */ diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 9a586ff25a..4a836b186e 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -349,7 +349,6 @@ struct pg_conn * retransmits */ char *keepalives_count; /* maximum number of TCP keepalive * retransmits */ - char *scram_channel_binding; /* SCRAM channel binding type */ char *sslmode; /* SSL mode (require,prefer,allow,disable) */ char *sslcompression; /* SSL compression (0 or 1) */ char *sslkey; /* client key filename */ @@ -715,22 +714,20 @@ extern bool pgtls_read_pending(PGconn *conn); */ extern ssize_t pgtls_write(PGconn *conn, const void *ptr, size_t len); -/* - * Get the TLS finish message sent during last handshake. - * - * This information is useful for callers doing channel binding during - * authentication. - */ -extern char *pgtls_get_finished(PGconn *conn, size_t *len); - /* * Get the hash of the server certificate, for SCRAM channel binding type * tls-server-end-point. * * NULL is sent back to the caller in the event of an error, with an * error message for the caller to consume. + * + * This is not supported with old versions of OpenSSL that don't have + * the X509_get_signature_nid() function. */ +#if defined(USE_OPENSSL) && defined(HAVE_X509_GET_SIGNATURE_NID) +#define HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH extern char *pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len); +#endif /* * Verify that the server certificate matches the host name we connected to. diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl index 52a8f458cb..01f35265bf 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 = 1; # This is the hostname used to connect to the server. my $SERVERHOSTADDR = '127.0.0.1'; @@ -47,35 +47,6 @@ $common_connstr = # Default settings test_connect_ok($common_connstr, '', - "SCRAM authentication with default channel binding"); - -# Channel binding settings -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=''", - "SCRAM authentication without channel binding"); -if ($supports_tls_server_end_point) -{ - test_connect_ok( - $common_connstr, - "scram_channel_binding=tls-server-end-point", - "SCRAM authentication with tls-server-end-point as channel binding"); -} -else -{ - test_connect_fails( - $common_connstr, - "scram_channel_binding=tls-server-end-point", - qr/channel binding type "tls-server-end-point" is not supported by this build/, - "SCRAM authentication with tls-server-end-point as channel binding"); - $number_of_tests++; -} -test_connect_fails( - $common_connstr, - "scram_channel_binding=not-exists", - qr/unsupported SCRAM channel-binding type/, - "SCRAM authentication with invalid channel binding"); + "Basic SCRAM authentication with SSL"); done_testing($number_of_tests);