Обсуждение: [HACKERS] 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

-- 
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/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

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

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

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"
-- 
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: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 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.

[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] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

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-hackers

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 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.
> 
> [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] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.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:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

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 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.
> > 
> > [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] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.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:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.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] 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] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

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] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.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

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] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.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

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 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.

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

-- 
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/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

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