Horton

Thread

Commits

  1. Avoid masking EOF (no-password-supplied) conditions in auth.c.

  1. lost status 'STATUS_EOF' for authentication when using 'MD5' or 'scram-sha-256'

    liulang <lang.liu@esgyn.cn> — 2024-01-02T10:31:00Z

    Hello:

   I'm an extension writer and I wrote an extension to lock users who 
tried to enter the wrong password too much. This extension is hooking 
'ClientAuthentication_hook' and checking the hook's 'status' parameter
to count the times of wrong password. It’s work fine when auth is 
'password', but get double count when auth is'MD5'or'scram-sha-256'.

problem reappearance:

1. *create an user without password*
2. set pg_hba.conf with ‘MD5’ or ‘scram-sha-256’
3. use psql without -W or -w or .pgpass file or env PGPASSFILE
4. It's ok when client is pgadmin or another
5. all of version pg can reappearance

when I read the source, find an incorret logical way on 
CheckPWChallengeAuth at src/backend/libpq/auth.c

Like
​static int
CheckPWChallengeAuth(Port *port, char **logdetail)
{
    ...
    /* the CheckMD5Auth or CheckSCRAMAuth returns STATUS_EOF because 
psql is not provide password,
     * It will prompt user to type and send the password to other 
backend process next time.
     * The current backend will exit normally.
     */

    if (port->hba->auth_method == uaMD5 && pwtype == PASSWORD_TYPE_MD5)
         auth_result = CheckMD5Auth(port, shadow_pass, logdetail);
    else
         auth_result = CheckSCRAMAuth(port, shadow_pass, logdetail);
     ...

     /*
      * If get_role_password() returned error, return error, even if the
      * authentication succeeded.
      */
     /* The get_role_password returns NULL when the user without password */
     if (!shadow_pass)
     {
         Assert(auth_result != STATUS_OK);
         /* lost STATUS_EOF */
*return STATUS_ERROR;*
     }
    ...
}

The above code does not affect the database execution,but 
ClientAuthentication_hook will be confused whether the password is 
incorrect or not currently entered?
so.. The CheckPWChallengeAuth should returns STATUS_EOF when It is, I think.

Try to change the code
​static int
CheckPWChallengeAuth(Port *port, char **logdetail)
{
    if (port->hba->auth_method == uaMD5 && pwtype == PASSWORD_TYPE_MD5)
         auth_result = CheckMD5Auth(port, shadow_pass, logdetail);
    else
         auth_result = CheckSCRAMAuth(port, shadow_pass, logdetail);
     ...

     if (STATUS_EOF == auth_result)
     {
*/* do nothing */*
     }
else if (!shadow_pass)
     {
         Assert(auth_result != STATUS_OK);
      return STATUS_ERROR;
     }
else if (auth_result == STATUS_OK)
     set_authn_id(port, port->user_name);
return auth_result;
}

I don't know if this is right, please point out. thanks!

  2. Re: lost status 'STATUS_EOF' for authentication when using 'MD5' or 'scram-sha-256'

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-01-02T20:28:40Z

    liulang <lang.liu@esgyn.cn> writes:
> The above code does not affect the database execution,but 
> ClientAuthentication_hook will be confused whether the password is 
> incorrect or not currently entered?
> so.. The CheckPWChallengeAuth should returns STATUS_EOF when It is, I think.

Yeah, I think you are right.  Overriding the subroutine's result
here is mistaken, even without considering whether it confuses any
ClientAuthentication_hook.  The whole point, as per the comments,
is to not betray to the remote end whether or not there is a user
with a password set.  But if we substitute STATUS_ERROR for
STATUS_EOF then we cause exactly that to happen: if the remote closes
the connection for send only, it can tell by whether an error comes
back whether or not the code found a password.

I think we can do it more simply than you suggest though.  Just
drop the "return STATUS_ERROR" bit; the Assert is enough.

			regards, tom lane