postgres.git / summary / log / commit / refs

commit    e6c33d594a004a2c831cdff1a16276347d30f703
Author:   Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date:     Wed Jun 07 11:01:46 2017 +0000

    Clear auth context correctly when re-connecting after failed auth attempt.
    
    If authentication over an SSL connection fails, with sslmode=prefer,
    libpq will reconnect without SSL and retry. However, we did not clear
    the variables related to GSS, SSPI, and SASL authentication state, when
    reconnecting. Because of that, the second authentication attempt would
    always fail with a "duplicate GSS/SASL authentication request" error.
    pg_SSPI_startup did not check for duplicate authentication requests like
    the corresponding GSS and SASL functions, so with SSPI, you would leak
    some memory instead.
    
    Another way this could manifest itself, on version 10, is if you list
    multiple hostnames in the "host" parameter. If the first server requests
    Kerberos or SCRAM authentication, but it fails, the attempts to connect to
    the other servers will also fail with "duplicate authentication request"
    errors.
    
    To fix, move the clearing of authentication state from closePGconn to
    pgDropConnection, so that it is cleared also when re-connecting.
    
    Patch by Michael Paquier, with some kibitzing by me.
    
    Backpatch down to 9.3. 9.2 has the same bug, but the code around closing
    the connection is somewhat different, so that this patch doesn't apply.
    To fix this in 9.2, I think we would need to back-port commit 210eb9b743
    first, and then apply this patch. However, given that we only bumped into
    this in our own testing, we haven't heard any reports from users about
    this, and that 9.2 will be end-of-lifed in a couple of months anyway, it
    doesn't seem worth the risk and trouble.
    
    Discussion: https://www.postgresql.org/message-id/CAB7nPqRuOUm0MyJaUy9L3eXYJU3AKCZ-0-03=-aDTZJGV4GyWw@mail.gmail.com


 src/interfaces/libpq/fe-auth.c    |  7 +++-
 src/interfaces/libpq/fe-connect.c | 80 +++++++++++++++++++++------------------
 2 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index f30fb04809c..ad9b0315e13 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -416,7 +416,12 @@ pg_SSPI_startup(PGconn *conn, int use_negotiate, int payloadlen)
 	TimeStamp	expire;
 	char	   *host = PQhost(conn);
 
-	conn->sspictx = NULL;
+	if (conn->sspictx)
+	{
+		printfPQExpBuffer(&conn->errorMessage,
+					libpq_gettext("duplicate SSPI authentication request\n"));
+		return STATUS_ERROR;
+	}
 
 	/*
 	 * Retrieve credentials handle
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index f2c9bf7a883..77b170af94d 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -406,15 +406,59 @@ pqDropConnection(PGconn *conn, bool flushInput)
 {
 	/* Drop any SSL state */
 	pqsecure_close(conn);
+
 	/* Close the socket itself */
 	if (conn->sock != PGINVALID_SOCKET)
 		closesocket(conn->sock);
 	conn->sock = PGINVALID_SOCKET;
+
 	/* Optionally discard any unread data */
 	if (flushInput)
 		conn->inStart = conn->inCursor = conn->inEnd = 0;
+
 	/* Always discard any unsent data */
 	conn->outCount = 0;
+
+	/* Free authentication state */
+#ifdef ENABLE_GSS
+	{
+		OM_uint32	min_s;
+
+		if (conn->gctx)
+			gss_delete_sec_context(&min_s, &conn->gctx, GSS_C_NO_BUFFER);
+		if (conn->gtarg_nam)
+			gss_release_name(&min_s, &conn->gtarg_nam);
+	}
+#endif
+#ifdef ENABLE_SSPI
+	if (conn->sspitarget)
+	{
+		free(conn->sspitarget);
+		conn->sspitarget = NULL;
+	}
+	if (conn->sspicred)
+	{
+		FreeCredentialsHandle(conn->sspicred);
+		free(conn->sspicred);
+		conn->sspicred = NULL;
+	}
+	if (conn->sspictx)
+	{
+		DeleteSecurityContext(conn->sspictx);
+		free(conn->sspictx);
+		conn->sspictx = NULL;
+	}
+	conn->usesspi = 0;
+#endif
+	if (conn->sasl_state)
+	{
+		/*
+		 * XXX: if support for more authentication mechanisms is added, this
+		 * needs to call the right 'free' function.
+		 */
+		pg_fe_scram_free(conn->sasl_state);
+		conn->sasl_state = NULL;
+	}
 }
 
 
@@ -3475,42 +3519,6 @@ closePGconn(PGconn *conn)
 	if (conn->lobjfuncs)
 		free(conn->lobjfuncs);
 	conn->lobjfuncs = NULL;
-#ifdef ENABLE_GSS
-	{
-		OM_uint32	min_s;
-
-		if (conn->gctx)
-			gss_delete_sec_context(&min_s, &conn->gctx, GSS_C_NO_BUFFER);
-		if (conn->gtarg_nam)
-			gss_release_name(&min_s, &conn->gtarg_nam);
-	}
-#endif
-#ifdef ENABLE_SSPI
-	if (conn->sspitarget)
-		free(conn->sspitarget);
-	conn->sspitarget = NULL;
-	if (conn->sspicred)
-	{
-		FreeCredentialsHandle(conn->sspicred);
-		free(conn->sspicred);
-		conn->sspicred = NULL;
-	}
-	if (conn->sspictx)
-	{
-		DeleteSecurityContext(conn->sspictx);
-		free(conn->sspictx);
-		conn->sspictx = NULL;
-	}
-#endif
-	if (conn->sasl_state)
-	{
-		/*
-		 * XXX: if support for more authentication mechanisms is added, this
-		 * needs to call the right 'free' function.
-		 */
-		pg_fe_scram_free(conn->sasl_state);
-		conn->sasl_state = NULL;
-	}
 }
 
 /*


[parent: 3344582e6f16]