On Wed, Jun 06, 2018 at 11:46:11PM +0300, Heikki Linnakangas wrote: > Ok. Perhaps add a comment pointing out that as the code stands, > get_auth_request_str() is never called with AUTH_REQ_OK. So that if someone > starts calling it with that, maybe they'll know to revisit this.
That makes sense. Done. >> + <varlistentry> >> + <term><literal>prefer</literal> (default)</term> >> + <listitem> >> + <para> >> + Use channel binding if available. If the cluster does not >> + support it, then it is not used. This is the default. >> + </para> >> + </listitem> >> + </varlistentry> > > I'd suggest s/cluster/server/. Here, and elsewhere in the docs changes. Done. > There are some funny behaviors with different combinations of > "scram_channel_binding_mode=require" and different sslmode settings. For > example, with sslmode=disable, the client will attempt to connect, but it's > guaranteed to fail at authentication, because channel binding is only > possible with SSL. Perhaps it should throw an error without even attempting > to connect? Or at least, the "server does not support channel binding" is > misleading, as it was the client that insisted on an impossible combination; > the server might well support channel binding, if SSL was used. > > And with sslmode=allow, if the server allows a non-SSL connection, then the > client won't use SSL, and will fail, as with sslmode=disable. But if the > server requires SSL, then it will work. Perhaps sslmode=allow should be > "promoted" to "sslmode=require", if scram_channel_binding_mode=require? Or > don't let the user select a silly combination like that at all, and throw an > error. I was not really planning to make the code more complicated with cross-option checks (that's complicated enough), but isn't what you are looking for here to make sslmode=require a mandatory thing if channel_binding_mode=require is set? It is possible to add checks like that in connectOptions2 for example after setting all the values. Promoting automatically sslmode="allow" to "require" if scram_channel_binding_mode=require feels like a trap for users as well. > If scram_channel_binding_mode=require, but the server doesn't support SSL at > all, you get: > > psql: server does not support channel binding, and > scram_channel_binding_mode=require was used > > Perhaps it would be more clear to say "server does not support SSL" or > something like that. I could imagine an admin spending a long time looking > for an "enable channel binding" option in server settings, not realizing > that it's actually "ssl=off" that's the problem. It really depends on the connection context as a v10 server allows SSL without channel binding, so just saying that server does not support SSL is not correct either. It seems to me that if you want an operator to take correct actions then we want to see if the backend connected to is a v10 server or something newer, or actually uses SSL in the connection context. If that's a v10 or older, then libpq should still complain about channel binding not supported. If it is a v11 or newer, then libpq should complain about SSL not being supported. By adding the cross-option check in connectOptions2, the error message about SSL not being supported is moot if requiring sslmode=require for channel_binding_mode=require. I have added a more complicated logic in pg_SASL_init() for that (grep for "server does not support channel binding") as it looks safer to me to have that for future sanity checks, so please let me know your thoughts. And it could be perfectly possible that an attacking server tries to make the client think that it is a v11 server but tries to downgrade to SCRAM without channel binding. Still, the tests reach their limit here, as you would normally bump into HBA entries which don't match normally, still it is easy to add a test which checks that sslmode!=require fails with scram_channel_binding=require. > As the patch stands, if the server is configured for "trust" authentication, > and the client uses "scram_channel_binding_mode=require", you get this > error: > > psql: channel binding required for authentication but SASL exchange is not > finished > > "SASL exchange is not finished" is quite technical. Can we have something > like "... but server was configured for \"trust\" authentication"? OK, here is a suggestion then as in the attached: "channel binding required, but the server requested authentication \"trust\"" > So, in general, would be good to go through the different combinations of > scram_channel_binding_mode, sslmode, server configured for SSL or not, > server configured for different authentication methods, one more time. To > make sure the error message in each case makes sense, and points to what the > admin needs to change to make it work. In the updated patch attached, you get the following failures with all those configurations (making sslmode=require a requirement with channel_binding_mode=require reduces the set of combinations a lot): 1) Server supporting SSL. 1-1) v10 server with SCRAM, failure: $ psql "sslmode=require scram_channel_binding_mode=require host=localhost" psql: server does not support channel binding, and scram_channel_binding_mode=require was used 1-2) v10 server with md5, password or trust, failures: $ psql "sslmode=require scram_channel_binding_mode=require host=localhost" psql: SSL enabled and channel binding required, but the server requested authentication "trust $ psql "sslmode=require scram_channel_binding_mode=require host=localhost" psql: SSL enabled and channel binding required, but the server requested authentication "md5" 1-3) v11 server with SCRAM, works up to password processing... Of course. 1-4) v11 server with md5, password or trust: $ psql "sslmode=require scram_channel_binding_mode=require host=localhost" psql: SSL enabled and channel binding required, but the server requested authentication "trust" $ psql "sslmode=require scram_channel_binding_mode=require host=localhost" psql: SSL enabled and channel binding required, but the server requested authentication "md5" So I have added an extra hint to tell that SSL is enabled and added an assertion as well as this is enforced by sslmode=require. 2) Server not supporting SSL, with failures for all cases as sslmode overrides any settings of scram_channel_binding_mode=require 2-1) v10 server with SCRAM. 2-2) v10 server with md5, password or trust, failures. 2-3) v11 server with SCRAM: failure. 2-4) v11 server with md5, password or trust: failures. $ psql "sslmode=require scram_channel_binding_mode=require host=localhost" psql: server does not support SSL, but SSL was required Thanks, -- Michael
From 1d479c950ce54d3414d528fc65b8f14dd66508a9 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> 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 cluster 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 by splitting it into two pieces: - scram_channel_binding_mode, which can use as values "require", "disable" or "prefer". - scram_channel_binding_type, which is similar to the previous scram_channel_binding, except that it does not accept anymore an empty value to mean that channel binding is disabled. "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 for all exchanges. "require" makes channel binding a hard requirement for the authentication. For "require", if the cluster does not support channel binding with SCRAM (v10 server), if another authentication that SCRAM is used 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 targeted cluster downgrades to "trust" or similar. In order to achieve that, filter authentication request messages before processing them if channel binding is required, authorizing only SASL-related messages. 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. Author: Michael Paquier Reviewed-by: Heikki Linnakangas, Robert Haas, Bruce Momjian, Magnus Hagander Discussion: https://postgr.es/m/20180519123557.gb2...@paquier.xyz --- doc/src/sgml/libpq.sgml | 77 ++++++++++++-- doc/src/sgml/release-11.sgml | 2 +- src/interfaces/libpq/fe-auth-scram.c | 43 ++++++-- src/interfaces/libpq/fe-auth.c | 153 +++++++++++++++++++++++++-- src/interfaces/libpq/fe-auth.h | 1 + src/interfaces/libpq/fe-connect.c | 79 ++++++++++++-- src/interfaces/libpq/libpq-int.h | 3 +- src/test/ssl/ServerSetup.pm | 24 +++-- src/test/ssl/t/002_scram.pl | 64 +++++++++-- 9 files changed, 395 insertions(+), 51 deletions(-) diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 498b8df988..46ea3f945d 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -1238,24 +1238,81 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname </listitem> </varlistentry> - <varlistentry id="libpq-scram-channel-binding" xreflabel="scram_channel_binding"> - <term><literal>scram_channel_binding</literal></term> + <varlistentry id="libpq-scram-channel-binding-mode" xreflabel="scram_channel_binding_mode"> + <term><literal>scram_channel_binding_mode</literal></term> + <listitem> + <para> + This option determines whether channel binding should be used + with <acronym>SCRAM</acronym> authentication. While + <acronym>SCRAM</acronym> alone prevents the replay of transmitted + hashed passwords, channel binding also prevents man-in-the-middle + attacks. There are three modes: + + <variablelist> + <varlistentry> + <term><literal>disable</literal></term> + <listitem> + <para> + Disable the use of channel binding for all connection attempts. + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><literal>prefer</literal> (default)</term> + <listitem> + <para> + Use channel binding if available. If the server does not + support it, then it is not used. This is the default. + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><literal>require</literal></term> + <listitem> + <para> + Require the presence of channel binding for authentication. + If the server does not support channel binding or if another + authentication protocol other than <acronym>SCRAM</acronym> is + used, then fail the connection. + </para> + </listitem> + </varlistentry> + </variablelist> + </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 <literal>require</literal> is used, in which case the connection + to the server is refused even if SSL is not enabled, if the server + is attempting an authentication requests different than + <acronym>SCRAM</acronym>, or if the server does not support + <acronym>SCRAM</acronym> channel binding in order to prevent + authentication downgrade attacks. Using <literal>sslmode</literal> + with <literal>require</literal> is mandatory for security reasons if + <literal>scram_channel_binding_mode</literal> is set to + <literal>require</literal>. + </para> + </listitem> + </varlistentry> + + <varlistentry id="libpq-scram-channel-binding-type" xreflabel="scram_channel_binding_type"> + <term><literal>scram_channel_binding_type</literal></term> <listitem> <para> Specifies the channel binding type to use with SCRAM - authentication. While <acronym>SCRAM</acronym> alone prevents - the replay of transmitted hashed passwords, channel binding also - prevents man-in-the-middle attacks. + authentication. </para> <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 + listed in <xref linkend="sasl-authentication"/>. 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. + 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. </para> <para> diff --git a/doc/src/sgml/release-11.sgml b/doc/src/sgml/release-11.sgml index 7599c6f7a7..15f21fd91a 100644 --- a/doc/src/sgml/release-11.sgml +++ b/doc/src/sgml/release-11.sgml @@ -1246,7 +1246,7 @@ same commits as above replay of transmitted hashed passwords in a later session, <acronym>SCRAM</acronym> with channel binding also prevents man-in-the-middle attacks. The options are <link - linkend="libpq-scram-channel-binding"><option>scram_channel_binding=tls-unique</option></link> + linkend="libpq-scram-channel-binding-type"><option>scram_channel_binding=tls-unique</option></link> and <option>scram_channel_binding=tls-server-end-point</option>. </para> </listitem> diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c index 8415bbb5c6..7b650f6e32 100644 --- a/src/interfaces/libpq/fe-auth-scram.c +++ b/src/interfaces/libpq/fe-auth-scram.c @@ -352,10 +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); + appendPQExpBuffer(&buf, "p=%s", conn->scram_channel_binding_type); } - else if (conn->scram_channel_binding == NULL || - strlen(conn->scram_channel_binding) == 0) + else if (strcmp(conn->scram_channel_binding_mode, "disable") == 0) { /* * Client has chosen to not show to server that it supports channel @@ -438,7 +437,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(conn->scram_channel_binding_type, + SCRAM_CHANNEL_BINDING_TLS_UNIQUE) == 0) { #ifdef USE_SSL cbind_data = pgtls_get_finished(state->conn, &cbind_data_len); @@ -446,7 +446,7 @@ build_client_final_message(fe_scram_state *state) goto oom_error; #endif } - else if (strcmp(conn->scram_channel_binding, + else if (strcmp(conn->scram_channel_binding_type, SCRAM_CHANNEL_BINDING_TLS_END_POINT) == 0) { /* Fetch hash data of server's SSL certificate */ @@ -478,14 +478,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); + conn->scram_channel_binding_type); return NULL; } appendPQExpBuffer(&buf, "c="); /* p=type,, */ - cbind_header_len = 4 + strlen(conn->scram_channel_binding); + cbind_header_len = 4 + strlen(conn->scram_channel_binding_type); cbind_input_len = cbind_header_len + cbind_data_len; cbind_input = malloc(cbind_input_len); if (!cbind_input) @@ -494,7 +494,7 @@ build_client_final_message(fe_scram_state *state) goto oom_error; } snprintf(cbind_input, cbind_input_len, "p=%s,,", - conn->scram_channel_binding); + conn->scram_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 +509,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_mode, "disable") == 0) appendPQExpBuffer(&buf, "c=biws"); /* base64 of "n,," */ else if (conn->ssl_in_use) appendPQExpBuffer(&buf, "c=eSws"); /* base64 of "y,," */ @@ -811,6 +810,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..fd66db464d 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -525,14 +525,15 @@ pg_SASL_init(PGconn *conn, int payloadlen) break; /* - * Select the mechanism to use. Pick SCRAM-SHA-256-PLUS over anything - * else if a channel binding type is set. Pick SCRAM-SHA-256 if - * nothing else has already been picked. If we add more mechanisms, a - * more refined priority mechanism might become necessary. + * Select the mechanism to use. SCRAM-SHA-256-PLUS is the first + * choice when the following conditions are met: the channel binding + * mode is not "disable", SSL is used and server has advertised this + * SASL mechanism. Pick SCRAM-SHA-256 as a second choice if 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(conn->scram_channel_binding_mode, "disable") != 0 && strcmp(mechanism_buf.data, SCRAM_SHA_256_PLUS_NAME) == 0) selected_mechanism = SCRAM_SHA_256_PLUS_NAME; else if (strcmp(mechanism_buf.data, SCRAM_SHA_256_NAME) == 0 && @@ -547,6 +548,35 @@ pg_SASL_init(PGconn *conn, int payloadlen) goto error; } + /* + * If client requires 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. + */ + if (strcmp(conn->scram_channel_binding_mode, "require") == 0 && + strcmp(selected_mechanism, SCRAM_SHA_256_PLUS_NAME) != 0) + { + /* + * Depending on if SSL is used or not, complain differently. A + * PostgreSQL 10 server does not support channel binding so the second + * error message can be reached in this context. The first error + * message should not actually happen, but it looks safer to keep it + * even if scram_channel_binding_mode=require implies sslmode=require + * to cover any kind of future changes in libpq. + */ + if (!conn->ssl_in_use) + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("server does not support SSL, and scram_channel_binding_mode=require was used\n")); + else + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("server does not support channel binding, and scram_channel_binding_mode=require was used\n")); + goto error; + } + /* * Now that the SASL mechanism has been chosen for the exchange, * initialize its state information. @@ -821,6 +851,62 @@ pg_password_sendauth(PGconn *conn, const char *password, AuthRequest areq) return ret; } +/* + * Helper function able to translate authentication request codes to + * to the related authentication types, useful for error messages. + * This routine is never called yet when the authentication request + * is AUTH_REQ_OK which has no value assigned. If this were to + * happen, then this routine's logic ought to be revisited. + */ +static const char * +get_auth_request_str(AuthRequest areq) +{ + const char *areq_name = "???"; + + switch (areq) + { + case AUTH_REQ_KRB4: /* not supported */ + case AUTH_REQ_KRB5: /* not supported */ + areq_name = "kerberos"; + break; + case AUTH_REQ_PASSWORD: + areq_name = "password"; + break; + case AUTH_REQ_CRYPT: /* not supported */ + areq_name = "crypt"; + break; + case AUTH_REQ_MD5: + areq_name = "md5"; + break; + case AUTH_REQ_SCM_CREDS: + areq_name = "scm"; /* removed as of 9.1 */ + break; + case AUTH_REQ_GSS: + case AUTH_REQ_GSS_CONT: + areq_name = "gssapi"; + break; + case AUTH_REQ_SSPI: + areq_name = "sspi"; + break; + case AUTH_REQ_SASL: + case AUTH_REQ_SASL_CONT: + case AUTH_REQ_SASL_FIN: + areq_name = "sasl"; + break; + + /* + * This can map directly to "trust", or anything else as final + * message, so there is no way to know what this is precisely. + */ + case AUTH_REQ_OK: + /* fall through */ + default: + break; + } + + return areq_name; +} + /* * pg_fe_sendauth * client demux routine for processing an authentication request @@ -835,6 +921,61 @@ pg_password_sendauth(PGconn *conn, const char *password, AuthRequest areq) int pg_fe_sendauth(AuthRequest areq, int payloadlen, PGconn *conn) { + /* + * If SCRAM with channel binding is required, refuse all other + * authentication methods. Requiring channel binding implies that the + * client doesn't trust the server, until the SCRAM authentication is + * completed, so it's important that we don't divulge the plaintext + * password, or perform some weaker authentication, even if the server + * asks for it. In all other authentication methods, it is currently + * assumed that the client trusts the server, either implicitly or because + * the SSL certificate was already verified during the SSL handshake. + */ + if (strcmp(conn->scram_channel_binding_mode, "require") == 0) + { + /* + * SSL has to be used if we reached this code path as sslmode=require. + */ + Assert(conn->ssl_in_use); + switch (areq) + { + case AUTH_REQ_OK: + + /* + * 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. It could be possible + * that a rogue server is attempting to send this message + * before AUTH_REQ_SASL_FIN is processed and after a set of + * AUTH_REQ_SASL and AUTH_REQ_SASL_CONT messages are exchanged + * but still complain about a server trying to enforce with + * "trust", as this case does not seem worth complicating the + * code. + */ + if (!pg_fe_scram_exchange_finished(conn)) + { + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("SSL enabled and channel binding required, but the server requested authentication \"trust\"\n")); + return STATUS_ERROR; + } + break; + + /* Those are used for the SASL exchange */ + case AUTH_REQ_SASL: + case AUTH_REQ_SASL_CONT: + case AUTH_REQ_SASL_FIN: + break; + + default: + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("SSL enabled and channel binding required, but the server requested authentication \"%s\"\n"), + get_auth_request_str(areq)); + return STATUS_ERROR; + } + } + + /* Now process the request */ switch (areq) { case AUTH_REQ_OK: diff --git a/src/interfaces/libpq/fe-auth.h b/src/interfaces/libpq/fe-auth.h index a8a27c24a6..bf41bb4343 100644 --- a/src/interfaces/libpq/fe-auth.h +++ b/src/interfaces/libpq/fe-auth.h @@ -31,5 +31,6 @@ 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..fbd0d9050e 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -123,7 +123,8 @@ static int ldapServiceLookup(const char *purl, PQconninfoOption *options, #define DefaultOption "" #define DefaultAuthtype "" #define DefaultTargetSessionAttrs "any" -#define DefaultSCRAMChannelBinding SCRAM_CHANNEL_BINDING_TLS_UNIQUE +#define DefaultSCRAMChannelBindingMode "prefer" +#define DefaultSCRAMChannelBindingType SCRAM_CHANNEL_BINDING_TLS_UNIQUE #ifdef USE_SSL #define DefaultSSLMode "prefer" #else @@ -264,10 +265,15 @@ 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", + {"scram_channel_binding_mode", NULL, DefaultSCRAMChannelBindingMode, NULL, + "SCRAM-Channel-Binding-Mode", "D", + 8, /* sizeof("require") == 8 */ + offsetof(struct pg_conn, scram_channel_binding_mode)}, + + {"scram_channel_binding_type", NULL, DefaultSCRAMChannelBindingType, NULL, + "SCRAM-Channel-Binding-Type", "D", 21, /* sizeof("tls-server-end-point") == 21 */ - offsetof(struct pg_conn, scram_channel_binding)}, + offsetof(struct pg_conn, scram_channel_binding_type)}, /* * ssl options are allowed even without client SSL support because the @@ -1166,6 +1172,65 @@ connectOptions2(PGconn *conn) goto oom_error; } + /* validate scram_channel_binding_mode option */ + if (conn->scram_channel_binding_mode) + { + if (strcmp(conn->scram_channel_binding_mode, "prefer") != 0 && + strcmp(conn->scram_channel_binding_mode, "disable") != 0 && + strcmp(conn->scram_channel_binding_mode, "require") != 0) + { + conn->status = CONNECTION_BAD; + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("invalid scram_channel_binding_mode value: \"%s\"\n"), + conn->scram_channel_binding_mode); + return false; + } + } + else + { + conn->scram_channel_binding_mode = + strdup(DefaultSCRAMChannelBindingMode); + if (!conn->scram_channel_binding_mode) + goto oom_error; + } + + /* validate scram_channel_binding_type option */ + if (conn->scram_channel_binding_type) + { + if (strcmp(conn->scram_channel_binding_type, + SCRAM_CHANNEL_BINDING_TLS_UNIQUE) != 0 && + strcmp(conn->scram_channel_binding_type, + SCRAM_CHANNEL_BINDING_TLS_END_POINT) != 0) + { + conn->status = CONNECTION_BAD; + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("invalid scram_channel_binding_type value: \"%s\"\n"), + conn->scram_channel_binding_type); + return false; + } + } + else + { + conn->scram_channel_binding_type = + strdup(DefaultSCRAMChannelBindingType); + if (!conn->scram_channel_binding_type) + goto oom_error; + } + + /* + * Validate combinations of multiple options. scram_channel_binding_mode + * set to "require" must be used with sslmode=require to ensure a secure + * connection setup. + */ + if (strcmp(conn->scram_channel_binding_mode, "require") == 0 && + strcmp(conn->sslmode, "require") != 0) + { + conn->status = CONNECTION_BAD; + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("sslmode=require mandatory with scram_channel_binding_mode=require\n")); + return false; + } + /* * Resolve special "auto" client_encoding from the locale */ @@ -3476,8 +3541,10 @@ 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->scram_channel_binding_mode) + free(conn->scram_channel_binding_mode); + if (conn->scram_channel_binding_type) + free(conn->scram_channel_binding_type); if (conn->sslmode) free(conn->sslmode); if (conn->sslcert) diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 9a586ff25a..ddcd442ad0 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -349,7 +349,8 @@ struct pg_conn * retransmits */ char *keepalives_count; /* maximum number of TCP keepalive * retransmits */ - char *scram_channel_binding; /* SCRAM channel binding type */ + char *scram_channel_binding_mode; /* SCRAM channel binding mode */ + char *scram_channel_binding_type; /* type of SCRAM channel binding */ char *sslmode; /* SSL mode (require,prefer,allow,disable) */ char *sslcompression; /* SSL compression (0 or 1) */ char *sslkey; /* client key filename */ diff --git a/src/test/ssl/ServerSetup.pm b/src/test/ssl/ServerSetup.pm index 1cd3badaa1..89311a5eec 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 @@ -132,7 +133,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); return; } @@ -160,7 +161,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 @@ -171,15 +172,20 @@ 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; return; } -1; \ No newline at end of file +1; diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl index 52a8f458cb..83f980a500 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 = 17; # This is the hostname used to connect to the server. my $SERVERHOSTADDR = '127.0.0.1'; @@ -52,30 +52,78 @@ test_connect_ok($common_connstr, '', # Channel binding settings test_connect_ok( $common_connstr, - "scram_channel_binding=tls-unique", + "scram_channel_binding_mode='require' scram_channel_binding_type=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_mode='disable'", "SCRAM authentication without channel binding"); if ($supports_tls_server_end_point) { test_connect_ok( $common_connstr, - "scram_channel_binding=tls-server-end-point", + "scram_channel_binding_mode='require' scram_channel_binding_type=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", + "scram_channel_binding_mode='require' scram_channel_binding_type=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"); + "scram_channel_binding_mode=not-exists", + qr/invalid scram_channel_binding_mode value/, + "SCRAM authentication with invalid channel binding mode"); +test_connect_fails( + $common_connstr, + "scram_channel_binding_type=not-exists", + qr/invalid scram_channel_binding_type value/, + "SCRAM authentication with invalid channel binding type"); + +# 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_mode=require", + qr/SSL enabled and channel binding required, but the server requested authentication "trust"/, + "Connection to \"trust\" server refused with mode require"); + +# 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_mode=require", + "SCRAM authentication with channel binding required"); + +# Now test "password" level, which should fail as client requires +# channel binding. +configure_hba_for_ssl($node, $SERVERHOSTADDR, "hostssl", "password"); +$node->reload; +test_connect_fails( + $common_connstr, + "scram_channel_binding_mode=require", + qr/SSL enabled and channel binding required, but the server requested authentication "password"/, + "Connection to \"password\" server refused with mode require"); + +# scram_channel_binding_mode=require is only compatible with sslmode=require. +configure_hba_for_ssl($node, $SERVERHOSTADDR, "hostnossl", "scram-sha-256"); +$node->reload; +$common_connstr = + "user=ssltestuser dbname=trustdb hostaddr=$SERVERHOSTADDR"; +test_connect_fails( + $common_connstr, + "scram_channel_binding_mode=require sslmode=disable", + qr/sslmode=require mandatory with scram_channel_binding_mode=require/, + "sslmode not set to \"require\" with scram_channel_binding_mode=require"); +test_connect_fails( + $common_connstr, + "scram_channel_binding_mode=require sslmode=require", + qr/no pg_hba.conf entry for host/, + "sslmode set to \"require\" with scram_channel_binding_mode=require"); done_testing($number_of_tests); -- 2.17.1
signature.asc
Description: PGP signature
