Avoid corner-case memory leak in SSL parameter processing.

After reading the root cert list from the ssl_ca_file, immediately
install it as client CA list of the new SSL context.  That gives the
SSL context ownership of the list, so that SSL_CTX_free will free it.
This avoids a permanent memory leak if we fail further down in
be_tls_init(), which could happen if bogus CRL data is offered.

The leak could only amount to something if the CRL parameters get
broken after server start (else we'd just quit) and then the server
is SIGHUP'd many times without fixing the CRL data.  That's rather
unlikely perhaps, but it seems worth fixing, if only because the
code is clearer this way.

While we're here, add some comments about the memory management
aspects of this logic.

Noted by Jelte Fennema and independently by Andres Freund.
Back-patch to v10; before commit de41869b6 it doesn't matter,
since we'd not re-execute this code during SIGHUP.

Discussion: https://postgr.es/m/16160-18367e56e9a28264@postgresql.org

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 8c37381add39ff1b2b59dc3268aecd231de7155f..ad33122e0ed08e44e37fc465176ae69234797b85 100644 (file)
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -81,7 +81,6 @@ static const char *ssl_protocol_version_to_string(int v);
 int
 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;
@@ -100,6 +99,10 @@ be_tls_init(bool isServerStart)
    }
 
    /*
+    * Create a new SSL context into which we'll load all the configuration
+    * settings.  If we fail partway through, we can avoid memory leakage by
+    * freeing this context; we don't install it as active until the end.
+    *
     * We use SSLv23_method() because it can negotiate use of the highest
     * mutually supported protocol version, while alternatives like
     * TLSv1_2_method() permit only one specific version.  Note that we don't
@@ -272,6 +275,8 @@ be_tls_init(bool isServerStart)
     */
    if (ssl_ca_file[0])
    {
+       STACK_OF(X509_NAME) * root_cert_list;
+
        if (SSL_CTX_load_verify_locations(context, ssl_ca_file, NULL) != 1 ||
            (root_cert_list = SSL_load_client_CA_file(ssl_ca_file)) == NULL)
        {
@@ -281,6 +286,25 @@ be_tls_init(bool isServerStart)
                            ssl_ca_file, SSLerrmessage(ERR_get_error()))));
            goto error;
        }
+
+       /*
+        * Tell OpenSSL to send the list of root certs we trust to clients in
+        * CertificateRequests.  This lets a client with a keystore select the
+        * appropriate client certificate to send to us.  Also, this ensures
+        * that the SSL context will "own" the root_cert_list and remember to
+        * free it when no longer needed.
+        */
+       SSL_CTX_set_client_CA_list(context, root_cert_list);
+
+       /*
+        * Always ask for SSL client cert, but don't fail if it's not
+        * presented.  We might fail such connections later, depending on what
+        * we find in pg_hba.conf.
+        */
+       SSL_CTX_set_verify(context,
+                          (SSL_VERIFY_PEER |
+                           SSL_VERIFY_CLIENT_ONCE),
+                          verify_cb);
    }
 
    /*----------
@@ -297,7 +321,7 @@ be_tls_init(bool isServerStart)
            /* Set the flags to check against the complete CRL chain */
            if (X509_STORE_load_locations(cvstore,
                                          ssl_crl_file[0] ? ssl_crl_file : NULL,
-                                         ssl_crl_dir[0]  ? ssl_crl_dir : NULL)
+                                         ssl_crl_dir[0] ? ssl_crl_dir : NULL)
                == 1)
            {
                X509_STORE_set_flags(cvstore,
@@ -331,26 +355,6 @@ be_tls_init(bool isServerStart)
        }
    }
 
-   if (ssl_ca_file[0])
-   {
-       /*
-        * Always ask for SSL client cert, but don't fail if it's not
-        * presented.  We might fail such connections later, depending on what
-        * we find in pg_hba.conf.
-        */
-       SSL_CTX_set_verify(context,
-                          (SSL_VERIFY_PEER |
-                           SSL_VERIFY_CLIENT_ONCE),
-                          verify_cb);
-
-       /*
-        * Tell OpenSSL to send the list of root certs we trust to clients in
-        * CertificateRequests.  This lets a client with a keystore select the
-        * appropriate client certificate to send to us.
-        */
-       SSL_CTX_set_client_CA_list(context, root_cert_list);
-   }
-
    /*
     * Success!  Replace any existing SSL_context.
     */
@@ -369,6 +373,7 @@ be_tls_init(bool isServerStart)
 
    return 0;
 
+   /* Clean up by releasing working context. */
 error:
    if (context)
        SSL_CTX_free(context);