Add bound checks for ssl_min_protocol_version and ssl_max_protocol_version

Mixing incorrect bounds in the SSL context leads to confusing error
messages generated by OpenSSL which are hard to act on.  New range
checks are added when both min/max parameters are loaded in the context
of a SSL reload to improve the error reporting.  Note that this does not
make use of the GUC hook machinery contrary to 41aadee, as there is no
way to ensure a consistent range check (except if there is a way one day
to define range types for GUC parameters?).  Hence, this patch applies
only to OpenSSL, and uses a logic similar to other parameters to trigger
an error when reloading the SSL context in a session.

Author: Michael Paquier
Reviewed-by: Daniel Gustafsson
Discussion: https://postgr.es/m/20200114035420.GE1515@paquier.xyz

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 18d3fff068be1258317e01837e0187a486ea1af2..b53c2b813ebb8687c20dba02400c19bc4ef0ed1f 100644 (file)
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -68,8 +68,7 @@ static bool SSL_initialized = false;
 static bool dummy_ssl_passwd_cb_called = false;
 static bool ssl_is_server_start;
 
-static int     ssl_protocol_version_to_openssl(int v, const char *guc_name,
-                                                                                       int loglevel);
+static int     ssl_protocol_version_to_openssl(int v);
 
 /* ------------------------------------------------------------ */
 /*                                              Public interface                                               */
@@ -80,6 +79,8 @@ be_tls_init(bool isServerStart)
 {
        STACK_OF(X509_NAME) *root_cert_list = NULL;
        SSL_CTX    *context;
+       int                     ssl_ver_min = -1;
+       int                     ssl_ver_max = -1;
 
        /* This stuff need be done only once. */
        if (!SSL_initialized)
@@ -188,13 +189,19 @@ be_tls_init(bool isServerStart)
 
        if (ssl_min_protocol_version)
        {
-               int                     ssl_ver = ssl_protocol_version_to_openssl(ssl_min_protocol_version,
-                                                                                                                         "ssl_min_protocol_version",
-                                                                                                                         isServerStart ? FATAL : LOG);
+               ssl_ver_min = ssl_protocol_version_to_openssl(ssl_min_protocol_version);
 
-               if (ssl_ver == -1)
+               if (ssl_ver_min == -1)
+               {
+                       ereport(isServerStart ? FATAL : LOG,
+                                       (errmsg("\"%s\" setting \"%s\" not supported by this build",
+                                                       "ssl_min_protocol_version",
+                                                       GetConfigOption("ssl_min_protocol_version",
+                                                                                       false, false))));
                        goto error;
-               if (!SSL_CTX_set_min_proto_version(context, ssl_ver))
+               }
+
+               if (!SSL_CTX_set_min_proto_version(context, ssl_ver_min))
                {
                        ereport(isServerStart ? FATAL : LOG,
                                        (errmsg("could not set minimum SSL protocol version")));
@@ -204,13 +211,19 @@ be_tls_init(bool isServerStart)
 
        if (ssl_max_protocol_version)
        {
-               int                     ssl_ver = ssl_protocol_version_to_openssl(ssl_max_protocol_version,
-                                                                                                                         "ssl_max_protocol_version",
-                                                                                                                         isServerStart ? FATAL : LOG);
+               ssl_ver_max = ssl_protocol_version_to_openssl(ssl_max_protocol_version);
 
-               if (ssl_ver == -1)
+               if (ssl_ver_max == -1)
+               {
+                       ereport(isServerStart ? FATAL : LOG,
+                                       (errmsg("\"%s\" setting \"%s\" not supported by this build",
+                                                       "ssl_max_protocol_version",
+                                                       GetConfigOption("ssl_max_protocol_version",
+                                                                                       false, false))));
                        goto error;
-               if (!SSL_CTX_set_max_proto_version(context, ssl_ver))
+               }
+
+               if (!SSL_CTX_set_max_proto_version(context, ssl_ver_max))
                {
                        ereport(isServerStart ? FATAL : LOG,
                                        (errmsg("could not set maximum SSL protocol version")));
@@ -218,6 +231,23 @@ be_tls_init(bool isServerStart)
                }
        }
 
+       /* Check compatibility of min/max protocols */
+       if (ssl_min_protocol_version &&
+               ssl_max_protocol_version)
+       {
+               /*
+                * No need to check for invalid values (-1) for each protocol number
+                * as the code above would have already generated an error.
+                */
+               if (ssl_ver_min > ssl_ver_max)
+                       ereport(isServerStart ? FATAL : LOG,
+                                       (errmsg("could not set SSL protocol version range"),
+                                        errdetail("\"%s\" cannot be higher than \"%s\"",
+                                                          "ssl_min_protocol_version",
+                                                          "ssl_max_protocol_version")));
+               goto error;
+       }
+
        /* disallow SSL session tickets */
        SSL_CTX_set_options(context, SSL_OP_NO_TICKET);
 
@@ -1271,15 +1301,14 @@ X509_NAME_to_cstring(X509_NAME *name)
  * guc.c independent of OpenSSL availability and version.
  *
  * If a version is passed that is not supported by the current OpenSSL
- * version, then we log with the given loglevel and return (if we return) -1.
- * If a nonnegative value is returned, subsequent code can assume it's working
- * with a supported version.
+ * version, then we return -1.  If a nonnegative value is returned,
+ * subsequent code can assume it's working with a supported version.
  *
  * Note: this is rather similar to libpq's routine in fe-secure-openssl.c,
  * so make sure to update both routines if changing this one.
  */
 static int
-ssl_protocol_version_to_openssl(int v, const char *guc_name, int loglevel)
+ssl_protocol_version_to_openssl(int v)
 {
        switch (v)
        {
@@ -1307,9 +1336,5 @@ ssl_protocol_version_to_openssl(int v, const char *guc_name, int loglevel)
 #endif
        }
 
-       ereport(loglevel,
-                       (errmsg("%s setting %s not supported by this build",
-                                       guc_name,
-                                       GetConfigOption(guc_name, false, false))));
        return -1;
 }

diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index e740099aca82dc5ae6c4b6322621108c98089304..d035ac7fc9762cd22f83c2d214981d04f4633708 100644 (file)
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -13,7 +13,7 @@ use SSLServer;
 
 if ($ENV{with_openssl} eq 'yes')
 {
-       plan tests => 91;
+       plan tests => 93;
 }
 else
 {
@@ -97,6 +97,22 @@ command_ok(
        'restart succeeds with password-protected key file');
 $node->_update_pid(1);
 
+# Test compatibility of SSL protocols.
+# TLSv1.1 is lower than TLSv1.2, so it won't work.
+$node->append_conf('postgresql.conf',
+                  qq{ssl_min_protocol_version='TLSv1.2'
+ssl_max_protocol_version='TLSv1.1'});
+command_fails(
+       [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ],
+       'restart fails with incorrect SSL protocol bounds');
+# Go back to the defaults, this works.
+$node->append_conf('postgresql.conf',
+                  qq{ssl_min_protocol_version='TLSv1.2'
+ssl_max_protocol_version=''});
+command_ok(
+       [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ],
+       'restart succeeds with correct SSL protocol bounds');
+
 ### Run client-side tests.
 ###
 ### Test that libpq accepts/rejects the connection correctly, depending