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

tglsfdc · tglsfdc · commit 126cdaf47af2 · 2021-06-23T14:01:32.000-04:00
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
@@ -1213,6 +1213,7 @@ pg_GSS_checkauth(Port *port)
 				min_stat,
 				lmin_s;
 	gss_buffer_desc gbuf;
+	char	   *princ;
 
 	/*
 	 * Get the name of the user that authenticated, and compare it to the pg
@@ -1226,6 +1227,15 @@ pg_GSS_checkauth(Port *port)
 		return STATUS_ERROR;
 	}
 
+	/*
+	 * 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);
+
 	/*
 	 * Copy the original name of the authenticated principal into our backend
 	 * memory for display later.
@@ -1234,15 +1244,15 @@ pg_GSS_checkauth(Port *port)
 	 * waiting for the usermap check below, because authentication has already
 	 * succeeded and we want the log file to reflect that.
 	 */
-	port->gss->princ = MemoryContextStrdup(TopMemoryContext, gbuf.value);
-	set_authn_id(port, gbuf.value);
+	port->gss->princ = MemoryContextStrdup(TopMemoryContext, princ);
+	set_authn_id(port, princ);
 
 	/*
 	 * 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
@@ -1269,7 +1279,7 @@ pg_GSS_checkauth(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;
 			}
 		}
@@ -1278,15 +1288,14 @@ pg_GSS_checkauth(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/backend/libpq/be-gssapi-common.c b/src/backend/libpq/be-gssapi-common.c
@@ -29,8 +29,6 @@ pg_GSS_error_int(char *s, size_t len, OM_uint32 stat, int type)
 	OM_uint32	lmin_s,
 				msg_ctx = 0;
 
-	s[0] = '\0';				/* just in case gss_display_status fails */
-
 	do
 	{
 		if (gss_display_status(&lmin_s, stat, type, GSS_C_NO_OID,
@@ -43,16 +41,19 @@ pg_GSS_error_int(char *s, size_t len, OM_uint32 stat, int type)
 			i++;
 		}
 		if (i < len)
-			strlcpy(s + i, gmsg.value, len - i);
+			memcpy(s + i, gmsg.value, Min(len - i, gmsg.length));
 		i += gmsg.length;
 		gss_release_buffer(&lmin_s, &gmsg);
 	}
 	while (msg_ctx);
 
-	if (i >= len)
+	/* add nul termination */
+	if (i < len)
+		s[i] = '\0';
+	else
 	{
 		elog(COMMERROR, "incomplete GSS error report");
-		s[len - 1] = '\0';		/* ensure string is nul-terminated */
+		s[len - 1] = '\0';
 	}
 }
 
diff --git a/src/interfaces/libpq/fe-gssapi-common.c b/src/interfaces/libpq/fe-gssapi-common.c
@@ -34,7 +34,8 @@ pg_GSS_error_int(PQExpBuffer str, OM_uint32 stat, int type)
 		if (gss_display_status(&lmin_s, stat, type, GSS_C_NO_OID,
 							   &msg_ctx, &lmsg) != GSS_S_COMPLETE)
 			break;
-		appendPQExpBuffer(str, " %s", (char *) lmsg.value);
+		appendPQExpBufferChar(str, ' ');
+		appendBinaryPQExpBuffer(str, lmsg.value, lmsg.length);
 		gss_release_buffer(&lmin_s, &lmsg);
 	} while (msg_ctx);
 }

Original file line number	Diff line number	Diff line change
`@@ -29,8 +29,6 @@ pg_GSS_error_int(char *s, size_t len, OM_uint32 stat, int type)`
`29`	`29`	`OM_uint32 lmin_s,`
`30`	`30`	`msg_ctx = 0;`
`31`	`31`
`32`		`- s[0] = '\0'; /* just in case gss_display_status fails */`
`33`		`-`
`34`	`32`	`do`
`35`	`33`	`{`
`36`	`34`	`if (gss_display_status(&lmin_s, stat, type, GSS_C_NO_OID,`
`@@ -43,16 +41,19 @@ pg_GSS_error_int(char *s, size_t len, OM_uint32 stat, int type)`
`43`	`41`	`i++;`
`44`	`42`	`}`
`45`	`43`	`if (i < len)`
`46`		`- strlcpy(s + i, gmsg.value, len - i);`
	`44`	`+ memcpy(s + i, gmsg.value, Min(len - i, gmsg.length));`
`47`	`45`	`i += gmsg.length;`
`48`	`46`	`gss_release_buffer(&lmin_s, &gmsg);`
`49`	`47`	`}`
`50`	`48`	`while (msg_ctx);`
`51`	`49`
`52`		`- if (i >= len)`
	`50`	`+ /* add nul termination */`
	`51`	`+ if (i < len)`
	`52`	`+ s[i] = '\0';`
	`53`	`+ else`
`53`	`54`	`{`
`54`	`55`	`elog(COMMERROR, "incomplete GSS error report");`
`55`		`- s[len - 1] = '\0'; /* ensure string is nul-terminated */`
	`56`	`+ s[len - 1] = '\0';`
`56`	`57`	`}`
`57`	`58`	`}`
`58`	`59`