Fix timing-dependent failure in GSSAPI data transmission.

tglsfdc · tglsfdc · commit d053a879bb36 · 2023-11-23T13:30:18.000-05:00
When using GSSAPI encryption in non-blocking mode, libpq sometimes failed with "GSSAPI caller failed to retransmit all data needing to be retried". The cause is that pqPutMsgEnd rounds its transmit request down to an even multiple of 8K, and sometimes that can lead to not requesting a write of data that was requested to be written (but reported as not written) earlier. That can upset pg_GSS_write's logic for dealing with not-yet-written data, since it's possible the data in question had already been incorporated into an encrypted packet that we weren't able to send during the previous call. We could fix this with a one-or-two-line hack to disable pqPutMsgEnd's round-down behavior, but that seems like making the caller work around a behavior that pg_GSS_write shouldn't expose in this way. Instead, adjust pg_GSS_write to never report a partial write: it either reports a complete write, or reflects the failure of the lower-level pqsecure_raw_write call. The requirement still exists for the caller to present at least as much data as on the previous call, but with the caller-visible write start point not moving there is no temptation for it to present less. We lose some ability to reclaim buffer space early, but I doubt that that will make much difference in practice. This also gets rid of a rather dubious assumption that "any interesting failure condition (from pqsecure_raw_write) will recur on the next try". We've not seen failure reports traceable to that, but I've never trusted it particularly and am glad to remove it. Make the same adjustments to the equivalent backend routine be_gssapi_write(). It is probable that there's no bug on the backend side, since we don't have a notion of nonblock mode there; but we should keep the logic the same to ease future maintenance. Per bug #18210 from Lars Kanis. Back-patch to all supported branches. Discussion: https://postgr.es/m/18210-4c6d0b14627f2eb8@postgresql.org
diff --git a/src/backend/libpq/be-secure-gssapi.c b/src/backend/libpq/be-secure-gssapi.c
@@ -60,8 +60,8 @@ static char *PqGSSSendBuffer;	/* Encrypted data waiting to be sent */
 static int	PqGSSSendLength;	/* End of data available in PqGSSSendBuffer */
 static int	PqGSSSendNext;		/* Next index to send a byte from
 								 * PqGSSSendBuffer */
-static int	PqGSSSendConsumed;	/* Number of *unencrypted* bytes consumed for
-								 * current contents of PqGSSSendBuffer */
+static int	PqGSSSendConsumed;	/* Number of source bytes encrypted but not
+								 * yet reported as sent */
 
 static char *PqGSSRecvBuffer;	/* Received, encrypted data */
 static int	PqGSSRecvLength;	/* End of data available in PqGSSRecvBuffer */
@@ -83,8 +83,8 @@ static uint32 PqGSSMaxPktSize;	/* Maximum size we can encrypt and fit the
  *
  * On success, returns the number of data bytes consumed (possibly less than
  * len).  On failure, returns -1 with errno set appropriately.  For retryable
- * errors, caller should call again (passing the same data) once the socket
- * is ready.
+ * errors, caller should call again (passing the same or more data) once the
+ * socket is ready.
  *
  * Dealing with fatal errors here is a bit tricky: we can't invoke elog(FATAL)
  * since it would try to write to the client, probably resulting in infinite
@@ -98,26 +98,33 @@ be_gssapi_write(Port *port, void *ptr, size_t len)
 				minor;
 	gss_buffer_desc input,
 				output;
-	size_t		bytes_sent = 0;
 	size_t		bytes_to_encrypt;
 	size_t		bytes_encrypted;
 	gss_ctx_id_t gctx = port->gss->ctx;
 
 	/*
-	 * When we get a failure, we must not tell the caller we have successfully
-	 * transmitted everything, else it won't retry.  Hence a "success"
-	 * (positive) return value must only count source bytes corresponding to
-	 * fully-transmitted encrypted packets.  The amount of source data
-	 * corresponding to the current partly-transmitted packet is remembered in
+	 * When we get a retryable failure, we must not tell the caller we have
+	 * successfully transmitted everything, else it won't retry.  For
+	 * simplicity, we claim we haven't transmitted anything until we have
+	 * successfully transmitted all "len" bytes.  Between calls, the amount of
+	 * the current input data that's already been encrypted and placed into
+	 * PqGSSSendBuffer (and perhaps transmitted) is remembered in
 	 * PqGSSSendConsumed.  On a retry, the caller *must* be sending that data
 	 * again, so if it offers a len less than that, something is wrong.
+	 *
+	 * Note: it may seem attractive to report partial write completion once
+	 * we've successfully sent any encrypted packets.  However, that can cause
+	 * problems for callers; notably, pqPutMsgEnd's heuristic to send only
+	 * full 8K blocks interacts badly with such a hack.  We won't save much,
+	 * typically, by letting callers discard data early, so don't risk it.
 	 */
 	if (len < PqGSSSendConsumed)
 	{
 		elog(COMMERROR, "GSSAPI caller failed to retransmit all data needing to be retried");
 		errno = ECONNRESET;
 		return -1;
 	}
+
 	/* Discount whatever source data we already encrypted. */
 	bytes_to_encrypt = len - PqGSSSendConsumed;
 	bytes_encrypted = PqGSSSendConsumed;
@@ -146,33 +153,20 @@ be_gssapi_write(Port *port, void *ptr, size_t len)
 
 			ret = secure_raw_write(port, PqGSSSendBuffer + PqGSSSendNext, amount);
 			if (ret <= 0)
-			{
-				/*
-				 * Report any previously-sent data; if there was none, reflect
-				 * the secure_raw_write result up to our caller.  When there
-				 * was some, we're effectively assuming that any interesting
-				 * failure condition will recur on the next try.
-				 */
-				if (bytes_sent)
-					return bytes_sent;
 				return ret;
-			}
 
 			/*
 			 * Check if this was a partial write, and if so, move forward that
 			 * far in our buffer and try again.
 			 */
-			if (ret != amount)
+			if (ret < amount)
 			{
 				PqGSSSendNext += ret;
 				continue;
 			}
 
-			/* We've successfully sent whatever data was in that packet. */
-			bytes_sent += PqGSSSendConsumed;
-
-			/* All encrypted data was sent, our buffer is empty now. */
-			PqGSSSendLength = PqGSSSendNext = PqGSSSendConsumed = 0;
+			/* We've successfully sent whatever data was in the buffer. */
+			PqGSSSendLength = PqGSSSendNext = 0;
 		}
 
 		/*
@@ -196,7 +190,10 @@ be_gssapi_write(Port *port, void *ptr, size_t len)
 		output.value = NULL;
 		output.length = 0;
 
-		/* Create the next encrypted packet */
+		/*
+		 * Create the next encrypted packet.  Any failure here is considered a
+		 * hard failure, so we return -1 even if some data has been sent.
+		 */
 		major = gss_wrap(&minor, gctx, 1, GSS_C_QOP_DEFAULT,
 						 &input, &conf_state, &output);
 		if (major != GSS_S_COMPLETE)
@@ -239,10 +236,13 @@ be_gssapi_write(Port *port, void *ptr, size_t len)
 	}
 
 	/* If we get here, our counters should all match up. */
-	Assert(bytes_sent == len);
-	Assert(bytes_sent == bytes_encrypted);
+	Assert(len == PqGSSSendConsumed);
+	Assert(len == bytes_encrypted);
+
+	/* We're reporting all the data as sent, so reset PqGSSSendConsumed. */
+	PqGSSSendConsumed = 0;
 
-	return bytes_sent;
+	return bytes_encrypted;
 }
 
 /*
diff --git a/src/interfaces/libpq/fe-secure-gssapi.c b/src/interfaces/libpq/fe-secure-gssapi.c
@@ -79,8 +79,8 @@
  * On success, returns the number of data bytes consumed (possibly less than
  * len).  On failure, returns -1 with errno set appropriately.  If the errno
  * indicates a non-retryable error, a message is added to conn->errorMessage.
- * For retryable errors, caller should call again (passing the same data)
- * once the socket is ready.
+ * For retryable errors, caller should call again (passing the same or more
+ * data) once the socket is ready.
  */
 ssize_t
 pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
@@ -90,19 +90,25 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
 	gss_buffer_desc input,
 				output = GSS_C_EMPTY_BUFFER;
 	ssize_t		ret = -1;
-	size_t		bytes_sent = 0;
 	size_t		bytes_to_encrypt;
 	size_t		bytes_encrypted;
 	gss_ctx_id_t gctx = conn->gctx;
 
 	/*
-	 * When we get a failure, we must not tell the caller we have successfully
-	 * transmitted everything, else it won't retry.  Hence a "success"
-	 * (positive) return value must only count source bytes corresponding to
-	 * fully-transmitted encrypted packets.  The amount of source data
-	 * corresponding to the current partly-transmitted packet is remembered in
+	 * When we get a retryable failure, we must not tell the caller we have
+	 * successfully transmitted everything, else it won't retry.  For
+	 * simplicity, we claim we haven't transmitted anything until we have
+	 * successfully transmitted all "len" bytes.  Between calls, the amount of
+	 * the current input data that's already been encrypted and placed into
+	 * PqGSSSendBuffer (and perhaps transmitted) is remembered in
 	 * PqGSSSendConsumed.  On a retry, the caller *must* be sending that data
 	 * again, so if it offers a len less than that, something is wrong.
+	 *
+	 * Note: it may seem attractive to report partial write completion once
+	 * we've successfully sent any encrypted packets.  However, that can cause
+	 * problems for callers; notably, pqPutMsgEnd's heuristic to send only
+	 * full 8K blocks interacts badly with such a hack.  We won't save much,
+	 * typically, by letting callers discard data early, so don't risk it.
 	 */
 	if (len < PqGSSSendConsumed)
 	{
@@ -140,33 +146,20 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
 
 			retval = pqsecure_raw_write(conn, PqGSSSendBuffer + PqGSSSendNext, amount);
 			if (retval <= 0)
-			{
-				/*
-				 * Report any previously-sent data; if there was none, reflect
-				 * the pqsecure_raw_write result up to our caller.  When there
-				 * was some, we're effectively assuming that any interesting
-				 * failure condition will recur on the next try.
-				 */
-				if (bytes_sent)
-					return bytes_sent;
 				return retval;
-			}
 
 			/*
 			 * Check if this was a partial write, and if so, move forward that
 			 * far in our buffer and try again.
 			 */
-			if (retval != amount)
+			if (retval < amount)
 			{
 				PqGSSSendNext += retval;
 				continue;
 			}
 
-			/* We've successfully sent whatever data was in that packet. */
-			bytes_sent += PqGSSSendConsumed;
-
-			/* All encrypted data was sent, our buffer is empty now. */
-			PqGSSSendLength = PqGSSSendNext = PqGSSSendConsumed = 0;
+			/* We've successfully sent whatever data was in the buffer. */
+			PqGSSSendLength = PqGSSSendNext = 0;
 		}
 
 		/*
@@ -192,7 +185,7 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
 
 		/*
 		 * Create the next encrypted packet.  Any failure here is considered a
-		 * hard failure, so we return -1 even if bytes_sent > 0.
+		 * hard failure, so we return -1 even if some data has been sent.
 		 */
 		major = gss_wrap(&minor, gctx, 1, GSS_C_QOP_DEFAULT,
 						 &input, &conf_state, &output);
@@ -236,10 +229,13 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
 	}
 
 	/* If we get here, our counters should all match up. */
-	Assert(bytes_sent == len);
-	Assert(bytes_sent == bytes_encrypted);
+	Assert(len == PqGSSSendConsumed);
+	Assert(len == bytes_encrypted);
+
+	/* We're reporting all the data as sent, so reset PqGSSSendConsumed. */
+	PqGSSSendConsumed = 0;
 
-	ret = bytes_sent;
+	ret = bytes_encrypted;
 
 cleanup:
 	/* Release GSSAPI buffer storage, if we didn't already */
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
@@ -583,8 +583,8 @@ struct pg_conn
 	int			gss_SendLength; /* End of data available in gss_SendBuffer */
 	int			gss_SendNext;	/* Next index to send a byte from
 								 * gss_SendBuffer */
-	int			gss_SendConsumed;	/* Number of *unencrypted* bytes consumed
-									 * for current contents of gss_SendBuffer */
+	int			gss_SendConsumed;	/* Number of source bytes encrypted but
+									 * not yet reported as sent */
 	char	   *gss_RecvBuffer; /* Received, encrypted data */
 	int			gss_RecvLength; /* End of data available in gss_RecvBuffer */
 	char	   *gss_ResultBuffer;	/* Decryption of data in gss_RecvBuffer */
