Prevent a double free by not reentering be_tls_close().

Reentering this function with the right timing caused a double free,
typically crashing the backend.  By synchronizing a disconnection with
the authentication timeout, an unauthenticated attacker could achieve
this somewhat consistently.  Call be_tls_close() solely from within
proc_exit_prepare().  Back-patch to 9.0 (all supported versions).

Benkocs Norbert Attila

Security: CVE-2015-3165

diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index 89c30d06542afb1fae3e5589af1b07d5204f472b..7f80cc8d2fc402829ff819b137121a4f69276983 100644 (file)
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -990,7 +990,6 @@ open_server_SSL(Port *port)
                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
                                 errmsg("could not initialize SSL connection: %s",
                                                SSLerrmessage())));
-               close_SSL(port);
                return -1;
        }
        if (!my_SSL_set_fd(port->ssl, port->sock))
@@ -999,7 +998,6 @@ open_server_SSL(Port *port)
                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
                                 errmsg("could not set SSL socket: %s",
                                                SSLerrmessage())));
-               close_SSL(port);
                return -1;
        }
 
@@ -1047,7 +1045,6 @@ aloop:
                                                                err)));
                                break;
                }
-               close_SSL(port);
                return -1;
        }
 
@@ -1076,7 +1073,6 @@ aloop:
                        {
                                /* shouldn't happen */
                                pfree(peer_cn);
-                               close_SSL(port);
                                return -1;
                        }
 
@@ -1090,7 +1086,6 @@ aloop:
                                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
                                                 errmsg("SSL certificate's common name contains embedded null")));
                                pfree(peer_cn);
-                               close_SSL(port);
                                return -1;
                        }
 

diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index c08c5d73ba403ac8a7bad33bdbaf1e8343c5c820..49a394be94312138645a7fb53ea47f8f974da217 100644 (file)
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -182,32 +182,45 @@ pq_comm_reset(void)
 /* --------------------------------
  *             pq_close - shutdown libpq at backend exit
  *
- * Note: in a standalone backend MyProcPort will be null,
- * don't crash during exit...
+ * This is the one pg_on_exit_callback in place during BackendInitialize().
+ * That function's unusual signal handling constrains that this callback be
+ * safe to run at any instant.
  * --------------------------------
  */
 static void
 pq_close(int code, Datum arg)
 {
+       /* Nothing to do in a standalone backend, where MyProcPort is NULL. */
        if (MyProcPort != NULL)
        {
 #if defined(ENABLE_GSS) || defined(ENABLE_SSPI)
 #ifdef ENABLE_GSS
                OM_uint32       min_s;
 
-               /* Shutdown GSSAPI layer */
+               /*
+                * Shutdown GSSAPI layer.  This section does nothing when interrupting
+                * BackendInitialize(), because pg_GSS_recvauth() makes first use of
+                * "ctx" and "cred".
+                */
                if (MyProcPort->gss->ctx != GSS_C_NO_CONTEXT)
                        gss_delete_sec_context(&min_s, &MyProcPort->gss->ctx, NULL);
 
                if (MyProcPort->gss->cred != GSS_C_NO_CREDENTIAL)
                        gss_release_cred(&min_s, &MyProcPort->gss->cred);
 #endif   /* ENABLE_GSS */
-               /* GSS and SSPI share the port->gss struct */
 
+               /*
+                * GSS and SSPI share the port->gss struct.  Since nowhere else does a
+                * postmaster child free this, doing so is safe when interrupting
+                * BackendInitialize().
+                */
                free(MyProcPort->gss);
 #endif   /* ENABLE_GSS || ENABLE_SSPI */
 
-               /* Cleanly shut down SSL layer */
+               /*
+                * Cleanly shut down SSL layer.  Nowhere else does a postmaster child
+                * call this, so this is safe when interrupting BackendInitialize().
+                */
                secure_close(MyProcPort);
 
                /*

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index f05114d129b9a808ac07a90278b682c27b7e85fe..d6721d7971c7f2a3afd6f882b49f2331c0ad148f 100644 (file)
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -3961,7 +3961,16 @@ BackendInitialize(Port *port)
         * We arrange for a simple exit(1) if we receive SIGTERM or SIGQUIT or
         * timeout while trying to collect the startup packet.  Otherwise the
         * postmaster cannot shutdown the database FAST or IMMED cleanly if a
-        * buggy client fails to send the packet promptly.
+        * buggy client fails to send the packet promptly.  XXX it follows that
+        * the remainder of this function must tolerate losing control at any
+        * instant.  Likewise, any pg_on_exit_callback registered before or during
+        * this function must be prepared to execute at any instant between here
+        * and the end of this function.  Furthermore, affected callbacks execute
+        * partially or not at all when a second exit-inducing signal arrives
+        * after proc_exit_prepare() decrements on_proc_exit_index.  (Thanks to
+        * that mechanic, callbacks need not anticipate more than one call.)  This
+        * is fragile; it ought to instead follow the norm of handling interrupts
+        * at selected, safe opportunities.
         */
        pqsignal(SIGTERM, startup_die);
        pqsignal(SIGQUIT, startup_die);