]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Fix timing-dependent failure in GSSAPI data transmission.
authorTom Lane <tgl@sss.pgh.pa.us>
Thu, 23 Nov 2023 18:30:19 +0000 (13:30 -0500)
committerTom Lane <tgl@sss.pgh.pa.us>
Thu, 23 Nov 2023 18:30:19 +0000 (13:30 -0500)
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

src/backend/libpq/be-secure-gssapi.c
src/interfaces/libpq/fe-secure-gssapi.c
src/interfaces/libpq/libpq-int.h

index 211634fd5b7582269543ebd70425280a9996e713..80c673b6ba6a72b627dd4517332b4d25fb309f44 100644 (file)
@@ -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,19 +98,25 @@ 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)
        {
@@ -118,6 +124,7 @@ be_gssapi_write(Port *port, void *ptr, size_t len)
                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;
 }
 
 /*
index aef201b99cda371b34e6873af48a6ef85189ecbb..1b750d69447780a1c8f5bf8f7f48ef0670ac77b5 100644 (file)
@@ -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 put into 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)
        {
@@ -135,38 +141,25 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
                 */
                if (PqGSSSendLength)
                {
-                       ssize_t         ret;
+                       ssize_t         retval;
                        ssize_t         amount = PqGSSSendLength - PqGSSSendNext;
 
-                       ret = pqsecure_raw_write(conn, PqGSSSendBuffer + PqGSSSendNext, amount);
-                       if (ret <= 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 ret;
-                       }
+                       retval = pqsecure_raw_write(conn, PqGSSSendBuffer + PqGSSSendNext, amount);
+                       if (retval <= 0)
+                               return retval;
 
                        /*
                         * Check if this was a partial write, and if so, move forward that
                         * far in our buffer and try again.
                         */
-                       if (ret != amount)
+                       if (retval < amount)
                        {
-                               PqGSSSendNext += ret;
+                               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);
@@ -238,10 +231,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 */
index a443418699f8e616df292d2f822b085b54f9b288..7ecbd55224f2bebc575986c93642aeb8a710e2bf 100644 (file)
@@ -495,8 +495,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 */