Fix bugs in libpq's GSSAPI encryption support.

The critical issue fixed here is that if a GSSAPI-encrypted connection
is successfully made, pqsecure_open_gss() cleared conn->allow_ssl_try,
as an admittedly-hacky way of preventing us from then trying to tunnel
SSL encryption over the already-encrypted connection.  The problem
with that is that if we abandon the GSSAPI connection because of a
failure during authentication, we would not attempt SSL encryption
in the next try with the same server.  This can lead to unexpected
connection failure, or silently getting a non-encrypted connection
where an encrypted one is expected.

Fortunately, we'd only manage to make a GSSAPI-encrypted connection
if both client and server hold valid tickets in the same Kerberos
infrastructure, which is a relatively uncommon environment.
Nonetheless this is a very nasty bug with potential security
consequences.  To fix, don't reset the flag, instead adding a
check for conn->gssenc being already true when deciding whether
to try to initiate SSL.

While here, fix some lesser issues in libpq's GSSAPI code:

* Use the need_new_connection stanza when dropping an attempted
GSSAPI connection, instead of partially duplicating that code.
The consequences of this are pretty minor: AFAICS it could only
lead to auth_req_received or password_needed remaining set when
they shouldn't, which is not too harmful.

* Fix pg_GSS_error() to not repeat the "mprefix" it's given multiple
times, and to notice any failure return from gss_display_status().

* Avoid gratuitous dependency on NI_MAXHOST in
pg_GSS_load_servicename().

Per report from Mikael Gustavsson.  Back-patch to v12 where
this code was introduced.

Discussion: https://postgr.es/m/e5b0b6ed05764324a2f3fe7acfc766d5@smhi.se

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d2b1c207eb4d41a13b83da313005554dd4fac32c..ae4866c8e427eaba335bb81aa2e36e0054d6f7e3 100644 (file)
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2909,11 +2909,16 @@ keep_going:                                             /* We will come back to here until there is
 #ifdef USE_SSL
 
                                /*
-                                * If SSL is enabled and we haven't already got it running,
-                                * request it instead of sending the startup message.
+                                * If SSL is enabled and we haven't already got encryption of
+                                * some sort running, request SSL instead of sending the
+                                * startup message.
                                 */
                                if (conn->allow_ssl_try && !conn->wait_ssl_try &&
-                                       !conn->ssl_in_use)
+                                       !conn->ssl_in_use
+#ifdef ENABLE_GSS
+                                       && !conn->gssenc
+#endif
+                                       )
                                {
                                        ProtocolVersion pv;
 
@@ -3042,6 +3047,7 @@ keep_going:                                               /* We will come back to here until there is
                                                }
                                                /* Otherwise, proceed with normal startup */
                                                conn->allow_ssl_try = false;
+                                               /* We can proceed using this connection */
                                                conn->status = CONNECTION_MADE;
                                                return PGRES_POLLING_WRITING;
                                        }
@@ -3139,8 +3145,7 @@ keep_going:                                               /* We will come back to here until there is
                                                 * don't hang up the socket, though.
                                                 */
                                                conn->try_gss = false;
-                                               pqDropConnection(conn, true);
-                                               conn->status = CONNECTION_NEEDED;
+                                               need_new_connection = true;
                                                goto keep_going;
                                        }
 
@@ -3158,6 +3163,7 @@ keep_going:                                               /* We will come back to here until there is
                                                }
 
                                                conn->try_gss = false;
+                                               /* We can proceed using this connection */
                                                conn->status = CONNECTION_MADE;
                                                return PGRES_POLLING_WRITING;
                                        }
@@ -3186,8 +3192,7 @@ keep_going:                                               /* We will come back to here until there is
                                         * the current connection to do so, though.
                                         */
                                        conn->try_gss = false;
-                                       pqDropConnection(conn, true);
-                                       conn->status = CONNECTION_NEEDED;
+                                       need_new_connection = true;
                                        goto keep_going;
                                }
                                return pollres;
@@ -3354,10 +3359,9 @@ keep_going:                                              /* We will come back to here until there is
                                         */
                                        if (conn->gssenc && conn->gssencmode[0] == 'p')
                                        {
-                                               /* postmaster expects us to drop the connection */
+                                               /* only retry once */
                                                conn->try_gss = false;
-                                               pqDropConnection(conn, true);
-                                               conn->status = CONNECTION_NEEDED;
+                                               need_new_connection = true;
                                                goto keep_going;
                                        }
 #endif

diff --git a/src/interfaces/libpq/fe-gssapi-common.c b/src/interfaces/libpq/fe-gssapi-common.c
index a892df91ca8266420f97ccd90f52221a36be21e1..6538a421c701d3e4bc099b318d761584c2534262 100644 (file)
--- a/src/interfaces/libpq/fe-gssapi-common.c
+++ b/src/interfaces/libpq/fe-gssapi-common.c
@@ -20,10 +20,10 @@
 
 /*
  * Fetch all errors of a specific type and append to "str".
+ * Each error string is preceded by a space.
  */
 static void
-pg_GSS_error_int(PQExpBuffer str, const char *mprefix,
-                                OM_uint32 stat, int type)
+pg_GSS_error_int(PQExpBuffer str, OM_uint32 stat, int type)
 {
        OM_uint32       lmin_s;
        gss_buffer_desc lmsg;
@@ -31,9 +31,10 @@ pg_GSS_error_int(PQExpBuffer str, const char *mprefix,
 
        do
        {
-               gss_display_status(&lmin_s, stat, type,
-                                                  GSS_C_NO_OID, &msg_ctx, &lmsg);
-               appendPQExpBuffer(str, "%s: %s\n", mprefix, (char *) lmsg.value);
+               if (gss_display_status(&lmin_s, stat, type, GSS_C_NO_OID,
+                                                          &msg_ctx, &lmsg) != GSS_S_COMPLETE)
+                       break;
+               appendPQExpBuffer(str, " %s", (char *) lmsg.value);
                gss_release_buffer(&lmin_s, &lmsg);
        } while (msg_ctx);
 }
@@ -46,12 +47,11 @@ pg_GSS_error(const char *mprefix, PGconn *conn,
                         OM_uint32 maj_stat, OM_uint32 min_stat)
 {
        resetPQExpBuffer(&conn->errorMessage);
-
-       /* Fetch major error codes */
-       pg_GSS_error_int(&conn->errorMessage, mprefix, maj_stat, GSS_C_GSS_CODE);
-
-       /* Add the minor codes as well */
-       pg_GSS_error_int(&conn->errorMessage, mprefix, min_stat, GSS_C_MECH_CODE);
+       appendPQExpBuffer(&conn->errorMessage, "%s:", mprefix);
+       pg_GSS_error_int(&conn->errorMessage, maj_stat, GSS_C_GSS_CODE);
+       appendPQExpBufferChar(&conn->errorMessage, ':');
+       pg_GSS_error_int(&conn->errorMessage, min_stat, GSS_C_MECH_CODE);
+       appendPQExpBufferChar(&conn->errorMessage, '\n');
 }
 
 /*
@@ -103,7 +103,7 @@ pg_GSS_load_servicename(PGconn *conn)
         * Import service principal name so the proper ticket can be acquired by
         * the GSSAPI system.
         */
-       maxlen = NI_MAXHOST + strlen(conn->krbsrvname) + 2;
+       maxlen = strlen(conn->krbsrvname) + strlen(host) + 2;
        temp_gbuf.value = (char *) malloc(maxlen);
        if (!temp_gbuf.value)
        {

diff --git a/src/interfaces/libpq/fe-secure-gssapi.c b/src/interfaces/libpq/fe-secure-gssapi.c
index bfc0f5521468bb4b6f3605d978d62e8c35204d0f..9416306eea02b9cc98505a2f2c543342d166155d 100644 (file)
--- a/src/interfaces/libpq/fe-secure-gssapi.c
+++ b/src/interfaces/libpq/fe-secure-gssapi.c
@@ -647,17 +647,14 @@ pqsecure_open_gss(PGconn *conn)
        if (output.length == 0)
        {
                /*
-                * We're done - hooray!  Kind of gross, but we need to disable SSL
-                * here so that we don't accidentally tunnel one over the other.
+                * We're done - hooray!  Set flag to tell the low-level I/O routines
+                * to do GSS wrapping/unwrapping.
                 */
-#ifdef USE_SSL
-               conn->allow_ssl_try = false;
-#endif
+               conn->gssenc = true;
 
                /* Clean up */
                gss_release_cred(&minor, &conn->gcred);
                conn->gcred = GSS_C_NO_CREDENTIAL;
-               conn->gssenc = true;
                gss_release_buffer(&minor, &output);
 
                /*