database/postgres
database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-05-03 22:24:49 +03:00
Code

Fix bugs in libpq's GSSAPI encryption support.

Browse Source 
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 31d2b11b94
commit 06b844c2b8
3 changed files with 29 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
src/interfaces/libpq/fe-connect.c
View File

@ -2904,11 +2904,16 @@ keep_going: /* We will come back to here until there is
#ifdef USE_SSL #ifdef USE_SSL
/* /*
* If SSL is enabled and we haven't already got it running, * If SSL is enabled and we haven't already got encryption of
* request it instead of sending the startup message. * some sort running, request SSL instead of sending the
* startup message.
*/ */
if (conn->allow_ssl_try && !conn->wait_ssl_try && 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; ProtocolVersion pv;
@ -3037,6 +3042,7 @@ keep_going: /* We will come back to here until there is
} }
/* Otherwise, proceed with normal startup */ /* Otherwise, proceed with normal startup */
conn->allow_ssl_try = false; conn->allow_ssl_try = false;
/* We can proceed using this connection */
conn->status = CONNECTION_MADE; conn->status = CONNECTION_MADE;
return PGRES_POLLING_WRITING; return PGRES_POLLING_WRITING;
} }
@ -3134,8 +3140,7 @@ keep_going: /* We will come back to here until there is
* don't hang up the socket, though. * don't hang up the socket, though.
*/ */
conn->try_gss = false; conn->try_gss = false;
pqDropConnection(conn, true); need_new_connection = true;
conn->status = CONNECTION_NEEDED;
goto keep_going; goto keep_going;
} }
@ -3153,6 +3158,7 @@ keep_going: /* We will come back to here until there is
} }
conn->try_gss = false; conn->try_gss = false;
/* We can proceed using this connection */
conn->status = CONNECTION_MADE; conn->status = CONNECTION_MADE;
return PGRES_POLLING_WRITING; return PGRES_POLLING_WRITING;
} }
@ -3181,8 +3187,7 @@ keep_going: /* We will come back to here until there is
* the current connection to do so, though. * the current connection to do so, though.
*/ */
conn->try_gss = false; conn->try_gss = false;
pqDropConnection(conn, true); need_new_connection = true;
conn->status = CONNECTION_NEEDED;
goto keep_going; goto keep_going;
} }
return pollres; return pollres;
@ -3349,10 +3354,9 @@ keep_going: /* We will come back to here until there is
*/ */
if (conn->gssenc && conn->gssencmode[0] == 'p') if (conn->gssenc && conn->gssencmode[0] == 'p')
{ {
/* postmaster expects us to drop the connection */ /* only retry once */
conn->try_gss = false; conn->try_gss = false;
pqDropConnection(conn, true); need_new_connection = true;
conn->status = CONNECTION_NEEDED;
goto keep_going; goto keep_going;
} }
#endif #endif

24
src/interfaces/libpq/fe-gssapi-common.c
View File

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

9
src/interfaces/libpq/fe-secure-gssapi.c
View File

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