public inbox for [email protected]  

help / color / mirror / Atom feed

From: Jacob Champion <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Cc: Michael Paquier <[email protected]>
Subject: [PATCH] Align GSS and TLS error handling in PQconnectPoll()
Date: Mon, 13 Feb 2023 10:49:17 -0800
Message-ID: <[email protected]> (raw)

Hi all,

During the gssencmode CVE discussion, we noticed that PQconnectPoll()
handles the error cases for TLS and GSS transport encryption slightly
differently. After TLS fails, the connection handle is dead and future
calls to PQconnectPoll() return immediately. But after GSS encryption
fails, the connection handle can still be used to reenter the GSS
handling code.

This doesn't appear to have any security implications today -- and a
client has to actively try to reuse a handle that's already failed --
but it seems undesirable. Michael (cc'd) came up with a patch, which I
have attached here and will register in the CF.

Thanks,
--Jacob

Attachments:

  [text/x-patch] PQconnectPoll-poison-connection-on-gssenc-error.patch (1.9K, ../[email protected]/2-PQconnectPoll-poison-connection-on-gssenc-error.patch)
  download | inline diff:
From 75b1be5f484a28e543290e208474d3603a3270bf Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Mon, 13 Feb 2023 10:30:43 -0800
Subject: [PATCH] PQconnectPoll: poison connection on gssenc error

Currently, conn->status isn't updated when gssenc establishment fails,
which allows any (misbehaved) future calls to PQconnectPoll() to reenter
CONNECTION_GSS_STARTUP. Align this code with the CONNECTION_SSL_STARTUP
case, which jumps to error_return and poisons the connection handle.

Patch is by Michael Paquier; I just rebased over HEAD.
---
 src/interfaces/libpq/fe-connect.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 50b5df3490..1070b6db24 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -3148,17 +3148,22 @@ keep_going:						/* We will come back to here until there is
 					conn->status = CONNECTION_MADE;
 					return PGRES_POLLING_WRITING;
 				}
-				else if (pollres == PGRES_POLLING_FAILED &&
-						 conn->gssencmode[0] == 'p')
+				else if (pollres == PGRES_POLLING_FAILED)
 				{
-					/*
-					 * We failed, but we can retry on "prefer".  Have to drop
-					 * the current connection to do so, though.
-					 */
-					conn->try_gss = false;
-					need_new_connection = true;
-					goto keep_going;
+					if (conn->gssencmode[0] == 'p')
+					{
+						/*
+						 * We failed, but we can retry on "prefer".  Have to drop
+						 * the current connection to do so, though.
+						 */
+						conn->try_gss = false;
+						need_new_connection = true;
+						goto keep_going;
+					}
+					/* Else it's a hard failure */
+					goto error_return;
 				}
+				/* Else, return POLLING_READING or POLLING_WRITING status */
 				return pollres;
 #else							/* !ENABLE_GSS */
 				/* unreachable */
-- 
2.25.1

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Align GSS and TLS error handling in PQconnectPoll()
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox