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
This commit is contained in:
Tom Lane 2020-12-28 15:43:44 -05:00
parent cf61b0734c
commit ff6ce9a3a6
3 changed files with 29 additions and 28 deletions

View File

@ -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

View File

@ -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)
{

View File

@ -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);
/*