Refactor the code for verifying user's password.

Split md5_crypt_verify() into three functions:
* get_role_password() to fetch user's password from pg_authid, and check
  its expiration.
* md5_crypt_verify() to check an MD5 authentication challenge
* plain_crypt_verify() to check a plaintext password.

get_role_password() will be needed as a separate function by the upcoming
SCRAM authentication patch set. Most of the remaining functionality in
md5_crypt_verify() was different for MD5 and plaintext authentication, so
split that for readability.

While we're at it, simplify the *_crypt_verify functions by using
stack-allocated buffers to hold the temporary MD5 hashes, instead of
pallocing.

Reviewed by Michael Paquier.

Discussion: https://www.postgresql.org/message-id/3029e460-d47c-710e-507e-d8ba759d7cbb@iki.fi

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 9b79dc517da472a400d77ce39c6d19c150a3975e..b8ebf1b6f39a66d93774d4ce3371401452d98440 100644 (file)
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -704,6 +704,7 @@ CheckMD5Auth(Port *port, char **logdetail)
 {
    char        md5Salt[4];     /* Password salt */
    char       *passwd;
+   char       *shadow_pass;
    int         result;
 
    if (Db_user_namespace)
@@ -722,12 +723,16 @@ CheckMD5Auth(Port *port, char **logdetail)
    sendAuthRequest(port, AUTH_REQ_MD5, md5Salt, 4);
 
    passwd = recv_password_packet(port);
-
    if (passwd == NULL)
        return STATUS_EOF;      /* client wouldn't send password */
 
-   result = md5_crypt_verify(port->user_name, passwd, md5Salt, 4, logdetail);
+   result = get_role_password(port->user_name, &shadow_pass, logdetail);
+   if (result == STATUS_OK)
+       result = md5_crypt_verify(port->user_name, shadow_pass, passwd,
+                                 md5Salt, 4, logdetail);
 
+   if (shadow_pass)
+       pfree(shadow_pass);
    pfree(passwd);
 
    return result;
@@ -743,16 +748,21 @@ CheckPasswordAuth(Port *port, char **logdetail)
 {
    char       *passwd;
    int         result;
+   char       *shadow_pass;
 
    sendAuthRequest(port, AUTH_REQ_PASSWORD, NULL, 0);
 
    passwd = recv_password_packet(port);
-
    if (passwd == NULL)
        return STATUS_EOF;      /* client wouldn't send password */
 
-   result = md5_crypt_verify(port->user_name, passwd, NULL, 0, logdetail);
+   result = get_role_password(port->user_name, &shadow_pass, logdetail);
+   if (result == STATUS_OK)
+       result = plain_crypt_verify(port->user_name, shadow_pass, passwd,
+                                   logdetail);
 
+   if (shadow_pass)
+       pfree(shadow_pass);
    pfree(passwd);
 
    return result;

diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index b4ca17431a231d7dd8173b77f09e5a36d890092d..7e9124f39e828a6bdaacf1b60d33cd805c7edfa0 100644 (file)
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -30,28 +30,28 @@
 
 
 /*
- * Check given password for given user, and return STATUS_OK or STATUS_ERROR.
+ * Fetch stored password for a user, for authentication.
  *
- * 'client_pass' is the password response given by the remote user.  If
- * 'md5_salt' is not NULL, it is a response to an MD5 authentication
- * challenge, with the given salt.  Otherwise, it is a plaintext password.
+ * Returns STATUS_OK on success.  On error, returns STATUS_ERROR, and stores
+ * a palloc'd string describing the reason, for the postmaster log, in
+ * *logdetail.  The error reason should *not* be sent to the client, to avoid
+ * giving away user information!
  *
- * In the error case, optionally store a palloc'd string at *logdetail
- * that will be sent to the postmaster log (but not the client).
+ * If the password is expired, it is still returned in *shadow_pass, but the
+ * return code is STATUS_ERROR.  On other errors, *shadow_pass is set to
+ * NULL.
  */
 int
-md5_crypt_verify(const char *role, char *client_pass,
-                char *md5_salt, int md5_salt_len, char **logdetail)
+get_role_password(const char *role, char **shadow_pass, char **logdetail)
 {
    int         retval = STATUS_ERROR;
-   char       *shadow_pass,
-              *crypt_pwd;
    TimestampTz vuntil = 0;
-   char       *crypt_client_pass = client_pass;
    HeapTuple   roleTup;
    Datum       datum;
    bool        isnull;
 
+   *shadow_pass = NULL;
+
    /* Get role info from pg_authid */
    roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(role));
    if (!HeapTupleIsValid(roleTup))
@@ -70,7 +70,7 @@ md5_crypt_verify(const char *role, char *client_pass,
                              role);
        return STATUS_ERROR;    /* user has no password */
    }
-   shadow_pass = TextDatumGetCString(datum);
+   *shadow_pass = TextDatumGetCString(datum);
 
    datum = SysCacheGetAttr(AUTHNAME, roleTup,
                            Anum_pg_authid_rolvaliduntil, &isnull);
@@ -79,104 +79,151 @@ md5_crypt_verify(const char *role, char *client_pass,
 
    ReleaseSysCache(roleTup);
 
-   if (*shadow_pass == '\0')
+   if (**shadow_pass == '\0')
    {
        *logdetail = psprintf(_("User \"%s\" has an empty password."),
                              role);
+       pfree(*shadow_pass);
+       *shadow_pass = NULL;
        return STATUS_ERROR;    /* empty password */
    }
 
    /*
-    * Compare with the encrypted or plain password depending on the
-    * authentication method being used for this connection.  (We do not
-    * bother setting logdetail for pg_md5_encrypt failure: the only possible
-    * error is out-of-memory, which is unlikely, and if it did happen adding
-    * a psprintf call would only make things worse.)
+    * Password OK, now check to be sure we are not past rolvaliduntil
     */
-   if (md5_salt)
+   if (isnull)
+       retval = STATUS_OK;
+   else if (vuntil < GetCurrentTimestamp())
    {
-       /* MD5 authentication */
-       Assert(md5_salt_len > 0);
-       crypt_pwd = palloc(MD5_PASSWD_LEN + 1);
-       if (isMD5(shadow_pass))
-       {
-           /* stored password already encrypted, only do salt */
-           if (!pg_md5_encrypt(shadow_pass + strlen("md5"),
-                               md5_salt, md5_salt_len,
-                               crypt_pwd))
-           {
-               pfree(crypt_pwd);
-               return STATUS_ERROR;
-           }
-       }
-       else
+       *logdetail = psprintf(_("User \"%s\" has an expired password."),
+                             role);
+       retval = STATUS_ERROR;
+   }
+   else
+       retval = STATUS_OK;
+
+   return retval;
+}
+
+/*
+ * Check MD5 authentication response, and return STATUS_OK or STATUS_ERROR.
+ *
+ * 'shadow_pass' is the user's correct password or password hash, as stored
+ * in pg_authid.rolpassword.
+ * 'client_pass' is the response given by the remote user to the MD5 challenge.
+ * 'md5_salt' is the salt used in the MD5 authentication challenge.
+ *
+ * In the error case, optionally store a palloc'd string at *logdetail
+ * that will be sent to the postmaster log (but not the client).
+ */
+int
+md5_crypt_verify(const char *role, const char *shadow_pass,
+                const char *client_pass,
+                const char *md5_salt, int md5_salt_len,
+                char **logdetail)
+{
+   int         retval;
+   char        crypt_pwd[MD5_PASSWD_LEN + 1];
+   char        crypt_pwd2[MD5_PASSWD_LEN + 1];
+
+   Assert(md5_salt_len > 0);
+
+   /*
+    * Compute the correct answer for the MD5 challenge.
+    *
+    * We do not bother setting logdetail for any pg_md5_encrypt failure
+    * below: the only possible error is out-of-memory, which is unlikely, and
+    * if it did happen adding a psprintf call would only make things worse.
+    */
+   if (isMD5(shadow_pass))
+   {
+       /* stored password already encrypted, only do salt */
+       if (!pg_md5_encrypt(shadow_pass + strlen("md5"),
+                           md5_salt, md5_salt_len,
+                           crypt_pwd))
        {
-           /* stored password is plain, double-encrypt */
-           char       *crypt_pwd2 = palloc(MD5_PASSWD_LEN + 1);
-
-           if (!pg_md5_encrypt(shadow_pass,
-                               role,
-                               strlen(role),
-                               crypt_pwd2))
-           {
-               pfree(crypt_pwd);
-               pfree(crypt_pwd2);
-               return STATUS_ERROR;
-           }
-           if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),
-                               md5_salt, md5_salt_len,
-                               crypt_pwd))
-           {
-               pfree(crypt_pwd);
-               pfree(crypt_pwd2);
-               return STATUS_ERROR;
-           }
-           pfree(crypt_pwd2);
+           return STATUS_ERROR;
        }
    }
    else
    {
-       /* Client sent password in plaintext */
-       if (isMD5(shadow_pass))
+       /* stored password is plain, double-encrypt */
+       if (!pg_md5_encrypt(shadow_pass,
+                           role,
+                           strlen(role),
+                           crypt_pwd2))
        {
-           /* Encrypt user-supplied password to match stored MD5 */
-           crypt_client_pass = palloc(MD5_PASSWD_LEN + 1);
-           if (!pg_md5_encrypt(client_pass,
-                               role,
-                               strlen(role),
-                               crypt_client_pass))
-           {
-               pfree(crypt_client_pass);
-               return STATUS_ERROR;
-           }
+           return STATUS_ERROR;
+       }
+       if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),
+                           md5_salt, md5_salt_len,
+                           crypt_pwd))
+       {
+           return STATUS_ERROR;
        }
-       crypt_pwd = shadow_pass;
    }
 
-   if (strcmp(crypt_client_pass, crypt_pwd) == 0)
+   if (strcmp(client_pass, crypt_pwd) == 0)
+       retval = STATUS_OK;
+   else
    {
-       /*
-        * Password OK, now check to be sure we are not past rolvaliduntil
-        */
-       if (isnull)
-           retval = STATUS_OK;
-       else if (vuntil < GetCurrentTimestamp())
+       *logdetail = psprintf(_("Password does not match for user \"%s\"."),
+                             role);
+       retval = STATUS_ERROR;
+   }
+
+   return retval;
+}
+
+/*
+ * Check given password for given user, and return STATUS_OK or STATUS_ERROR.
+ *
+ * 'shadow_pass' is the user's correct password or password hash, as stored
+ * in pg_authid.rolpassword.
+ * 'client_pass' is the password given by the remote user.
+ *
+ * In the error case, optionally store a palloc'd string at *logdetail
+ * that will be sent to the postmaster log (but not the client).
+ */
+int
+plain_crypt_verify(const char *role, const char *shadow_pass,
+                  const char *client_pass,
+                  char **logdetail)
+{
+   int         retval;
+   char        crypt_client_pass[MD5_PASSWD_LEN + 1];
+
+   /*
+    * Client sent password in plaintext.  If we have an MD5 hash stored, hash
+    * the password the client sent, and compare the hashes.  Otherwise
+    * compare the plaintext passwords directly.
+    */
+   if (isMD5(shadow_pass))
+   {
+       if (!pg_md5_encrypt(client_pass,
+                           role,
+                           strlen(role),
+                           crypt_client_pass))
        {
-           *logdetail = psprintf(_("User \"%s\" has an expired password."),
-                                 role);
-           retval = STATUS_ERROR;
+           /*
+            * We do not bother setting logdetail for pg_md5_encrypt failure:
+            * the only possible error is out-of-memory, which is unlikely,
+            * and if it did happen adding a psprintf call would only make
+            * things worse.
+            */
+           return STATUS_ERROR;
        }
-       else
-           retval = STATUS_OK;
+       client_pass = crypt_client_pass;
    }
+
+   if (strcmp(client_pass, shadow_pass) == 0)
+       retval = STATUS_OK;
    else
+   {
        *logdetail = psprintf(_("Password does not match for user \"%s\"."),
                              role);
-
-   if (crypt_pwd != shadow_pass)
-       pfree(crypt_pwd);
-   if (crypt_client_pass != client_pass)
-       pfree(crypt_client_pass);
+       retval = STATUS_ERROR;
+   }
 
    return retval;
 }

diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index 4ca8a75c4681118bcce523096ff17b7931c817b4..229ce76b61e87bca1fd6168626c3c96f4b1950fd 100644 (file)
--- a/src/include/libpq/crypt.h
+++ b/src/include/libpq/crypt.h
@@ -15,7 +15,12 @@
 
 #include "datatype/timestamp.h"
 
-extern int md5_crypt_verify(const char *role, char *client_pass,
-                char *md5_salt, int md5_salt_len, char **logdetail);
+extern int get_role_password(const char *role, char **shadow_pass, char **logdetail);
+
+extern int md5_crypt_verify(const char *role, const char *shadow_pass,
+                const char *client_pass, const char *md5_salt,
+                int md5_salt_len, char **logdetail);
+extern int plain_crypt_verify(const char *role, const char *shadow_pass,
+                  const char *client_pass, char **logdetail);
 
 #endif