From 733ba28a13fd3baf2959061dc660dc6b7e5afd37 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Thu, 4 Mar 2021 23:10:22 +0100 Subject: [PATCH v4] Disallow SSL compression SSL compression has been disabled in OpenSSL since version 1.1.0, and was disabled in many distros long before that. The most recent TLS version, TLSv1.3, also disallows compression at the protocol level. PostgreSQL disabled compression by default years ago and recommended against using it, this finally removes the feaure while leaving the parameter to keep applications from breaking. The compression field in pg_stat_ssl is kept for a set number of releases to avoid breaking applications consuming it. On top of removing the ability to activate compression by configuration, compression is actively disabled in both frontend and backend to avoid overrides from local openssl configurations. Also add a test for deprecated SSL parameters to ensure backwards compatibility. Discussion: https://postgr.es/m/595cf3b1-4ffe-7f05-6f72-f72b7afa7993%402ndquadrant.com Discussion: https://postgr.es/m/7E384D48-11C5-441B-9EC3-F7DB1F8518F6@yesql.se --- .../postgres_fdw/expected/postgres_fdw.out | 3 +- contrib/postgres_fdw/sql/postgres_fdw.sql | 1 - doc/src/sgml/libpq.sgml | 33 ++----------------- doc/src/sgml/monitoring.sgml | 5 +-- src/backend/libpq/be-secure-openssl.c | 12 ++----- src/backend/postmaster/pgstat.c | 1 - src/backend/utils/adt/pgstatfuncs.c | 2 +- src/backend/utils/init/postinit.c | 5 ++- src/include/libpq/libpq-be.h | 1 - src/include/pgstat.h | 1 - src/interfaces/libpq/fe-connect.c | 11 ++++--- src/interfaces/libpq/fe-secure-openssl.c | 13 ++++---- src/interfaces/libpq/libpq-int.h | 1 - src/test/ssl/t/001_ssltests.pl | 9 ++++- 14 files changed, 33 insertions(+), 65 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 0649b6b81c..c2565dfc70 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -163,7 +163,6 @@ ALTER SERVER testserver1 OPTIONS ( keepalives_interval 'value', tcp_user_timeout 'value', -- requiressl 'value', - sslcompression 'value', sslmode 'value', sslcert 'value', sslkey 'value', @@ -8946,7 +8945,7 @@ DO $d$ END; $d$; ERROR: invalid option "password" -HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, sslcrldir, requirepeer, ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch_size, batch_size +HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcert, sslkey, sslrootcert, sslcrl, sslcrldir, requirepeer, ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch_size, batch_size CONTEXT: SQL statement "ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')" PL/pgSQL function inline_code_block line 3 at EXECUTE -- If we add a password for our user mapping instead, we should get a different diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 2b525ea44a..a143a70406 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -177,7 +177,6 @@ ALTER SERVER testserver1 OPTIONS ( keepalives_interval 'value', tcp_user_timeout 'value', -- requiressl 'value', - sslcompression 'value', sslmode 'value', sslcert 'value', sslkey 'value', diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 0553279314..eb9bc7084d 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -1635,24 +1635,7 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname sslcompression - If set to 1, data sent over SSL connections will be compressed. If - set to 0, compression will be disabled. The default is 0. This - parameter is ignored if a connection without SSL is made. - - - - SSL compression is nowadays considered insecure and its use is no - longer recommended. OpenSSL 1.1.0 disables - compression by default, and many operating system distributions - disable it in prior versions as well, so setting this parameter to on - will not have any effect if the server does not accept compression. - - - - If security is not a primary concern, compression can improve - throughput if the network is the bottleneck. Disabling compression - can improve response time and throughput if CPU performance is the - limiting factor. + Ignored (formerly, this specified whether to attempt SSL compression). @@ -2545,9 +2528,7 @@ const char *PQsslAttribute(const PGconn *conn, const char *attribute_name); compression - If SSL compression is in use, returns the name of the compression - algorithm, or "on" if compression is used but the algorithm is - not known. If compression is not in use, returns "off". + Compression is no longer supported, always returns "off". @@ -7182,16 +7163,6 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough) - - - - PGSSLCOMPRESSION - - PGSSLCOMPRESSION behaves the same as the connection parameter. - - - diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 3513e127b7..419a101697 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3070,8 +3070,9 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i compression boolean - True if SSL compression is in use, false if not, - or NULL if SSL is not in use on this connection + SSL compression is deprecated, this field is always false. This field + will be removed in a future version of + PostgreSQL diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c index 4c4f025eb1..8c37381add 100644 --- a/src/backend/libpq/be-secure-openssl.c +++ b/src/backend/libpq/be-secure-openssl.c @@ -245,6 +245,9 @@ be_tls_init(bool isServerStart) /* disallow SSL session caching, too */ SSL_CTX_set_session_cache_mode(context, SSL_SESS_CACHE_OFF); + /* disallow SSL compression */ + SSL_CTX_set_options(context, SSL_OP_NO_COMPRESSION); + /* set up ephemeral DH and ECDH keys */ if (!initialize_dh(context, isServerStart)) goto error; @@ -1182,15 +1185,6 @@ be_tls_get_cipher_bits(Port *port) return 0; } -bool -be_tls_get_compression(Port *port) -{ - if (port->ssl) - return (SSL_get_current_compression(port->ssl) != NULL); - else - return false; -} - const char * be_tls_get_version(Port *port) { diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index f75b52719d..9259dc9d3e 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -3215,7 +3215,6 @@ pgstat_bestart(void) { lbeentry.st_ssl = true; lsslstatus.ssl_bits = be_tls_get_cipher_bits(MyProcPort); - lsslstatus.ssl_compression = be_tls_get_compression(MyProcPort); strlcpy(lsslstatus.ssl_version, be_tls_get_version(MyProcPort), NAMEDATALEN); strlcpy(lsslstatus.ssl_cipher, be_tls_get_cipher(MyProcPort), NAMEDATALEN); be_tls_get_peer_subject_name(MyProcPort, lsslstatus.ssl_client_dn, NAMEDATALEN); diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c index 62bff52638..75f7b33410 100644 --- a/src/backend/utils/adt/pgstatfuncs.c +++ b/src/backend/utils/adt/pgstatfuncs.c @@ -875,7 +875,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS) values[19] = CStringGetTextDatum(beentry->st_sslstatus->ssl_version); values[20] = CStringGetTextDatum(beentry->st_sslstatus->ssl_cipher); values[21] = Int32GetDatum(beentry->st_sslstatus->ssl_bits); - values[22] = BoolGetDatum(beentry->st_sslstatus->ssl_compression); + values[22] = BoolGetDatum(false); /* compression deprecated */ if (beentry->st_sslstatus->ssl_client_dn[0]) values[23] = CStringGetTextDatum(beentry->st_sslstatus->ssl_client_dn); diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index e5965bc517..7abeccb536 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -264,11 +264,10 @@ PerformAuthentication(Port *port) #ifdef USE_SSL if (port->ssl_in_use) - appendStringInfo(&logmsg, _(" SSL enabled (protocol=%s, cipher=%s, bits=%d, compression=%s)"), + appendStringInfo(&logmsg, _(" SSL enabled (protocol=%s, cipher=%s, bits=%d)"), be_tls_get_version(port), be_tls_get_cipher(port), - be_tls_get_cipher_bits(port), - be_tls_get_compression(port) ? _("on") : _("off")); + be_tls_get_cipher_bits(port)); #endif #ifdef ENABLE_GSS if (port->gss) diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h index 7be1a67d69..30fb4e613d 100644 --- a/src/include/libpq/libpq-be.h +++ b/src/include/libpq/libpq-be.h @@ -266,7 +266,6 @@ extern ssize_t be_tls_write(Port *port, void *ptr, size_t len, int *waitfor); * Return information about the SSL connection. */ extern int be_tls_get_cipher_bits(Port *port); -extern bool be_tls_get_compression(Port *port); extern const char *be_tls_get_version(Port *port); extern const char *be_tls_get_cipher(Port *port); extern void be_tls_get_peer_subject_name(Port *port, char *ptr, size_t len); diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 724068cf87..e0c70d221b 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -1135,7 +1135,6 @@ typedef struct PgBackendSSLStatus { /* Information about SSL connection */ int ssl_bits; - bool ssl_compression; char ssl_version[NAMEDATALEN]; char ssl_cipher[NAMEDATALEN]; char ssl_client_dn[NAMEDATALEN]; diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index f83af03d0a..459c40bfac 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -293,9 +293,12 @@ static const internalPQconninfoOption PQconninfoOptions[] = { "SSL-Mode", "", 12, /* sizeof("verify-full") == 12 */ offsetof(struct pg_conn, sslmode)}, - {"sslcompression", "PGSSLCOMPRESSION", "0", NULL, - "SSL-Compression", "", 1, - offsetof(struct pg_conn, sslcompression)}, + /* + * "sslcompression" is no longer used, but keep it present for backwards + * compatibility. + */ + {"sslcompression", NULL, NULL, NULL, + "SSL-Compression", "D", 1, -1}, {"sslcert", "PGSSLCERT", NULL, NULL, "SSL-Client-Cert", "", 64, @@ -4080,8 +4083,6 @@ freePGconn(PGconn *conn) free(conn->sslcrl); if (conn->sslcrldir) free(conn->sslcrldir); - if (conn->sslcompression) - free(conn->sslcompression); if (conn->requirepeer) free(conn->requirepeer); if (conn->ssl_min_protocol_version) diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index 0fa10a23b4..bc58cdcd47 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -1258,12 +1258,9 @@ initialize_SSL(PGconn *conn) SSL_set_verify(conn->ssl, SSL_VERIFY_PEER, verify_cb); /* - * Set compression option if necessary. + * Disable SSL compression */ - if (conn->sslcompression && conn->sslcompression[0] == '0') - SSL_set_options(conn->ssl, SSL_OP_NO_COMPRESSION); - else - SSL_clear_options(conn->ssl, SSL_OP_NO_COMPRESSION); + SSL_set_options(conn->ssl, SSL_OP_NO_COMPRESSION); return 0; } @@ -1553,8 +1550,12 @@ PQsslAttribute(PGconn *conn, const char *attribute_name) if (strcmp(attribute_name, "cipher") == 0) return SSL_get_cipher(conn->ssl); + /* + * SSL compression is disabled, so even if connecting to an older server + * which still supports it, it wont be active. + */ if (strcmp(attribute_name, "compression") == 0) - return SSL_get_current_compression(conn->ssl) ? "on" : "off"; + return "off"; if (strcmp(attribute_name, "protocol") == 0) return SSL_get_version(conn->ssl); diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 8d51e6ed9f..cca98c14bf 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -360,7 +360,6 @@ struct pg_conn char *keepalives_count; /* maximum number of TCP keepalive * retransmits */ char *sslmode; /* SSL mode (require,prefer,allow,disable) */ - char *sslcompression; /* SSL compression (0 or 1) */ char *sslkey; /* client key filename */ char *sslcert; /* client certificate filename */ char *sslpassword; /* client key file password */ diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl index 864f6e209f..f15b3bf9db 100644 --- a/src/test/ssl/t/001_ssltests.pl +++ b/src/test/ssl/t/001_ssltests.pl @@ -17,7 +17,7 @@ if ($ENV{with_ssl} ne 'openssl') } else { - plan tests => 100; + plan tests => 101; } #### Some configuration @@ -157,6 +157,13 @@ test_connect_fails( qr/root certificate file "invalid" does not exist/, "connect without server root cert sslmode=verify-full"); +# Test deprecated SSL parameters which should be accepted for backwards +# compatibility +test_connect_ok( + $common_connstr, + "sslrootcert=invalid sslmode=require sslcompression=1 requiressl=1", + "connect with deprecated connection parameters"); + # Try with wrong root cert, should fail. (We're using the client CA as the # root, but the server's key is signed by the server CA.) test_connect_fails($common_connstr, -- 2.21.1 (Apple Git-122.3)