Skip to content

Commit 361acef

Browse files
tglsfdctglsfdc
committed
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
1 parent b1aa0f2 commit 361acef

File tree

2 files changed

+69
-35
lines changed

2 files changed

+69
-35
lines changed

‎src/backend/libpq/auth.c

Lines changed: 66 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1039,44 +1039,67 @@ static GSS_DLLIMP gss_OID GSS_C_NT_USER_NAME = &GSS_C_NT_USER_NAME_desc;
10391039

10401040

10411041
/*
1042-
* Generate an error for GSSAPI authentication. The caller should apply
1043-
* _() to errmsg to make it translatable.
1042+
* Fetch all errors of a specific type and append to "s" (buffer of size len).
1043+
* If we obtain more than one string, separate them with spaces.
1044+
* Call once for GSS_CODE and once for MECH_CODE.
10441045
*/
10451046
static void
1046-
pg_GSS_error(int severity, const char *errmsg, OM_uint32 maj_stat, OM_uint32 min_stat)
1047+
pg_GSS_error_int(char *s, size_t len, OM_uint32 stat, int type)
10471048
{
10481049
gss_buffer_desc gmsg;
1050+
size_t i = 0;
10491051
OM_uint32 lmin_s,
1050-
msg_ctx;
1052+
msg_ctx = 0;
1053+
1054+
do
1055+
{
1056+
if (gss_display_status(&lmin_s, stat, type, GSS_C_NO_OID,
1057+
&msg_ctx, &gmsg) != GSS_S_COMPLETE)
1058+
break;
1059+
if (i > 0)
1060+
{
1061+
if (i < len)
1062+
s[i] = ' ';
1063+
i++;
1064+
}
1065+
if (i < len)
1066+
memcpy(s + i, gmsg.value, Min(len - i, gmsg.length));
1067+
i += gmsg.length;
1068+
gss_release_buffer(&lmin_s, &gmsg);
1069+
}
1070+
while (msg_ctx);
1071+
1072+
/* add nul termination */
1073+
if (i < len)
1074+
s[i] = '\0';
1075+
else
1076+
{
1077+
elog(COMMERROR, "incomplete GSS error report");
1078+
s[len - 1] = '\0';
1079+
}
1080+
}
1081+
1082+
/*
1083+
* Report the GSSAPI error described by maj_stat/min_stat.
1084+
*
1085+
* errmsg should be an already-translated primary error message.
1086+
* The GSSAPI info is appended as errdetail.
1087+
*
1088+
* To avoid memory allocation, total error size is capped (at 128 bytes for
1089+
* each of major and minor). No known mechanisms will produce error messages
1090+
* beyond this cap.
1091+
*/
1092+
static void
1093+
pg_GSS_error(int severity, const char *errmsg, OM_uint32 maj_stat, OM_uint32 min_stat)
1094+
{
10511095
char msg_major[128],
10521096
msg_minor[128];
10531097

10541098
/* Fetch major status message */
1055-
msg_ctx = 0;
1056-
gss_display_status(&lmin_s, maj_stat, GSS_C_GSS_CODE,
1057-
GSS_C_NO_OID, &msg_ctx, &gmsg);
1058-
strlcpy(msg_major, gmsg.value, sizeof(msg_major));
1059-
gss_release_buffer(&lmin_s, &gmsg);
1060-
1061-
if (msg_ctx)
1062-
1063-
/*
1064-
* More than one message available. XXX: Should we loop and read all
1065-
* messages? (same below)
1066-
*/
1067-
ereport(WARNING,
1068-
(errmsg_internal("incomplete GSS error report")));
1099+
pg_GSS_error_int(msg_major, sizeof(msg_major), maj_stat, GSS_C_GSS_CODE);
10691100

10701101
/* Fetch mechanism minor status message */
1071-
msg_ctx = 0;
1072-
gss_display_status(&lmin_s, min_stat, GSS_C_MECH_CODE,
1073-
GSS_C_NO_OID, &msg_ctx, &gmsg);
1074-
strlcpy(msg_minor, gmsg.value, sizeof(msg_minor));
1075-
gss_release_buffer(&lmin_s, &gmsg);
1076-
1077-
if (msg_ctx)
1078-
ereport(WARNING,
1079-
(errmsg_internal("incomplete GSS minor error report")));
1102+
pg_GSS_error_int(msg_minor, sizeof(msg_minor), min_stat, GSS_C_MECH_CODE);
10801103

10811104
/*
10821105
* errmsg_internal, since translation of the first part must be done
@@ -1098,6 +1121,7 @@ pg_GSS_recvauth(Port *port)
10981121
int ret;
10991122
StringInfoData buf;
11001123
gss_buffer_desc gbuf;
1124+
char *princ;
11011125

11021126
/*
11031127
* GSS auth is not supported for protocol versions before 3, because it
@@ -1261,12 +1285,21 @@ pg_GSS_recvauth(Port *port)
12611285
_("retrieving GSS user name failed"),
12621286
maj_stat, min_stat);
12631287

1288+
/*
1289+
* gbuf.value might not be null-terminated, so turn it into a regular
1290+
* null-terminated string.
1291+
*/
1292+
princ = palloc(gbuf.length + 1);
1293+
memcpy(princ, gbuf.value, gbuf.length);
1294+
princ[gbuf.length] = '\0';
1295+
gss_release_buffer(&lmin_s, &gbuf);
1296+
12641297
/*
12651298
* Split the username at the realm separator
12661299
*/
1267-
if (strchr(gbuf.value, '@'))
1300+
if (strchr(princ, '@'))
12681301
{
1269-
char *cp = strchr(gbuf.value, '@');
1302+
char *cp = strchr(princ, '@');
12701303

12711304
/*
12721305
* If we are not going to include the realm in the username that is
@@ -1293,7 +1326,7 @@ pg_GSS_recvauth(Port *port)
12931326
elog(DEBUG2,
12941327
"GSSAPI realm (%s) and configured realm (%s) don't match",
12951328
cp, port->hba->krb_realm);
1296-
gss_release_buffer(&lmin_s, &gbuf);
1329+
pfree(princ);
12971330
return STATUS_ERROR;
12981331
}
12991332
}
@@ -1302,15 +1335,14 @@ pg_GSS_recvauth(Port *port)
13021335
{
13031336
elog(DEBUG2,
13041337
"GSSAPI did not return realm but realm matching was requested");
1305-
1306-
gss_release_buffer(&lmin_s, &gbuf);
1338+
pfree(princ);
13071339
return STATUS_ERROR;
13081340
}
13091341

1310-
ret = check_usermap(port->hba->usermap, port->user_name, gbuf.value,
1342+
ret = check_usermap(port->hba->usermap, port->user_name, princ,
13111343
pg_krb_caseins_users);
13121344

1313-
gss_release_buffer(&lmin_s, &gbuf);
1345+
pfree(princ);
13141346

13151347
return ret;
13161348
}

‎src/interfaces/libpq/fe-auth.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,9 @@ pg_GSS_error_int(PQExpBuffer str, const char *mprefix,
7575
{
7676
gss_display_status(&lmin_s, stat, type,
7777
GSS_C_NO_OID, &msg_ctx, &lmsg);
78-
appendPQExpBuffer(str, "%s: %s\n", mprefix, (char *) lmsg.value);
78+
appendPQExpBuffer(str, "%s: ", mprefix);
79+
appendBinaryPQExpBuffer(str, lmsg.value, lmsg.length);
80+
appendPQExpBufferChar(str, '\n');
7981
gss_release_buffer(&lmin_s, &lmsg);
8082
} while (msg_ctx);
8183
}

0 commit comments

Comments
 (0)