Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 7eaf654

Browse files
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 d7da3ef commit 7eaf654

File tree

3 files changed

+25
-14
lines changed

3 files changed

+25
-14
lines changed

src/backend/libpq/auth.c

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1197,6 +1197,7 @@ pg_GSS_checkauth(Port *port)
11971197
min_stat,
11981198
lmin_s;
11991199
gss_buffer_desc gbuf;
1200+
char *princ;
12001201

12011202
/*
12021203
* Get the name of the user that authenticated, and compare it to the pg
@@ -1210,18 +1211,27 @@ pg_GSS_checkauth(Port *port)
12101211
return STATUS_ERROR;
12111212
}
12121213

1214+
/*
1215+
* gbuf.value might not be null-terminated, so turn it into a regular
1216+
* null-terminated string.
1217+
*/
1218+
princ = palloc(gbuf.length + 1);
1219+
memcpy(princ, gbuf.value, gbuf.length);
1220+
princ[gbuf.length] = '\0';
1221+
gss_release_buffer(&lmin_s, &gbuf);
1222+
12131223
/*
12141224
* Copy the original name of the authenticated principal into our backend
12151225
* memory for display later.
12161226
*/
1217-
port->gss->princ = MemoryContextStrdup(TopMemoryContext, gbuf.value);
1227+
port->gss->princ = MemoryContextStrdup(TopMemoryContext, princ);
12181228

12191229
/*
12201230
* Split the username at the realm separator
12211231
*/
1222-
if (strchr(gbuf.value, '@'))
1232+
if (strchr(princ, '@'))
12231233
{
1224-
char *cp = strchr(gbuf.value, '@');
1234+
char *cp = strchr(princ, '@');
12251235

12261236
/*
12271237
* If we are not going to include the realm in the username that is
@@ -1248,7 +1258,7 @@ pg_GSS_checkauth(Port *port)
12481258
elog(DEBUG2,
12491259
"GSSAPI realm (%s) and configured realm (%s) don't match",
12501260
cp, port->hba->krb_realm);
1251-
gss_release_buffer(&lmin_s, &gbuf);
1261+
pfree(princ);
12521262
return STATUS_ERROR;
12531263
}
12541264
}
@@ -1257,15 +1267,14 @@ pg_GSS_checkauth(Port *port)
12571267
{
12581268
elog(DEBUG2,
12591269
"GSSAPI did not return realm but realm matching was requested");
1260-
1261-
gss_release_buffer(&lmin_s, &gbuf);
1270+
pfree(princ);
12621271
return STATUS_ERROR;
12631272
}
12641273

1265-
ret = check_usermap(port->hba->usermap, port->user_name, gbuf.value,
1274+
ret = check_usermap(port->hba->usermap, port->user_name, princ,
12661275
pg_krb_caseins_users);
12671276

1268-
gss_release_buffer(&lmin_s, &gbuf);
1277+
pfree(princ);
12691278

12701279
return ret;
12711280
}

src/backend/libpq/be-gssapi-common.c

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,6 @@ pg_GSS_error_int(char *s, size_t len, OM_uint32 stat, int type)
2929
OM_uint32 lmin_s,
3030
msg_ctx = 0;
3131

32-
s[0] = '\0'; /* just in case gss_display_status fails */
33-
3432
do
3533
{
3634
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)
4341
i++;
4442
}
4543
if (i < len)
46-
strlcpy(s + i, gmsg.value, len - i);
44+
memcpy(s + i, gmsg.value, Min(len - i, gmsg.length));
4745
i += gmsg.length;
4846
gss_release_buffer(&lmin_s, &gmsg);
4947
}
5048
while (msg_ctx);
5149

52-
if (i >= len)
50+
/* add nul termination */
51+
if (i < len)
52+
s[i] = '\0';
53+
else
5354
{
5455
elog(COMMERROR, "incomplete GSS error report");
55-
s[len - 1] = '\0'; /* ensure string is nul-terminated */
56+
s[len - 1] = '\0';
5657
}
5758
}
5859

src/interfaces/libpq/fe-gssapi-common.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@ pg_GSS_error_int(PQExpBuffer str, OM_uint32 stat, int type)
3434
if (gss_display_status(&lmin_s, stat, type, GSS_C_NO_OID,
3535
&msg_ctx, &lmsg) != GSS_S_COMPLETE)
3636
break;
37-
appendPQExpBuffer(str, " %s", (char *) lmsg.value);
37+
appendPQExpBufferChar(str, ' ');
38+
appendBinaryPQExpBuffer(str, lmsg.value, lmsg.length);
3839
gss_release_buffer(&lmin_s, &lmsg);
3940
} while (msg_ctx);
4041
}

0 commit comments

Comments
 (0)