Thread: [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
Attachment
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
Attachment
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
Attachment
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
Attachment
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