Server ignores contents of SASLInitialResponse
Hi all,
I have noticed today that the server ignores completely the contents
of SASLInitialResponse. For example with the patch attached called
scram-trick-server:
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index f4397afc64..8fe1c8edfb 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -540,7 +540,7 @@ pg_SASL_init(PGconn *conn, int payloadlen)
conn->sasl_state = pg_fe_scram_init(conn->pguser, password);
if (!conn->sasl_state)
goto oom_error;
- selected_mechanism = SCRAM_SHA256_NAME;
+ selected_mechanism = "kunfoobar";
}
}This sends a custom string to the server to name a SASL mechanism,
about which the server complains with a COMMERROR log:
LOG: client selected an invalid SASL authentication mechanism
However this error is completely ignored and the server continues
authentication, succeeding if the password is right. It seems to me
that the error that should be returned to the user is a password
mismatch, and that the COMMERROR message is kept only for the server
logs. Attached is a patch to fix the problem.
Open item added as well.
Thanks,
--
Michael
Attachments:
scram-trick-server.patchapplication/octet-stream; name=scram-trick-server.patchDownload
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index f4397afc64..8fe1c8edfb 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -540,7 +540,7 @@ pg_SASL_init(PGconn *conn, int payloadlen)
conn->sasl_state = pg_fe_scram_init(conn->pguser, password);
if (!conn->sasl_state)
goto oom_error;
- selected_mechanism = SCRAM_SHA256_NAME;
+ selected_mechanism = "kunfoobar";
}
}
fix-sasl-init.patchapplication/octet-stream; name=fix-sasl-init.patchDownload
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index c895ba0c32..4a2926b6d9 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -934,9 +934,12 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
*/
selected_mech = pq_getmsgrawstring(&buf);
if (strcmp(selected_mech, SCRAM_SHA256_NAME) != 0)
+ {
ereport(COMMERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("client selected an invalid SASL authentication mechanism")));
+ return STATUS_ERROR;
+ }
inputlen = pq_getmsgint(&buf, 4);
if (inputlen == -1)
On 05/24/2017 11:33 PM, Michael Paquier wrote:
I have noticed today that the server ignores completely the contents
of SASLInitialResponse. ... Attached is a patch to fix the problem.
Fixed, thanks!
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, May 25, 2017 at 8:51 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 05/24/2017 11:33 PM, Michael Paquier wrote:
I have noticed today that the server ignores completely the contents
of SASLInitialResponse. ... Attached is a patch to fix the problem.Fixed, thanks!
Thanks for the commit.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, May 25, 2017 at 9:32 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Thu, May 25, 2017 at 8:51 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 05/24/2017 11:33 PM, Michael Paquier wrote:
I have noticed today that the server ignores completely the contents
of SASLInitialResponse. ... Attached is a patch to fix the problem.Fixed, thanks!
Thanks for the commit.
Actually, I don't think that we are completely done here. Using the
patch of upthread to enforce a failure on SASLInitialResponse, I see
that connecting without SSL causes the following error:
psql: FATAL: password authentication failed for user "mpaquier"
But connecting with SSL returns that:
psql: duplicate SASL authentication request
I have not looked at that in details yet, but it seems to me that we
should not take pg_SASL_init() twice in the scram authentication code
path in libpq for a single attempt.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, May 25, 2017 at 10:52 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
Actually, I don't think that we are completely done here. Using the
patch of upthread to enforce a failure on SASLInitialResponse, I see
that connecting without SSL causes the following error:
psql: FATAL: password authentication failed for user "mpaquier"
But connecting with SSL returns that:
psql: duplicate SASL authentication requestI have not looked at that in details yet, but it seems to me that we
should not take pg_SASL_init() twice in the scram authentication code
path in libpq for a single attempt.
Gotcha. This happens because of sslmode=prefer, on which
pqDropConnection is used to clean up the connection state. So it seems
to me that the correct fix is to move the cleanup of sasl_state to
pqDropConnection() instead of closePGconn(). Once I do so the failures
are correct, showing to the user two FATAL errors because of the two
attempts:
psql: FATAL: password authentication failed for user "mpaquier"
FATAL: password authentication failed for user "mpaquier"
--
Michael
Attachments:
libpq-sasl-cleanup.patchapplication/octet-stream; name=libpq-sasl-cleanup.patchDownload
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index f2c9bf7a88..0fab50ff07 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -413,6 +413,16 @@ pqDropConnection(PGconn *conn, bool flushInput)
/* Optionally discard any unread data */
if (flushInput)
conn->inStart = conn->inCursor = conn->inEnd = 0;
+ /* Discard authentication states */
+ if (conn->sasl_state)
+ {
+ /*
+ * XXX: if support for more authentication mechanisms is added, this
+ * needs to call the right 'free' function.
+ */
+ pg_fe_scram_free(conn->sasl_state);
+ conn->sasl_state = NULL;
+ }
/* Always discard any unsent data */
conn->outCount = 0;
}
@@ -3502,15 +3512,6 @@ closePGconn(PGconn *conn)
conn->sspictx = NULL;
}
#endif
- if (conn->sasl_state)
- {
- /*
- * XXX: if support for more authentication mechanisms is added, this
- * needs to call the right 'free' function.
- */
- pg_fe_scram_free(conn->sasl_state);
- conn->sasl_state = NULL;
- }
}
/*
On Thu, May 25, 2017 at 10:52:23AM -0400, Michael Paquier wrote:
On Thu, May 25, 2017 at 9:32 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Thu, May 25, 2017 at 8:51 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 05/24/2017 11:33 PM, Michael Paquier wrote:
I have noticed today that the server ignores completely the contents
of SASLInitialResponse. ... Attached is a patch to fix the problem.Fixed, thanks!
Thanks for the commit.
Actually, I don't think that we are completely done here. Using the
patch of upthread to enforce a failure on SASLInitialResponse, I see
that connecting without SSL causes the following error:
psql: FATAL: password authentication failed for user "mpaquier"
But connecting with SSL returns that:
psql: duplicate SASL authentication requestI have not looked at that in details yet, but it seems to me that we
should not take pg_SASL_init() twice in the scram authentication code
path in libpq for a single attempt.
[Action required within three days. This is a generic notification.]
The above-described topic is currently a PostgreSQL 10 open item. Heikki,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.
[1]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 05/25/2017 06:36 PM, Michael Paquier wrote:
On Thu, May 25, 2017 at 10:52 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:Actually, I don't think that we are completely done here. Using the
patch of upthread to enforce a failure on SASLInitialResponse, I see
that connecting without SSL causes the following error:
psql: FATAL: password authentication failed for user "mpaquier"
But connecting with SSL returns that:
psql: duplicate SASL authentication requestI have not looked at that in details yet, but it seems to me that we
should not take pg_SASL_init() twice in the scram authentication code
path in libpq for a single attempt.Gotcha. This happens because of sslmode=prefer, on which
pqDropConnection is used to clean up the connection state. So it seems
to me that the correct fix is to move the cleanup of sasl_state to
pqDropConnection() instead of closePGconn(). Once I do so the failures
are correct, showing to the user two FATAL errors because of the two
attempts:
psql: FATAL: password authentication failed for user "mpaquier"
FATAL: password authentication failed for user "mpaquier"
Hmm, the SASL state cleanup is done pretty much the same way that
GSS/SSPI cleanup is. Don't we have a similar problem with GSS?
I tested that, and I got an error from glibc, complaining about a
double-free:
*** Error in `/home/heikki/pgsql.master/bin/psql': double free or corruption (out): 0x000056157d13be00 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x70bcb)[0x7f6a460b7bcb]
/lib/x86_64-linux-gnu/libc.so.6(+0x76f96)[0x7f6a460bdf96]
/lib/x86_64-linux-gnu/libc.so.6(+0x7778e)[0x7f6a460be78e]
/home/heikki/pgsql.master/lib/libpq.so.5(+0xa1dc)[0x7f6a46d651dc]
/home/heikki/pgsql.master/lib/libpq.so.5(+0xa4b3)[0x7f6a46d654b3]
/home/heikki/pgsql.master/lib/libpq.so.5(+0xacb9)[0x7f6a46d65cb9]
/home/heikki/pgsql.master/lib/libpq.so.5(PQconnectPoll+0x14e7)[0x7f6a46d6ae81]
/home/heikki/pgsql.master/lib/libpq.so.5(+0xe895)[0x7f6a46d69895]
/home/heikki/pgsql.master/lib/libpq.so.5(PQconnectdbParams+0x4f)[0x7f6a46d675b9]
/home/heikki/pgsql.master/bin/psql(+0x3daa9)[0x56157ccafaa9]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf1)[0x7f6a460672b1]
/home/heikki/pgsql.master/bin/psql(+0x1163a)[0x56157cc8363a]
I bisected that; the culprit was commit 61bf96cab0, where I refactored
the libpq authentication code in preparation for SCRAM. The logic around
that free() was always a bit wonky, but the refactoring made it outright
broken. Attached is a patch for that, see commit message for details.
(Review of that would be welcome.)
So, after fixing that, back to the original question; don't we have a
similar "duplicate authentication request" problem with GSS? Yes, turns
out that we do, even on stable branches:
psql "sslmode=prefer dbname=postgres hostaddr=127.0.0.1
krbsrvname=postgres host=localhost user=krbtestuser"
psql: duplicate GSS authentication request
To fix, I suppose we can do what you did for SASL in your patch, and
move the cleanup of conn->gctx from closePGconn to pgDropConnection. And
I presume we need to do the same for the SSPI state too, but I don't
have a Windows set up to test that at the moment.
- Heikki
Attachments:
0001-Fix-double-free-bug-in-GSS-authentication.patchinvalid/octet-stream; name=0001-Fix-double-free-bug-in-GSS-authentication.patchDownload
From c75b69e777a52520f9688a8926b9d6986c88c8cc Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Thu, 1 Jun 2017 13:26:06 +0300
Subject: [PATCH 1/1] Fix double-free bug in GSS authentication.
The logic to free the buffer after the gss_init_sec_context() call was
always a bit wonky. Because gss_init_sec_context() sets the GSS context
variable, conn->gctx, we would in fact always attempt to free the buffer.
That only works, because previously conn->ginbuf.value was initialized
to NULL, and free(NULL) is a no-op. Commit 61bf96cab0 refactored things so
that the GSS input token buffer is allocated locally in pg_GSS_continue,
and not held in the PGconn object. After that, the now-local ginbuf.value
variable isn't initialized when it's not used, so we pass a bogus pointer
to free().
To fix, only try to free the input buffer if we allocated it. That was
the intention, certainly after the refactoring, and probably even before
that.
---
src/interfaces/libpq/fe-auth.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index f4397afc64..f30fb04809 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -133,6 +133,11 @@ pg_GSS_continue(PGconn *conn, int payloadlen)
return STATUS_ERROR;
}
}
+ else
+ {
+ ginbuf.length = 0;
+ ginbuf.value = NULL;
+ }
maj_stat = gss_init_sec_context(&min_stat,
GSS_C_NO_CREDENTIAL,
@@ -142,13 +147,13 @@ pg_GSS_continue(PGconn *conn, int payloadlen)
GSS_C_MUTUAL_FLAG,
0,
GSS_C_NO_CHANNEL_BINDINGS,
- (conn->gctx == GSS_C_NO_CONTEXT) ? GSS_C_NO_BUFFER : &ginbuf,
+ (ginbuf.value == NULL) ? GSS_C_NO_BUFFER : &ginbuf,
NULL,
&goutbuf,
NULL,
NULL);
- if (conn->gctx != GSS_C_NO_CONTEXT)
+ if (ginbuf.value)
free(ginbuf.value);
if (goutbuf.length != 0)
--
2.11.0
On Tue, May 30, 2017 at 03:04:47AM +0000, Noah Misch wrote:
On Thu, May 25, 2017 at 10:52:23AM -0400, Michael Paquier wrote:
On Thu, May 25, 2017 at 9:32 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Thu, May 25, 2017 at 8:51 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 05/24/2017 11:33 PM, Michael Paquier wrote:
I have noticed today that the server ignores completely the contents
of SASLInitialResponse. ... Attached is a patch to fix the problem.Fixed, thanks!
Thanks for the commit.
Actually, I don't think that we are completely done here. Using the
patch of upthread to enforce a failure on SASLInitialResponse, I see
that connecting without SSL causes the following error:
psql: FATAL: password authentication failed for user "mpaquier"
But connecting with SSL returns that:
psql: duplicate SASL authentication requestI have not looked at that in details yet, but it seems to me that we
should not take pg_SASL_init() twice in the scram authentication code
path in libpq for a single attempt.[Action required within three days. This is a generic notification.]
The above-described topic is currently a PostgreSQL 10 open item. Heikki,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.[1] /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com
This PostgreSQL 10 open item is past due for your status update. Kindly send
a status update within 24 hours, and include a date for your subsequent status
update. Refer to the policy on open item ownership:
/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jun 02, 2017 at 09:58:40PM -0700, Noah Misch wrote:
On Tue, May 30, 2017 at 03:04:47AM +0000, Noah Misch wrote:
On Thu, May 25, 2017 at 10:52:23AM -0400, Michael Paquier wrote:
On Thu, May 25, 2017 at 9:32 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Thu, May 25, 2017 at 8:51 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 05/24/2017 11:33 PM, Michael Paquier wrote:
I have noticed today that the server ignores completely the contents
of SASLInitialResponse. ... Attached is a patch to fix the problem.Fixed, thanks!
Thanks for the commit.
Actually, I don't think that we are completely done here. Using the
patch of upthread to enforce a failure on SASLInitialResponse, I see
that connecting without SSL causes the following error:
psql: FATAL: password authentication failed for user "mpaquier"
But connecting with SSL returns that:
psql: duplicate SASL authentication requestI have not looked at that in details yet, but it seems to me that we
should not take pg_SASL_init() twice in the scram authentication code
path in libpq for a single attempt.[Action required within three days. This is a generic notification.]
The above-described topic is currently a PostgreSQL 10 open item. Heikki,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.[1] /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com
This PostgreSQL 10 open item is past due for your status update. Kindly send
a status update within 24 hours, and include a date for your subsequent status
update. Refer to the policy on open item ownership:
/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com
IMMEDIATE ATTENTION REQUIRED. This PostgreSQL 10 open item is long past due
for your status update. Please reacquaint yourself with the policy on open
item ownership[1]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com and then reply immediately. If I do not hear from you by
2017-06-05 23:00 UTC, I will transfer this item to release management team
ownership without further notice.
[1]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 5, 2017 at 7:49 AM, Noah Misch <noah@leadboat.com> wrote:
IMMEDIATE ATTENTION REQUIRED. This PostgreSQL 10 open item is long past due
for your status update. Please reacquaint yourself with the policy on open
item ownership[1] and then reply immediately. If I do not hear from you by
2017-06-05 23:00 UTC, I will transfer this item to release management team
ownership without further notice.[1] /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com
I am planning to look and comment about Heikki's patch tomorrow my
time. The GSS issue requires a back-patch and is actually linked to
what is discussed for SASL. The testing behind it also needs some care
and attention.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 06/05/2017 09:34 AM, Michael Paquier wrote:
On Mon, Jun 5, 2017 at 7:49 AM, Noah Misch <noah@leadboat.com> wrote:
IMMEDIATE ATTENTION REQUIRED. This PostgreSQL 10 open item is long past due
for your status update. Please reacquaint yourself with the policy on open
item ownership[1] and then reply immediately. If I do not hear from you by
2017-06-05 23:00 UTC, I will transfer this item to release management team
ownership without further notice.[1] /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com
I am planning to look and comment about Heikki's patch tomorrow my
time. The GSS issue requires a back-patch and is actually linked to
what is discussed for SASL. The testing behind it also needs some care
and attention.
Thanks. I'll wait for Michael's update tomorrow, and will have a look at
this after that. Expect a new status update by Wednesday.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 1, 2017 at 4:58 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
I bisected that; the culprit was commit 61bf96cab0, where I refactored the
libpq authentication code in preparation for SCRAM. The logic around that
free() was always a bit wonky, but the refactoring made it outright broken.
Attached is a patch for that, see commit message for details. (Review of
that would be welcome.)
That looks fine to me.
So, after fixing that, back to the original question; don't we have a
similar "duplicate authentication request" problem with GSS? Yes, turns out
that we do, even on stable branches:psql "sslmode=prefer dbname=postgres hostaddr=127.0.0.1 krbsrvname=postgres
host=localhost user=krbtestuser"
psql: duplicate GSS authentication requestTo fix, I suppose we can do what you did for SASL in your patch, and move
the cleanup of conn->gctx from closePGconn to pgDropConnection. And I
presume we need to do the same for the SSPI state too, but I don't have a
Windows set up to test that at the moment.
SSPI does not complain with sslmode=prefer as each time
pg_SSPI_startup() is called conn->sspictx is enforced to NULL. This
looks wrong to me by the way as pg_SSPI_startup() is invoked only once
per authentication, and it leaks memory this way. That's also
inconsistent with SASL and GSS. At the same time this inconsistency is
not causing actual problems except a leak with SSPI in libpq, so not
doing anything except on HEAD looks fine to me.
--
Michael
Attachments:
libpq-auth-cleanup.patchapplication/octet-stream; name=libpq-auth-cleanup.patchDownload
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index f4397af..746b666 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -411,7 +411,12 @@ pg_SSPI_startup(PGconn *conn, int use_negotiate, int payloadlen)
TimeStamp expire;
char *host = PQhost(conn);
- conn->sspictx = NULL;
+ if (conn->sspictx)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("duplicate SSPI authentication request\n"));
+ return STATUS_ERROR;
+ }
/*
* Retrieve credentials handle
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index f2c9bf7..24c357c 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -413,6 +413,42 @@ pqDropConnection(PGconn *conn, bool flushInput)
/* Optionally discard any unread data */
if (flushInput)
conn->inStart = conn->inCursor = conn->inEnd = 0;
+#ifdef ENABLE_GSS
+ {
+ OM_uint32 min_s;
+
+ if (conn->gctx)
+ gss_delete_sec_context(&min_s, &conn->gctx, GSS_C_NO_BUFFER);
+ if (conn->gtarg_nam)
+ gss_release_name(&min_s, &conn->gtarg_nam);
+ }
+#endif
+#ifdef ENABLE_SSPI
+ if (conn->sspitarget)
+ free(conn->sspitarget);
+ conn->sspitarget = NULL;
+ if (conn->sspicred)
+ {
+ FreeCredentialsHandle(conn->sspicred);
+ free(conn->sspicred);
+ conn->sspicred = NULL;
+ }
+ if (conn->sspictx)
+ {
+ DeleteSecurityContext(conn->sspictx);
+ free(conn->sspictx);
+ conn->sspictx = NULL;
+ }
+#endif
+ if (conn->sasl_state)
+ {
+ /*
+ * XXX: if support for more authentication mechanisms is added, this
+ * needs to call the right 'free' function.
+ */
+ pg_fe_scram_free(conn->sasl_state);
+ conn->sasl_state = NULL;
+ }
/* Always discard any unsent data */
conn->outCount = 0;
}
@@ -3475,42 +3511,6 @@ closePGconn(PGconn *conn)
if (conn->lobjfuncs)
free(conn->lobjfuncs);
conn->lobjfuncs = NULL;
-#ifdef ENABLE_GSS
- {
- OM_uint32 min_s;
-
- if (conn->gctx)
- gss_delete_sec_context(&min_s, &conn->gctx, GSS_C_NO_BUFFER);
- if (conn->gtarg_nam)
- gss_release_name(&min_s, &conn->gtarg_nam);
- }
-#endif
-#ifdef ENABLE_SSPI
- if (conn->sspitarget)
- free(conn->sspitarget);
- conn->sspitarget = NULL;
- if (conn->sspicred)
- {
- FreeCredentialsHandle(conn->sspicred);
- free(conn->sspicred);
- conn->sspicred = NULL;
- }
- if (conn->sspictx)
- {
- DeleteSecurityContext(conn->sspictx);
- free(conn->sspictx);
- conn->sspictx = NULL;
- }
-#endif
- if (conn->sasl_state)
- {
- /*
- * XXX: if support for more authentication mechanisms is added, this
- * needs to call the right 'free' function.
- */
- pg_fe_scram_free(conn->sasl_state);
- conn->sasl_state = NULL;
- }
}
/*
On 06/06/2017 06:09 AM, Michael Paquier wrote:
On Thu, Jun 1, 2017 at 4:58 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
To fix, I suppose we can do what you did for SASL in your patch, and move
the cleanup of conn->gctx from closePGconn to pgDropConnection. And I
presume we need to do the same for the SSPI state too, but I don't have a
Windows set up to test that at the moment.SSPI does not complain with sslmode=prefer as each time
pg_SSPI_startup() is called conn->sspictx is enforced to NULL. This
looks wrong to me by the way as pg_SSPI_startup() is invoked only once
per authentication, and it leaks memory this way. That's also
inconsistent with SASL and GSS. At the same time this inconsistency is
not causing actual problems except a leak with SSPI in libpq, so not
doing anything except on HEAD looks fine to me.
Ok, I committed your patch, with some minor changes. I added a line to
also clear "conn->usesspi". Without that, if the server on first attempt
asked for SSPI authentication, but GSS on the second attempt, we would
incorrectly try to continue SSPI authentication during the second
attempt. Also, I kept the existing code to discard the input and output
data together, and added the new code after that, instead of in the
middle. And added some newlines to pqDropConnection for beauty.
BTW, multiple connection attempts if "host" is a list of hostnames,
which is now possible in version 10, also had the same issue. On master,
that was the easiest way to reproduce this.
I decided to backpatch this down to 9.3, after all. It is clearly a bug,
although unlikely to be hit in typical configurations. One configuration
where this can be reproduced, is if you have separate "hostnossl" and
"hostssl" lines in pg_hba.conf, for Kerberos authentication, but with
different options. If the options are such that the first
authentication, with SSL, fails, but the second one should succeed,
before this fix the second attempt would nevertheless fail with the
"duplicate authentication request".
The code in 9.2 was sufficiently different that I didn't backport it
there, out of conservatism (ok, laziness).
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 7, 2017 at 8:27 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
Ok, I committed your patch, with some minor changes.
Thanks for the commit.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers