From 981a55f11d68e1ff4c461e8e1d6184c064852cd6 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Wed, 17 Feb 2021 11:15:40 +0100 Subject: [PATCH v3] 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. 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 | 4 +-- contrib/postgres_fdw/sql/postgres_fdw.sql | 2 +- doc/src/sgml/libpq.sgml | 33 ++----------------- src/backend/libpq/be-secure-openssl.c | 3 ++ src/interfaces/libpq/fe-connect.c | 6 +++- src/interfaces/libpq/fe-secure-openssl.c | 13 ++++---- src/interfaces/libpq/libpq-int.h | 2 +- src/test/ssl/t/001_ssltests.pl | 9 ++++- 8 files changed, 29 insertions(+), 43 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 0649b6b81c..01420d93b6 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -163,7 +163,7 @@ ALTER SERVER testserver1 OPTIONS ( keepalives_interval 'value', tcp_user_timeout 'value', -- requiressl 'value', - sslcompression 'value', + -- sslcompression 'value', sslmode 'value', sslcert 'value', sslkey 'value', @@ -8946,7 +8946,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..f343162cef 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -177,7 +177,7 @@ ALTER SERVER testserver1 OPTIONS ( keepalives_interval 'value', tcp_user_timeout 'value', -- requiressl 'value', - sslcompression 'value', + -- sslcompression 'value', sslmode 'value', sslcert 'value', sslkey 'value', diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index cc20b0f234..124c53e7b1 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). @@ -2562,9 +2545,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". @@ -7229,16 +7210,6 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough) - - - - PGSSLCOMPRESSION - - PGSSLCOMPRESSION behaves the same as the connection parameter. - - - diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c index 4c4f025eb1..b4800bb147 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; diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 9812a14662..3ff6d4a9ea 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -293,8 +293,12 @@ static const internalPQconninfoOption PQconninfoOptions[] = { "SSL-Mode", "", 12, /* sizeof("verify-full") == 12 */ offsetof(struct pg_conn, sslmode)}, + /* + * "sslcompression" is no longer used, but keep it present for backwards + * compatibility. + */ {"sslcompression", "PGSSLCOMPRESSION", "0", NULL, - "SSL-Compression", "", 1, + "SSL-Compression", "D", 1, offsetof(struct pg_conn, sslcompression)}, {"sslcert", "PGSSLCERT", NULL, NULL, 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 0c9e95f1a7..d15a447324 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -376,7 +376,7 @@ 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 *sslcompression; /* SSL compression (OBSOLETE, NOT USED) */ 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)