Don't assume GSSAPI result strings are null-terminated.

Our uses of gss_display_status() and gss_display_name() assumed
that the gss_buffer_desc strings returned by those functions are
null-terminated.  It appears that they generally are, given the
lack of field complaints up to now.  However, the available
documentation does not promise this, and some man pages
for gss_display_status() show examples that rely on the
gss_buffer_desc.length field instead of expecting null
termination.  Also, we now have a report that on some
implementations, clang's address sanitizer is of the opinion
that the byte after the specified length is undefined.

Hence, change the code to rely on the length field instead.

This might well be cosmetic rather than fixing any real bug, but
it's hard to be sure, so back-patch to all supported branches.
While here, also back-patch the v12 changes that made pg_GSS_error
deal honestly with multiple messages available from
gss_display_status.

Per report from Sudheer H R.

Discussion: https://postgr.es/m/5372B6D4-8276-42C0-B8FB-BD0918826FC3@tekenlight.com

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 852571bad6a5c962945bfdd4b2575529fc21c990..3af1cc021e3042c4bd70f03503aa43934bc5a44e 100644 (file)
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -1039,44 +1039,67 @@ static GSS_DLLIMP gss_OID GSS_C_NT_USER_NAME = &GSS_C_NT_USER_NAME_desc;
 
 
 /*
- * Generate an error for GSSAPI authentication.  The caller should apply
- * _() to errmsg to make it translatable.
+ * Fetch all errors of a specific type and append to "s" (buffer of size len).
+ * If we obtain more than one string, separate them with spaces.
+ * Call once for GSS_CODE and once for MECH_CODE.
  */
 static void
-pg_GSS_error(int severity, const char *errmsg, OM_uint32 maj_stat, OM_uint32 min_stat)
+pg_GSS_error_int(char *s, size_t len, OM_uint32 stat, int type)
 {
        gss_buffer_desc gmsg;
+       size_t          i = 0;
        OM_uint32       lmin_s,
-                               msg_ctx;
+                               msg_ctx = 0;
+
+       do
+       {
+               if (gss_display_status(&lmin_s, stat, type, GSS_C_NO_OID,
+                                                          &msg_ctx, &gmsg) != GSS_S_COMPLETE)
+                       break;
+               if (i > 0)
+               {
+                       if (i < len)
+                               s[i] = ' ';
+                       i++;
+               }
+               if (i < len)
+                       memcpy(s + i, gmsg.value, Min(len - i, gmsg.length));
+               i += gmsg.length;
+               gss_release_buffer(&lmin_s, &gmsg);
+       }
+       while (msg_ctx);
+
+       /* add nul termination */
+       if (i < len)
+               s[i] = '\0';
+       else
+       {
+               elog(COMMERROR, "incomplete GSS error report");
+               s[len - 1] = '\0';
+       }
+}
+
+/*
+ * Report the GSSAPI error described by maj_stat/min_stat.
+ *
+ * errmsg should be an already-translated primary error message.
+ * The GSSAPI info is appended as errdetail.
+ *
+ * To avoid memory allocation, total error size is capped (at 128 bytes for
+ * each of major and minor).  No known mechanisms will produce error messages
+ * beyond this cap.
+ */
+static void
+pg_GSS_error(int severity, const char *errmsg, OM_uint32 maj_stat, OM_uint32 min_stat)
+{
        char            msg_major[128],
                                msg_minor[128];
 
        /* Fetch major status message */
-       msg_ctx = 0;
-       gss_display_status(&lmin_s, maj_stat, GSS_C_GSS_CODE,
-                                          GSS_C_NO_OID, &msg_ctx, &gmsg);
-       strlcpy(msg_major, gmsg.value, sizeof(msg_major));
-       gss_release_buffer(&lmin_s, &gmsg);
-
-       if (msg_ctx)
-
-               /*
-                * More than one message available. XXX: Should we loop and read all
-                * messages? (same below)
-                */
-               ereport(WARNING,
-                               (errmsg_internal("incomplete GSS error report")));
+       pg_GSS_error_int(msg_major, sizeof(msg_major), maj_stat, GSS_C_GSS_CODE);
 
        /* Fetch mechanism minor status message */
-       msg_ctx = 0;
-       gss_display_status(&lmin_s, min_stat, GSS_C_MECH_CODE,
-                                          GSS_C_NO_OID, &msg_ctx, &gmsg);
-       strlcpy(msg_minor, gmsg.value, sizeof(msg_minor));
-       gss_release_buffer(&lmin_s, &gmsg);
-
-       if (msg_ctx)
-               ereport(WARNING,
-                               (errmsg_internal("incomplete GSS minor error report")));
+       pg_GSS_error_int(msg_minor, sizeof(msg_minor), min_stat, GSS_C_MECH_CODE);
 
        /*
         * errmsg_internal, since translation of the first part must be done
@@ -1098,6 +1121,7 @@ pg_GSS_recvauth(Port *port)
        int                     ret;
        StringInfoData buf;
        gss_buffer_desc gbuf;
+       char       *princ;
 
        /*
         * GSS auth is not supported for protocol versions before 3, because it
@@ -1261,12 +1285,21 @@ pg_GSS_recvauth(Port *port)
                                         _("retrieving GSS user name failed"),
                                         maj_stat, min_stat);
 
+       /*
+        * gbuf.value might not be null-terminated, so turn it into a regular
+        * null-terminated string.
+        */
+       princ = palloc(gbuf.length + 1);
+       memcpy(princ, gbuf.value, gbuf.length);
+       princ[gbuf.length] = '\0';
+       gss_release_buffer(&lmin_s, &gbuf);
+
        /*
         * Split the username at the realm separator
         */
-       if (strchr(gbuf.value, '@'))
+       if (strchr(princ, '@'))
        {
-               char       *cp = strchr(gbuf.value, '@');
+               char       *cp = strchr(princ, '@');
 
                /*
                 * If we are not going to include the realm in the username that is
@@ -1293,7 +1326,7 @@ pg_GSS_recvauth(Port *port)
                                elog(DEBUG2,
                                         "GSSAPI realm (%s) and configured realm (%s) don't match",
                                         cp, port->hba->krb_realm);
-                               gss_release_buffer(&lmin_s, &gbuf);
+                               pfree(princ);
                                return STATUS_ERROR;
                        }
                }
@@ -1302,15 +1335,14 @@ pg_GSS_recvauth(Port *port)
        {
                elog(DEBUG2,
                         "GSSAPI did not return realm but realm matching was requested");
-
-               gss_release_buffer(&lmin_s, &gbuf);
+               pfree(princ);
                return STATUS_ERROR;
        }
 
-       ret = check_usermap(port->hba->usermap, port->user_name, gbuf.value,
+       ret = check_usermap(port->hba->usermap, port->user_name, princ,
                                                pg_krb_caseins_users);
 
-       gss_release_buffer(&lmin_s, &gbuf);
+       pfree(princ);
 
        return ret;
 }

diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index b510bce668d0552401a28f7e79b9c5cf286b5897..61cb45059225a5ff975f12ec16aabe87ac765011 100644 (file)
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -75,7 +75,9 @@ pg_GSS_error_int(PQExpBuffer str, const char *mprefix,
        {
                gss_display_status(&lmin_s, stat, type,
                                                   GSS_C_NO_OID, &msg_ctx, &lmsg);
-               appendPQExpBuffer(str, "%s: %s\n", mprefix, (char *) lmsg.value);
+               appendPQExpBuffer(str, "%s: ", mprefix);
+               appendBinaryPQExpBuffer(str, lmsg.value, lmsg.length);
+               appendPQExpBufferChar(str, '\n');
                gss_release_buffer(&lmin_s, &lmsg);
        } while (msg_ctx);
 }