Thread: psql with GSS can crash
Hi all,
I got following stack:
fffffd7ffed14b70 strlen () + 40 fffffd7ffed71665 snprintf () + e5 fffffd7fff36d088 pg_GSS_startup () + 88
fffffd7fff36d43apg_fe_sendauth () + 15a fffffd7fff36e557 PQconnectPoll () + 3b7 fffffd7fff36e152 connectDBComplete () +
a2fffffd7fff36dc32 PQsetdbLogin () + 1b2 000000000041e96d main () + 30d 000000000041302c ???????? ()
It seems that connection is not fully configured and krbsrvname or
pghost is not filled. Following code in fe-auth.c pg_GSS_startup()
causes a crash:
440 maxlen = NI_MAXHOST + strlen(conn->krbsrvname) + 2; 441 temp_gbuf.value = (char *) malloc(maxlen);
442 snprintf(temp_gbuf.value, maxlen, "%s@%s", 443 conn->krbsrvname, conn->pghost); 444
temp_gbuf.length= strlen(temp_gbuf.value);
And following code in fe-connect.c fillPGconn() fill NULL value.
571 tmp = conninfo_getval(connOptions, "krbsrvname"); 572 conn->krbsrvname = tmp ? strdup(tmp) : NULL;
I think that pg_GSS_startup should sanity the input.
Zdenek
On Thu, Feb 25, 2010 at 15:04, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote: > Hi all, > > I got following stack: > > fffffd7ffed14b70 strlen () + 40 > fffffd7ffed71665 snprintf () + e5 > fffffd7fff36d088 pg_GSS_startup () + 88 > fffffd7fff36d43a pg_fe_sendauth () + 15a > fffffd7fff36e557 PQconnectPoll () + 3b7 > fffffd7fff36e152 connectDBComplete () + a2 > fffffd7fff36dc32 PQsetdbLogin () + 1b2 > 000000000041e96d main () + 30d > 000000000041302c ???????? () > > It seems that connection is not fully configured and krbsrvname or pghost is > not filled. Following code in fe-auth.c pg_GSS_startup() causes a crash: > > 440 maxlen = NI_MAXHOST + strlen(conn->krbsrvname) + 2; > 441 temp_gbuf.value = (char *) malloc(maxlen); > 442 snprintf(temp_gbuf.value, maxlen, "%s@%s", > 443 conn->krbsrvname, conn->pghost); > 444 temp_gbuf.length = strlen(temp_gbuf.value); > > And following code in fe-connect.c fillPGconn() fill NULL value. > > 571 tmp = conninfo_getval(connOptions, "krbsrvname"); > 572 conn->krbsrvname = tmp ? strdup(tmp) : NULL; > > I think that pg_GSS_startup should sanity the input. How did you get NULL in there? :-) There's a default set for that one that's PG_KRB_SRVNAM, so it really should never come out as NULL, I think... As for pghost, that certainly seems to be a bug. We check that one in krb5 and SSPI, but for some reason we seem to be missing it in GSSAPI. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander píše v čt 25. 02. 2010 v 15:17 +0100: > On Thu, Feb 25, 2010 at 15:04, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote: > > Hi all, > > > > I got following stack: > > > > fffffd7ffed14b70 strlen () + 40 > > fffffd7ffed71665 snprintf () + e5 > > fffffd7fff36d088 pg_GSS_startup () + 88 > > fffffd7fff36d43a pg_fe_sendauth () + 15a > > fffffd7fff36e557 PQconnectPoll () + 3b7 > > fffffd7fff36e152 connectDBComplete () + a2 > > fffffd7fff36dc32 PQsetdbLogin () + 1b2 > > 000000000041e96d main () + 30d > > 000000000041302c ???????? () > > > > It seems that connection is not fully configured and krbsrvname or pghost is > > not filled. Following code in fe-auth.c pg_GSS_startup() causes a crash: > > > > 440 maxlen = NI_MAXHOST + strlen(conn->krbsrvname) + 2; > > 441 temp_gbuf.value = (char *) malloc(maxlen); > > 442 snprintf(temp_gbuf.value, maxlen, "%s@%s", > > 443 conn->krbsrvname, conn->pghost); > > 444 temp_gbuf.length = strlen(temp_gbuf.value); > > > > And following code in fe-connect.c fillPGconn() fill NULL value. > > > > 571 tmp = conninfo_getval(connOptions, "krbsrvname"); > > 572 conn->krbsrvname = tmp ? strdup(tmp) : NULL; > > > > I think that pg_GSS_startup should sanity the input. > > How did you get NULL in there? :-) > There's a default set for that one that's PG_KRB_SRVNAM, so it really > should never come out as NULL, I think... Yeah, you are right. conn->krbsrvname is "postgres" and conn->pghost is null > As for pghost, that certainly seems to be a bug. We check that one in > krb5 and SSPI, but for some reason we seem to be missing it in GSSAPI. Yes. The check should be in GSSAPI too. However what I see in pg_hba.conf is following line: local all all gss Gss is used on local unix socket which probably cause a problem that conn->pghost is not filled when psql tries to connect. thanks Zdenek Zdenek
2010/3/1 Zdenek Kotala <Zdenek.Kotala@sun.com>: > Magnus Hagander píše v čt 25. 02. 2010 v 15:17 +0100: >> On Thu, Feb 25, 2010 at 15:04, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote: >> > Hi all, >> > >> > I got following stack: >> > >> > fffffd7ffed14b70 strlen () + 40 >> > fffffd7ffed71665 snprintf () + e5 >> > fffffd7fff36d088 pg_GSS_startup () + 88 >> > fffffd7fff36d43a pg_fe_sendauth () + 15a >> > fffffd7fff36e557 PQconnectPoll () + 3b7 >> > fffffd7fff36e152 connectDBComplete () + a2 >> > fffffd7fff36dc32 PQsetdbLogin () + 1b2 >> > 000000000041e96d main () + 30d >> > 000000000041302c ???????? () >> > >> > It seems that connection is not fully configured and krbsrvname or pghost is >> > not filled. Following code in fe-auth.c pg_GSS_startup() causes a crash: >> > >> > 440 maxlen = NI_MAXHOST + strlen(conn->krbsrvname) + 2; >> > 441 temp_gbuf.value = (char *) malloc(maxlen); >> > 442 snprintf(temp_gbuf.value, maxlen, "%s@%s", >> > 443 conn->krbsrvname, conn->pghost); >> > 444 temp_gbuf.length = strlen(temp_gbuf.value); >> > >> > And following code in fe-connect.c fillPGconn() fill NULL value. >> > >> > 571 tmp = conninfo_getval(connOptions, "krbsrvname"); >> > 572 conn->krbsrvname = tmp ? strdup(tmp) : NULL; >> > >> > I think that pg_GSS_startup should sanity the input. >> >> How did you get NULL in there? :-) >> There's a default set for that one that's PG_KRB_SRVNAM, so it really >> should never come out as NULL, I think... > > Yeah, you are right. conn->krbsrvname is "postgres" and conn->pghost is > null Ah, good. We should defentd against that then. >> As for pghost, that certainly seems to be a bug. We check that one in >> krb5 and SSPI, but for some reason we seem to be missing it in GSSAPI. > > Yes. The check should be in GSSAPI too. > > However what I see in pg_hba.conf is following line: > > local all all gss > > Gss is used on local unix socket which probably cause a problem that > conn->pghost is not filled when psql tries to connect. So there are really two errors - because we should disallow that. See attached patch - can you confirm it removes the crash with just the client side applied, and then that it properly rejects GSS with the server side applied as well? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Attachment
Magnus Hagander píše v po 01. 03. 2010 v 16:55 +0100: > 2010/3/1 Zdenek Kotala <Zdenek.Kotala@sun.com>: > > Magnus Hagander píše v čt 25. 02. 2010 v 15:17 +0100: > >> On Thu, Feb 25, 2010 at 15:04, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote: > >> > Hi all, > >> > > >> > I got following stack: > >> > > >> > fffffd7ffed14b70 strlen () + 40 > >> > fffffd7ffed71665 snprintf () + e5 > >> > fffffd7fff36d088 pg_GSS_startup () + 88 > >> > fffffd7fff36d43a pg_fe_sendauth () + 15a > >> > fffffd7fff36e557 PQconnectPoll () + 3b7 > >> > fffffd7fff36e152 connectDBComplete () + a2 > >> > fffffd7fff36dc32 PQsetdbLogin () + 1b2 > >> > 000000000041e96d main () + 30d > >> > 000000000041302c ???????? () > >> > > >> > It seems that connection is not fully configured and krbsrvname or pghost is > >> > not filled. Following code in fe-auth.c pg_GSS_startup() causes a crash: > >> > > >> > 440 maxlen = NI_MAXHOST + strlen(conn->krbsrvname) + 2; > >> > 441 temp_gbuf.value = (char *) malloc(maxlen); > >> > 442 snprintf(temp_gbuf.value, maxlen, "%s@%s", > >> > 443 conn->krbsrvname, conn->pghost); > >> > 444 temp_gbuf.length = strlen(temp_gbuf.value); > >> > > >> > And following code in fe-connect.c fillPGconn() fill NULL value. > >> > > >> > 571 tmp = conninfo_getval(connOptions, "krbsrvname"); > >> > 572 conn->krbsrvname = tmp ? strdup(tmp) : NULL; > >> > > >> > I think that pg_GSS_startup should sanity the input. > >> > >> How did you get NULL in there? :-) > >> There's a default set for that one that's PG_KRB_SRVNAM, so it really > >> should never come out as NULL, I think... > > > > Yeah, you are right. conn->krbsrvname is "postgres" and conn->pghost is > > null > > Ah, good. We should defentd against that then. > > > >> As for pghost, that certainly seems to be a bug. We check that one in > >> krb5 and SSPI, but for some reason we seem to be missing it in GSSAPI. > > > > Yes. The check should be in GSSAPI too. > > > > However what I see in pg_hba.conf is following line: > > > > local all all gss > > > > Gss is used on local unix socket which probably cause a problem that > > conn->pghost is not filled when psql tries to connect. > > So there are really two errors - because we should disallow that. > > See attached patch - can you confirm it removes the crash with just > the client side applied, and then that it properly rejects GSS with > the server side applied as well? I tested it, but I cannot reproduce crash because I cannot setup illegal combination now ;-). I think it is OK. Thanks Zdenek
2010/3/7 Zdenek Kotala <Zdenek.Kotala@sun.com>: > Magnus Hagander píše v po 01. 03. 2010 v 16:55 +0100: >> 2010/3/1 Zdenek Kotala <Zdenek.Kotala@sun.com>: >> > Magnus Hagander píše v čt 25. 02. 2010 v 15:17 +0100: >> >> On Thu, Feb 25, 2010 at 15:04, Zdenek Kotala <Zdenek.Kotala@sun.com> wrote: >> >> > Hi all, >> >> > >> >> > I got following stack: >> >> > >> >> > fffffd7ffed14b70 strlen () + 40 >> >> > fffffd7ffed71665 snprintf () + e5 >> >> > fffffd7fff36d088 pg_GSS_startup () + 88 >> >> > fffffd7fff36d43a pg_fe_sendauth () + 15a >> >> > fffffd7fff36e557 PQconnectPoll () + 3b7 >> >> > fffffd7fff36e152 connectDBComplete () + a2 >> >> > fffffd7fff36dc32 PQsetdbLogin () + 1b2 >> >> > 000000000041e96d main () + 30d >> >> > 000000000041302c ???????? () >> >> > >> >> > It seems that connection is not fully configured and krbsrvname or pghost is >> >> > not filled. Following code in fe-auth.c pg_GSS_startup() causes a crash: >> >> > >> >> > 440 maxlen = NI_MAXHOST + strlen(conn->krbsrvname) + 2; >> >> > 441 temp_gbuf.value = (char *) malloc(maxlen); >> >> > 442 snprintf(temp_gbuf.value, maxlen, "%s@%s", >> >> > 443 conn->krbsrvname, conn->pghost); >> >> > 444 temp_gbuf.length = strlen(temp_gbuf.value); >> >> > >> >> > And following code in fe-connect.c fillPGconn() fill NULL value. >> >> > >> >> > 571 tmp = conninfo_getval(connOptions, "krbsrvname"); >> >> > 572 conn->krbsrvname = tmp ? strdup(tmp) : NULL; >> >> > >> >> > I think that pg_GSS_startup should sanity the input. >> >> >> >> How did you get NULL in there? :-) >> >> There's a default set for that one that's PG_KRB_SRVNAM, so it really >> >> should never come out as NULL, I think... >> > >> > Yeah, you are right. conn->krbsrvname is "postgres" and conn->pghost is >> > null >> >> Ah, good. We should defentd against that then. >> >> >> >> As for pghost, that certainly seems to be a bug. We check that one in >> >> krb5 and SSPI, but for some reason we seem to be missing it in GSSAPI. >> > >> > Yes. The check should be in GSSAPI too. >> > >> > However what I see in pg_hba.conf is following line: >> > >> > local all all gss >> > >> > Gss is used on local unix socket which probably cause a problem that >> > conn->pghost is not filled when psql tries to connect. >> >> So there are really two errors - because we should disallow that. >> >> See attached patch - can you confirm it removes the crash with just >> the client side applied, and then that it properly rejects GSS with >> the server side applied as well? > > I tested it, but I cannot reproduce crash because I cannot setup illegal > combination now ;-). I think it is OK. Ok, thanks for testing. I've been unable to break it in my testing as well so - applied, and back-patched. -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/