8000 Don't assume GSSAPI result strings are null-terminated. · postgres/postgres@d3a845d · GitHub
[go: up one dir, main page]
Skip to content

Commit d3a845d

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 0a8929c commit d3a845d

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
@@ -1035,44 +1035,67 @@ static GSS_DLLIMP gss_OID GSS_C_NT_USER_NAME = &GSS_C_NT_USER_NAME_desc;
10351035

10361036

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

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

10661097
/* Fetch mechanism minor status message */
1067-
msg_ctx = 0;
1068-
gss_display_status(&lmin_s, min_stat, GSS_C_MECH_CODE,
1069-
GSS_C_NO_OID, &msg_ctx, &gmsg);
1070-
strlcpy(msg_minor, gmsg.value, sizeof(msg_minor));
1071-
gss_release_buffer(&lmin_s, &gmsg);
1072-
1073-
if (msg_ctx)
1074-
ereport(WARNING,
1075-
(errmsg_internal("incomplete GSS minor error report")));
1098+
pg_GSS_error_int(msg_minor, sizeof(msg_minor), min_stat, GSS_C_MECH_CODE);
10761099

10771100
/*
10781101
* errmsg_internal, since translation of the first part must be done
@@ -1094,6 +1117,7 @@ pg_GSS_recvauth(Port *port)
10941117
int ret;
10951118
StringInfoData buf;
10961119
gss_buffer_desc gbuf;
1120+
char *princ;
10971121

10981122
/*
10991123
* GSS auth is not supported for protocol versions before 3, because it
@@ -1257,12 +1281,21 @@ pg_GSS_recvauth(Port *port)
12571281
_("retrieving GSS user name failed"),
12581282
maj_stat, min_stat);
12591283

1284+
/*
1285+
* gbuf.value might not be null-terminated, so turn it into a regular
1286+
* null-terminated string.
1287+
*/
1288+
princ = palloc(gbuf.length + 1);
1289+
memcpy(princ, gbuf.value, gbuf.length);
1290+
princ[gbuf.length] = '\0';
1291+
gss_release_buffer(&lmin_s, &gbuf);
1292+
12601293
/*
12611294
* Split the username at the realm separator
12621295
*/
1263-
if (strchr(gbuf.value, '@'))
1296+
if (strchr(princ, '@'))
12641297
{
1265-
char *cp = strchr(gbuf.value, '@');
1298+
char *cp = strchr(princ, '@');
12661299

12671300
/*
12681301
* If we are not going to include the realm in the username that is
@@ -1289,7 +1322,7 @@ pg_GSS_recvauth(Port *port)
12891322
elog(DEBUG2,
12901323
"GSSAPI realm (%s) and configured realm (%s) don't match",
12911324
cp, port->hba->krb_realm);
1292-
gss_release_buffer(&lmin_s, &gbuf);
1325+
pfree(princ);
12931326
return STATUS_ERROR;
12941327
}
12951328
}
@@ -1298,15 +1331,14 @@ pg_GSS_recvauth(Port *port)
12981331
{
12991332
elog(DEBUG2,
13001333
"GSSAPI did not return realm but realm matching was requested");
1301-
1302-
gss_release_buffer(&lmin_s, &gbuf);
1334+
pfree(princ);
13031335
return STATUS_ERROR;
13041336
}
13051337

1306-
ret = check_usermap(port->hba->usermap, port->user_name, gbuf.value,
1338+
ret = check_usermap(port->hba->usermap, port->user_name, princ,
13071339
pg_krb_caseins_users);
13081340

1309-
gss_release_buffer(&lmin_s, &gbuf);
1341+
pfree(princ);
13101342

13111343
return ret;
13121344
}

‎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)
0