From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: Server ignores contents of SASLInitialResponse
Date: Thu, 1 Jun 2017 14:58:36 +0300
Message-ID: <6288d80e-a0bf-d4d3-4e12-7b79c77f1771@iki.fi> (raw)
In-Reply-To: <CAB7nPqTqytzGryms4zJb8xZ98YkMs==3ntMBf-mvjNK_SqEj0g@mail.gmail.com>
References: <CAB7nPqR0G5aF2_kc_LH29knVqwvmBc66TF5DicvpGVdke68nKw@mail.gmail.com>
<5e4ac357-077a-470b-4014-9d9e12f833ba@iki.fi>
<CAB7nPqTKjX5Pj588gGsgh9bYhEW=bF5UoBG+0Vocqstf6hpzqQ@mail.gmail.com>
<CAB7nPqQO9RijoeCYxY74v0eFq7jiLHWSy_eiidPqC1f1toFyfA@mail.gmail.com>
<CAB7nPqTqytzGryms4zJb8xZ98YkMs==3ntMBf-mvjNK_SqEj0g@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-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 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.
>
> 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:0001-Fix-double-free-bug-in-GSS-authentication.patch (invalid/octet-stream, 1.9K) download
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.cindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: hlinnaka@iki.fi, michael.paquier@gmail.com
Subject: Re: Server ignores contents of SASLInitialResponse
In-Reply-To: <6288d80e-a0bf-d4d3-4e12-7b79c77f1771@iki.fi>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox